From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [Xen-users] "xl restore" leaks a file descriptor?
Date: Thu, 13 Aug 2015 10:17:10 +0100
Message-ID: <1439457430.23981.27.camel@citrix.com>
References: <CA+jCKRUSxG3nFC=BJCqKy=kABrN27Nde4A67bxBEm5TYD71yPA@mail.gmail.com>
	<1439283311.9747.193.camel@citrix.com>
	<CA+jCKRVqL4DOYZK-etugCnVRhOocVKYdhGQWG4XYCqWZUWcmfA@mail.gmail.com>
	<1439308093.9747.291.camel@citrix.com>
	<20150811170725.GU7460@zion.uk.xensource.com>
	<1439368873.9747.302.camel@citrix.com>
	<20150812094918.GY7460@zion.uk.xensource.com>
	<1439373865.9747.330.camel@citrix.com>
	<20150812171245.GA7460@zion.uk.xensource.com>
	<1439455176.23981.12.camel@citrix.com>
	<20150813085017.GG7460@zion.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20150813085017.GG7460@zion.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wei Liu <wei.liu2@citrix.com>
Cc: xen-users@lists.xenproject.org, Andrew Armenia <andrew@asquaredlabs.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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