Login | Register
My pages Projects Community openCollabNet

Discussions > dev [DISABLED] > Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn

subversion
Discussion topic

Back to topic list

Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn

Author hwright
Full name Hyrum K. Wright
Date 2009-05-07 08:38:25 PDT
Message 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 dot com]
> Sent: donderdag 7 mei 2009 15:57
> To: dev at subversion dot tigris dot org
> Subject: Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn
>
> On May 7, 2009, at 00:20, Bert Huijben <rhuijben at sharpsvn dot 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
>

« Previous message in topic | 4 of 5 | Next message in topic »

Messages

Show all messages in topic

Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn mackyle Kyle McKay 2009-05-07 06:56:56 PDT
     RE: svn commit: r37615 - trunk/subversion/libsvn_ra_svn rhuijben Bert Huijben 2009-05-07 07:53:28 PDT
         Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn mackyle Kyle McKay 2009-05-07 08:37:28 PDT
         Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn hwright Hyrum K. Wright 2009-05-07 08:38:25 PDT
             Re: svn commit: r37615 - trunk/subversion/libsvn_ra_svn mackyle Kyle McKay 2009-05-07 08:49:25 PDT
Messages per page: