From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH for-4.6] libxl: clear O_NONBLOCK|O_NDELAY on migration fd and reinstate afterwards Date: Fri, 11 Sep 2015 14:44:01 +0100 Message-ID: <55F2DAA1.1050902@citrix.com> References: <1441968171-15844-1-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1441968171-15844-1-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , ian.jackson@eu.citrix.com, wei.liu2@citrix.com, xen-devel@lists.xen.org Cc: Shriram Rajagopalan , Jim Fehlig , Yang Hongyang List-Id: xen-devel@lists.xenproject.org On 11/09/15 11:42, Ian Campbell wrote: > The fd passed to us by libvirt for both save and restore has at least > O_NONBLOCK set, which libxl does not expect and therefore fails to > handle any EAGAIN which might arise. > > This has been observed with migration v2, but if v1 used to work I > think that would be just be by luck and/or coincidence. > > Unix convention (and the principal of least surprise) is usually to > ensure that an fd has no "strange" properties, such as being > non-blocking, when handing it to another component. > > However for the convenience of the application arrange instead for > libxl to clear any unexpected flags on the file descriptors it is > given for save or restore and restore them to their original state at > the end. O_NDELAY could be similarly problematic so clear that as > well as O_NONBLOCK. > > To do this introduce a pair of new helper functions one to modify+save > the flags and another to restore them and call them in the appropriate > places. > > The migration v1 code appeared to do some things with O_NONBLOCK in > the checkpoint case. Migration v2 doesn't seem to do so, and in any > case I wouldn't expect it to be relying on libvirt's setting of > O_NONBLOCK when xl doesn't use that flag. > > Signed-off-by: Ian Campbell > Cc: Jim Fehlig > Cc: Andrew Cooper > Cc: Shriram Rajagopalan > Cc: Yang Hongyang > --- > For 4.6: This fixes migration with libvirt, which I think is worth > doing before the release. > > For backports: Once "ts-xen-install: Rewrite /etc/hosts to comment out > 127.0.1.1 entry" passes through osstest's pretest gate and has run on > some of the older branches we should then know if this is necessary > for migration v1. Or we could backport it regardless. I don't believe any special consideration is needed for the legacy conversion case, as all other fds used there are created by components we control. > --- > tools/libxl/libxl.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_create.c | 23 +++++++++++++++- > tools/libxl/libxl_internal.h | 13 +++++++++ > 3 files changed, 100 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 4f2eb24..d6efdd8 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -952,6 +952,12 @@ static void domain_suspend_cb(libxl__egc *egc, > libxl__domain_suspend_state *dss, int rc) > { > STATE_AO_GC(dss->ao); > + int flrc; > + > + flrc = libxl__fd_flags_restore(gc, dss->fd, dss->fdfl); > + /* If suspend has failed already then report that error not this one. */ > + if (flrc && !rc) rc = flrc; > + > libxl__ao_complete(egc,ao,rc); > > } > @@ -980,6 +986,11 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags, > dss->live = flags & LIBXL_SUSPEND_LIVE; > dss->debug = flags & LIBXL_SUSPEND_DEBUG; > > + rc = libxl__fd_flags_modify_save(gc, dss->fd, > + ~(O_NONBLOCK|O_NDELAY), 0, > + &dss->fdfl); > + if (rc < 0) goto out_err; > + > libxl__domain_save(egc, dss); > return AO_INPROGRESS; > > @@ -6507,6 +6518,60 @@ int libxl_fd_set_cloexec(libxl_ctx *ctx, int fd, int cloexec) > int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock) > { return fd_set_flags(ctx,fd, F_GETFL,F_SETFL,"FL", O_NONBLOCK, nonblock); } > > +int libxl__fd_flags_modify_save(libxl__gc *gc, int fd, > + int mask, int val, int *r_oldflags) > +{ > + int rc, ret, fdfl; > + > + fdfl = fcntl(fd, F_GETFL); > + if (fdfl < 0) { > + LOGE(ERROR, "failed to fcntl.F_GETFL for fd %d", fd); > + rc = ERROR_FAIL; > + goto out_err; > + } > + > + LOG(DEBUG, "fnctl F_GETFL flags for fd %d are %x", fd, fdfl); %#x to distinguish decimal and hex numbers in the same message (and other debug messages) ~Andrew