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

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=2095949
Received on 2009-05-07 16:54:10 CEST