Re: [Xen-devel] [PATCH] libxl: use libxl_fd_set_{cloexec, nonblock} helpers

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jan Beulich writes ("[PATCH] libxl: use libxl_fd_set_{cloexec,nonblock} 
helpers"):
> ... instead of open-coding them or not using them at all. This in
> particular fixes a build (and presumably also runtime) problem on old
> enough libc due to the recent introduction of a use of O_CLOEXEC. The
> other two changes are only of cleanup kind.

I think most of libxl should probably be using libxl__carefd* not
setting cloexec.  This is necessary whenever we need cloexec for
correctness rather than just out of tidiness.  (Search for carefd in
libxl_internal.h.)

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -977,7 +977,7 @@ void libxl__xc_domain_restore_done(libxl
>      libxl_ctx *ctx = libxl__gc_owner(gc);
...
> +    ret = libxl_fd_set_nonblock(ctx, fd, 0);
> +    if (ret)
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unable to put restore fd"
>                           " back to blocking mode");

You do not need to log when libxl_fd_set_nonblock fails - it logs
already.

> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -358,19 +358,14 @@ static int qmp_open(libxl__qmp_handler *
>                      int timeout)
...
>      qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0);
...
> +    ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1);
> +    if (ret) return -1;

This change is correct.

>      ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
>      if (ret) return -1;

But this (which you haven't touched) needs to be done with
libxl__carefd* instead.  (Your patch is not unacceptable due to you
not making this change, on the basis that we should accept
improvements even if they don't leave the code perfect.)

> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -1047,11 +1047,17 @@ int libxl__random_bytes(libxl__gc *gc, u
...
> -    fd = open(dev, O_RDONLY | O_CLOEXEC);
> +    fd = open(dev, O_RDONLY);

Firstly, I don't understand why we care about cloexec on this fd, but
it's harmless to do so.

> +    ret = libxl_fd_set_cloexec(CTX, fd, 1);
> +    if (ret) {
> +        close(fd);
> +        LOGE(ERROR, "failed to set close-on-exec on handle to \"%s\"", dev);
> +        return ERROR_FAIL;
> +    }

Again, no need to log.


Thanks,
Ian.

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