Kyle,
Could you provide another patch which addresses Bert's concerns? I'm
hesitant to nominate r37615 for 1.6.x based upon this feedback.
-Hyrum
On May 7, 2009, at 9:53 AM, Bert Huijben wrote:
> Hi Kyle,
>
> (Sorry for top posting, but my mail client doesn’t allow me to add
> something between the other parts)
>
> Apr internally never checks a header before calling exit(). It just
> checks the define before #including the specific header named
> stdlib.h, so it requires exit() to be inside stdlib.h or any already
> included header.
>
> Apr defines apr_proc_fork() as a portable way to call fork
> (declared in <apr_thread_proc.h> when APR_HAS_FORK is true)
>
> Bert
>
> From: Kyle McKay [mailto:mackyle_at_gmail.com]
> Sent: donderdag 7 mei 2009 15:57
> To: dev_at_subversion.tigris.org
> Subject: Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn
>
> On May 7, 2009, at 00:20, Bert Huijben <rhuijben_at_sharpsvn.net> wrote:
>
> +static apr_status_t detach_child_cleanup(void *data)
> +{
> +#if APR_HAVE_STDLIB_H
> +#if APR_HAVE_UNISTD_H
> +#if APR_HAS_FORK
>
> I don't think we need the two outer #if's. (We only need the check for
> fork()).
>
>
> The exit function is declared in stdlib.h. It's not appropriate to
> call it if we haven't included the header where it's defined (hence
> the test for APR_HAVE_STDLIB_H).
>
> The fork function is declared in unistd.h, it's not appropriate to
> try and call it if we haven't included the header where it's defined
> (hence the test for APR_HAVE_UNISTD_H).
>
> For this patch it's also not appropriate to call fork if APR isn't
> using it (hence the test for APR_HAS_FORK).
>
> So the patch code is only compiled in if you have an stdlib.h (for
> exit()) and a unistd.h (for fork()) AND APR is using fork.
>
> Are you certain that there aren't some APR-supported and Subversion
> supported platforms where APR_HAS_FORK is true but one or the other
> of the standard headers that declare fork/exit are missing? Some
> older systems perhaps...
>
> These defines check whether a specific header file exist and not if
> functionality exist.
>
>
> Actually the tests are there to avoid calling functions for which we
> haven't included the header file which declares them.
>
> Using these checks is most likely a dumb guess on whether this is
> running on
> a Unix system. (stdlib.h is available on any operating system I know
> and
> probably part of recent C standards).
>
>
> Bert
>
>
> If stdlib.h is available everywhere, then why does apr define a test
> for it rather than just always use it?
>
> Since apr does define a test for APR_HAVE_STDLIB_H, that suggests
> apr supports systems without it and that the patch code should too.
>
> Kyle
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2096689
Received on 2009-05-07 17:38:47 CEST