[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#21, @zturner wrote:
>>>! In D6204#18, @clayborg wrote:
>> I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:
>> 
>> ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);
>> 
>> Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;
>> 
>> And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls
> 
> Wouldn't it be a little weird having a parameter on the Connect() method, which would then require modifying the base Connection class which is non-specific to files?

I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. I see 2 unclear things for me here:
- As I can see LLVM functions/methods follow the  

>>! In D6204#21, @zturner wrote:
>>>! In D6204#18, @clayborg wrote:
>> I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:
>> 
>> ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);
>> 
>> Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;
>> 
>> And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls
> 
> Wouldn't it be a little weird having a parameter on the Connect() method, which would then require modifying the base Connection class which is non-specific to files?

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.

http://reviews.llvm.org/D6204