Re: [Xen-devel] [PATCH 09/10] libxl: introduce libxl_fd_set_nonblock, rationalise _cloexec

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-06 at 20:35 +0000, Ian Jackson wrote:
> We want a function for setting fds to nonblocking, so introduce one.
> 
> This is a very similar requirement to that for libxl_fd_set_cloexec,
> so make it common with that.
> 
> While we're at it, fix a few deficiences that make this latter
> function less desirable than it could be:
>  * Change the return from 0/-1 (like a syscall) to a libxl error code
>  * Take a boolean parameter for turning the flag on and off
>  * Log on error (and so, take a ctx for this purpose)
> 
> Change callers of libxl_fd_set_cloexec to notice errors.  (Although,
> such errors are highly unlikely.)
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

(although I notice that you've slipped back into the
comment-follows-prototype style...)

> ---
>  tools/libxl/libxl.c      |   33 ++++++++++++++++++++++++++-------
>  tools/libxl/libxl.h      |    6 +++++-
>  tools/libxl/libxl_qmp.c  |    3 ++-
>  tools/libxl/xl_cmdimpl.c |    3 ++-
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index f6370d4..42b64d8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3647,19 +3647,38 @@ int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t 
> poolid, uint32_t domid)
>      return 0;
>  }
>  
> -int libxl_fd_set_cloexec(int fd)
> +static int fd_set_flags(libxl_ctx *ctx, int fd,
> +                        int fcntlgetop, int fcntlsetop, const char *fl,
> +                        int flagmask, int set_p)
>  {
> -    int flags = 0;
> +    int flags, r;
>  
> -    if ((flags = fcntl(fd, F_GETFD)) == -1) {
> -        flags = 0;
> +    flags = fcntl(fd, fcntlgetop);
> +    if (flags == -1) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fcntl(,F_GET%s) failed",fl);
> +        return ERROR_FAIL;
>      }
> -    if ((flags & FD_CLOEXEC)) {
> -        return 0;
> +
> +    if (set_p)
> +        flags |= flagmask;
> +    else
> +        flags &= ~flagmask;
> +
> +    r = fcntl(fd, fcntlsetop, flags);
> +    if (r == -1) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fcntl(,F_SET%s) failed",fl);
> +        return ERROR_FAIL;
>      }
> -    return fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
> +
> +    return 0;
>  }
>  
> +int libxl_fd_set_cloexec(libxl_ctx *ctx, int fd, int cloexec)
> +  { return fd_set_flags(ctx,fd, F_GETFD,F_SETFD,"FD", FD_CLOEXEC, 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); 
> }
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 97ca25e..5396ba8 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -613,7 +613,11 @@ const char *libxl_run_dir_path(void);
>  const char *libxl_xenpaging_dir_path(void);
>  
>  /* misc */
> -int libxl_fd_set_cloexec(int fd);
> +int libxl_fd_set_cloexec(libxl_ctx *ctx, int fd, int cloexec);
> +int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
> +  /* Each of these sets or clears the flag according to whether the
> +   * 2nd parameter is nonzero.  On failure, they log, and
> +   * return ERROR_FAIL, but also leave errno valid. */
>  
>  #include <libxl_event.h>
>  
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 58f5283..05df4d7 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -324,7 +324,8 @@ static int qmp_open(libxl__qmp_handler *qmp, const char 
> *qmp_socket_path,
>      if (fcntl(qmp->qmp_fd, F_SETFL, flags | O_NONBLOCK) == -1) {
>          return -1;
>      }
> -    libxl_fd_set_cloexec(qmp->qmp_fd);
> +    ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
> +    if (ret) return -1;
>  
>      memset(&qmp->addr, 0, sizeof (&qmp->addr));
>      qmp->addr.sun_family = AF_UNIX;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index a18c6b2..b88fc8a 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1490,7 +1490,8 @@ static int create_domain(struct domain_create *dom_info)
>              restore_fd = migrate_fd;
>          } else {
>              restore_fd = open(restore_file, O_RDONLY);
> -            libxl_fd_set_cloexec(restore_fd);
> +            rc = libxl_fd_set_cloexec(ctx, restore_fd, 1);
> +            if (rc) return rc;
>          }
>  
>          CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, &hdr,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel