[Lldb-commits] [PATCH] Apply SOCK_CLOEXEC flag to socket API functions in order to avoid handle leakage to a forked child process.

[Oleksiy Vyalov]

>>! In D6204#23, @zturner wrote:
>>>! In D6204#22, @ovyalov wrote:
>> I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. Additionally there are a few unclear things for me here:
>> - As I can see LLVM/LLDB functions/methods follow the convention where Error is always a last parameter if provided.
>> - If options struct is defined in ConnectionFileDescriptor it means that this struct type should be visible to Socket. In this case we may have mutual referencing between Socket and ConnectionFileDescriptor.
>> - If default constructor for the Options sets m_child_processes_inherit to true it means that we need to pass explicitly Options{false} at each ConnectionFileDescriptor::Connect call site, right?
>> 
>> Thank you.
> 
> I don't think we need an options struct at all.  Why not just make it a bool?
> 

I'm okay with having bool variable in ConnectionFileDescriptor but then question arises how to pass this value to multiple Socket factory methods - TcpConnect, UdpConnect, TcpListen,... I'd rather pass child_processes_inherit to them as a part Options struct (or bit-masked int flags) - so, if we'll need to support a new option here (e.g., non-blocking IO) no need to modify Socket method signatures again.

> Another, perhaps better option instead of passing it into the default constructor is to make a public method on CFD called SetChildProcessInherit(bool).  This way you can set the value, call Connect, set the value to something else, call connect again, etc.
> 
> Since this inheritance is specific to files, anyone who wants to call this method already knows they're working with a >ConnectionFileDescriptor, and not some other kind of Connection, so they already have a pointer or reference to a >ConnectionFileDescriptor, and not the base class.  So they can easily call this method.  Then just pass it through as a bool, and not >as an Options structure.

Makes sense.

http://reviews.llvm.org/D6204