Pierre A. Humblet
Mon Feb 2 14:47:00 GMT 2004
Corinna Vinschen wrote:
> On Feb 1 16:57, Pierre A. Humblet wrote:
> > At 01:39 PM 2/1/2004 -0500, Christopher Faylor wrote:
> > >On Sat, Jan 31, 2004 at 02:18:48PM -0500, Pierre A. Humblet wrote:
> > >>Fortunately it is never used in the case of spawn: all handles are
> > >>inherited, or the parent does the work (sockets).
> > >
> > >The one placed the handle is actually used is in
> > >fhandler_socket::fixup_after_exec. I'd like Corinna's confirmation
> > >before this patch is checked in.
> > Good idea. FWIW, I checked that one carefully. That's why I found
> > the secret_event bug a while back. I also tested on Win95 with an
> > old winsock.
> > It looks like the handle might be used, but the tests for close
> > on exec always block the paths where it is actually used.
> AFAICS, you're right. fhandler_socket::fixup_after_exec calls
> fhandler_socket::fixup_after_fork only if !close_on_exec.
> fhandler_socket::fixup_after_fork in turn calls fork_fixup which only
> uses the parent handle if close_on_exec. So the parent handle is never
> used in this scenario. So I think it's ok to drop the parent handle.
Do you want to apply the patch now, or I do it this evening?
To simplify the life of future maintainers, I was thinking of
redoing fixup_after_exec as follows
(instead of going through the cascade of calls above)
SOCKET new_sock = WSASocketA (FROM_PROTOCOL_INFO,
prot_info_ptr, 0, 0);
debug_printf("%x = WSASocketA(FROM_PROTOCOL_INFO)", new_sock);
if (new_sock) /* winsock2 */
set_io_handle ((HANDLE) new_sock);
> As a side note, it took me a while to understand that it's the same
> situation for the secret_event handle. The problem is the name of the
> function set_inheritance(). The second parameter is the *negation*
> of the inheritance. IMHO this is rather confusing. Either we should
> rename the function to set_no_inheritance or we should revert the
> meaning of the second parameter.
I am all for that! Renaming the function has my preference.
More information about the Cygwin-patches