Re: [Xen-users] "xl restore" leaks a file descriptor?

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/08/15 10:17, Ian Campbell wrote:
> On Thu, 2015-08-13 at 09:50 +0100, Wei Liu wrote:
>> On Thu, Aug 13, 2015 at 09:39:36AM +0100, Ian Campbell wrote:
>>> On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
>>>> On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
>>>> [...]
>>>>>>> As Andy says I think we want restore_fd in the check, I can't 
>>>>>>> see 
>>>>>>> any
>>>>>>> reason we wouldn't want to close the socket too.
>>>>>>>
>>>>>> Do you mean migrate_fd when you say "socket"?
>>>>> In the migrate case we do "restore_fd = migrate_fd;", so yes, 
>>>>> indirectly.
>>>>>
>>>>>
>>>>>>  I tried that, but that led
>>>>>> to failure because toolstack still needs to get controlling 
>>>>>> information
>>>>>> out of it (the "GO" message).
>>>>>>
>>>>>> Maybe I close this too early.
>>>>> Right.
>>>>>
>>>> I look at the code. Even if we should close that socket, it should 
>>>> not
>>>> happen inside create_domain, because the caller (migrate_receive) 
>>>> needs
>>>> that fd.
>>>>
>>>> IMO create_domain should only close restore_fd if that fd is opened 
>>>> by
>>>> itself.
>>> That makes sense, yes. The close should probably have an associated 
>>> comment
>>> since this will be a bit subtle.
>>>
>>> Perhaps rather than trying to repeat the conditions which lead to it 
>>> being
>>> opened we should just do:
>>>     int restore_fd_to_close = -1;
>>>     ...
>>>     restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
>>>     ...
>>>     if (restore_fd_to_close >= 0) {
>>>         close(restore_fd_to_close);
>>>         restore_fd_to_close = -1;
>>>     }
>>>
>>> Strictly speaking we ought to check the return of close too I suppose.
>>>
>> What would we do in case close fails?
> Maybe just log?
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html says
> we can get:
>
> EBADF, which would be an error in our code.
> EINTR, ...
> EIO, which would be from disk full or a disk dying or something.
>
> We (and most code in general) tends to not worry about close failing too
> much, there's an argument for that too...

The return value really shouldn't be ignored.

Logging loudly is about all which can be done, and that is fine, but the
user really does need to be aware that something bad went wrong.

~Andrew