Re: [PATCH for-4.6] libxl: clear O_NONBLOCK|O_NDELAY on migration fd and reinstate afterwards

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

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 <ian.campbell@citrix.com>
> Cc: Jim Fehlig <jfehlig@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> Cc: Yang Hongyang <yanghy@cn.fujitsu.com>
> ---
> 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