On May 7, 2009, at 00:20, Bert Huijben <rhuijben_at_sharpsvn.net> wrote:
>> -----Original Message-----
>> From: Hyrum K. Wright [mailto:hyrum_at_hyrumwright.org]
>> Sent: woensdag 6 mei 2009 18:31
>> To: svn_at_subversion.tigris.org
>> Subject: svn commit: r37615 - trunk/subversion/libsvn_ra_svn
>>
>> Author: hwright
>> Date: Wed May 6 09:31:28 2009
>> New Revision: 37615
>>
>> Log:
>> A better fix to issue #2580.
>>
>> Fix ssh zombie problem introduced with revision 35533
>>
>> * subversion/libsvn_ra_svn/client.c
>> (detach_child_cleanup): New.
>> (make_tunnel): Fully detach tunnel process to avoid having it
>> receive
>> signals
>> while restoring the original apr_pool_note_subprocess to avoid
>> creating
>> zombies.
>>
>> Patch by: Kyle McKay <mackyle_at_gmail.com>
>>
>> Modified:
>> trunk/subversion/libsvn_ra_svn/client.c
>>
>> Modified: trunk/subversion/libsvn_ra_svn/client.c
>> URL:
>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_svn/client
>> .
>> c?pathrev=37615&r1=37614&r2=37615
>> =
>> =
>> =====================================================================
>> =======
>> --- trunk/subversion/libsvn_ra_svn/client.c Wed May 6 09:02:16 2009
>> (r37614)
>> +++ trunk/subversion/libsvn_ra_svn/client.c Wed May 6 09:31:28 2009
>> (r37615)
>> @@ -26,6 +26,12 @@
>> #include <apr_strings.h>
>> #include <apr_network_io.h>
>> #include <apr_uri.h>
>> +#if APR_HAVE_STDLIB_H
>> +#include <stdlib.h>
>> +#endif
>> +#if APR_HAVE_UNISTD_H
>> +#include <unistd.h>
>> +#endif
>>
>> #include "svn_types.h"
>> #include "svn_string.h"
>> @@ -426,6 +432,43 @@ static void handle_child_process_error(a
>> svn_error_clear(svn_ra_svn_flush(conn, pool));
>> }
>>
>> +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=2095402
Received on 2009-05-07 15:57:14 CEST