dup3/O_CLOEXEC/F_DUPFD_CLOEXEC

[Eric Blake]

According to Corinna Vinschen on 1/14/2010 3:25 AM:
>>> +	  if (flags & O_CLOEXEC)
>>> +	    newfh->set_close_on_exec (true);
>> Is that a typo?
>>
>>> +	  else
>>> +	    newfh->close_on_exec (false);
>> If not, why not just newfh->close_on_exec (!!(flags & O_CLOEXEC)), instead
>> of the if-else?
> 
> There's a difference.  set_close_on_exec() calls SetHandleInformation()
> via one or more intermediate calls to fhandler_base::set_no_inheritance().
> close_on_exec() only sets the flag in the fhandler status flags.  The
> if-else avoids two or more unnecessary function calls in the dup/dup2 case.

Is there a window here where a newly created HANDLE is inheritable even
though it will later be marked cloexec?  The whole point of O_CLOEXEC in
POSIX is that the action is atomic, and that no other thread can ever win
the race to leak the handle into an exec'd child process.  Which implies
that anywhere possible, we should be requesting noinherit as part of
creating the HANDLE, rather than changing it after the fact.  Most of your
patch appeared to be doing that (with the sec_none_nih vs. sec_none
conditionals), but calling set_close_on_exec seems like it is too late.

>>                 (cmd == F_DUPFD_CLOEXEC) * O_CLOEXEC);
> 
> Ouch, no, thanks.  I'd rather ?: it.

No problem - that's just style, and a good compiler will (hopefully) emit
the same code for that.

>> According to spec, Linux dup3(-1,-1,0) can fail with either EBADF or
>> EINVAL; I haven't tested that on Linux yet, but am assuming that your
>> choice of EBADF for this condition was intentional?
> 
> Erm... EINVAL, and yes, it was intentional, per the Linux man page:
> 
>   ERRORS
>     [...]
>     EINVAL (dup3()) flags contain an invalid value.  Or, oldfd was equal to
> 	   newfd.

The Linux man page did not give a preference between EINVAL and EBADF.
dup3(-1,-1,0) and dup3(1000000,1000000,0) are instances where both errors
are equally applicable to the situation, but EINVAL is probably less
computing power to determine.  But your code picked different errnos for
the two cases.  Unfortunately, the gnulib unit test that I posted did not
test those two conditions to see which errno Linux preferred (and whether
it was the same errno for both calls).  But since Linux is the only
platform with dup3, it would be nice to guarantee the same errno for both
of those calls.  I guess that means I should take some time to actually
compile a simple app on Linux to test that behavior.

> That's very easy to answer:  Because http://cygwin.com/acronyms/#SHTDI
> and I have only so much time.  I'm just trying to lay the groundwork
> here.  I'm not at all opposed to patches adding the still missing
> functionality.

Fair enough.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 320 bytes
Desc: OpenPGP digital signature
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20100114/0f59a3f2/attachment.sig>