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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.07.14 at 19:07, <Ian.Jackson@xxxxxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel