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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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...

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