Re: [PATCH] libxl: use libxl_fd_set_{cloexec, nonblock} helpers

[David Vrabel <david.vrabel@citrix.com>]

On 28/07/14 07:32, Jan Beulich wrote:
>>>> On 25.07.14 at 19:07, <Ian.Jackson@eu.citrix.com> wrote:
>> Jan Beulich writes ("[PATCH] libxl: use libxl_fd_set_{cloexec,nonblock} 
>>> --- a/tools/libxl/libxl_qmp.c
>>> +++ b/tools/libxl/libxl_qmp.c
>>> @@ -358,19 +358,14 @@ static int qmp_open(libxl__qmp_handler *
>>>                      int timeout)
>> ...
>>>      qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0);
>> ...
>>> +    ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1);
>>> +    if (ret) return -1;
>>
>> This change is correct.
>>
>>>      ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
>>>      if (ret) return -1;
>>
>> But this (which you haven't touched) needs to be done with
>> libxl__carefd* instead.  (Your patch is not unacceptable due to you
>> not making this change, on the basis that we should accept
>> improvements even if they don't leave the code perfect.)
> 
> And indeed I'd like to leave that conversion to you or someone
> else more familiar with the libxl concepts.
> 
>>> --- a/tools/libxl/libxl_utils.c
>>> +++ b/tools/libxl/libxl_utils.c
>>> @@ -1047,11 +1047,17 @@ int libxl__random_bytes(libxl__gc *gc, u
>> ...
>>> -    fd = open(dev, O_RDONLY | O_CLOEXEC);
>>> +    fd = open(dev, O_RDONLY);
>>
>> Firstly, I don't understand why we care about cloexec on this fd, but
>> it's harmless to do so.
> 
> And of course I'd be perfectly fine with a one liner just dropping it
> (all I really care about is that the file builds correctly in all cases).
> The question of why cloexec is needed/wanted here I'll pass on to
> author and committer, but I'd guess this is just to avoid
> proliferation of file handles into clones.

Surely it should be standard practice for a library to always set
CLOEXEC on any files it opens, to minimize the side effects the library
call has?

And if CLOEXEC is required then setting it at open time is the only safe
way.

David