On Jan 13 16:49, Christopher Faylor wrote: > On Wed, Jan 13, 2010 at 10:25:37PM +0100, Corinna Vinschen wrote: > >Hi, > > > >the below patch implements the Linux dup3/O_CLOEXEC/F_DUPFD_CLOEXEC > >extension. I hope I didn't miss anything important since it affects > >quite a few fhandlers. Fortunately most is mechanical change, except > >for a few places (dtable.cc, pipe.cc, fhandeler_fifo.cc, syscalls.cc). > >Nevertheless, I'd be glad if somebody could have a second look into > >this. > > > >Eric, you asked for it in the first place, do you have a fine testcase > >for this functionality? > > The number of times that you typed: > > sa_buf = close_on_exec () > ? sec_user_nih ((PSECURITY_ATTRIBUTES) char_sa_buf, > cygheap->user.sid()) > : sec_user ((PSECURITY_ATTRIBUTES) char_sa_buf, > cygheap->user.sid()); > > implies that this should be a macro or a function.
The combination with sec_none_nih/sec_none is used four times in different fhandler files. Yes, good idea, I'll create an inline function in fhandler.h. The above combination with sec_user_nih/sec_user is only used two times, both in fhandler_fifo.cc. What about an inline function in fhandler_fifo.cc for this one? I'll add that to the revised patch. > Could the setting of close_on_exec be handled in the syscalls.cc open() > so that it doesn't have to be done so many times? You could have Yesterday I was sure that it has to be set in the various open methods since they could be called from elsewhere. Today, after a nights sleep, I'm not so sure anymore. I don't see any call to fh->open outside of open(2). And calls to the open_fs() function are covered anyway. I'll look into simplifying this. > build_fh_name set the noexec flag so that close_on_exec() would still > work in the fhandler_*::open functions. I'm not sure I understand you correctly. Do you mean build_fh_name should already set the close_on_exec flag so that later fhandler_*::open only have to call set_close_on_exec if a set_no_inheritance call is required? build_fh_name does not get a flag parameter so far. That would have to be added as default parameter. For now I'll go the road to add the default close_on_exec setting to the open(2) call. It's easy to switch to build_fh_name from there. Thanks for the review, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat