Re: [Qemu-devel] [PATCH v5] qemu-nbd: Implement socket activation.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

On 02/04/2017 04:03 AM, Richard W.M. Jones wrote:
> Socket activation (sometimes known as systemd socket activation)
> allows an Internet superserver to pass a pre-opened listening socket
> to the process, instead of having qemu-nbd open a socket itself.  This
> is done via the LISTEN_FDS and LISTEN_PID environment variables, and a
> standard file descriptor range.
> 
> This change partially implements socket activation for qemu-nbd.  If
> the environment variables are set correctly, then socket activation
> will happen automatically, otherwise everything works as before.  The
> limitation is that LISTEN_FDS must be 1.
> 
> Signed-off-by: Richard W.M. Jones.
> ---

> +
> +    /* So the file descriptors don't leak into child processes. */
> +    for (i = 0; i < nr_fds; ++i) {
> +        fd = FIRST_SOCKET_ACTIVATION_FD + i;
> +        if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) {

This is not the right way to do things (it risks clearing any other FD_
flags currently set on the fd, even though there aren't any such FD_
flags in common use).  You HAVE to do F_GETFD before F_SETFD
(read-modify-write) for correct usage - and the best way to do that is
to use qemu_set_cloexec() from util/oslib-posix.c.

Except that qemu_set_cloexec() doesn't pay attention to errors...

> +            /* If we cannot set FD_CLOEXEC then it probably means the file
> +             * descriptor is invalid, so socket activation has gone wrong
> +             * and we should exit.
> +             */
> +            error_report("Socket activation failed: "
> +                         "invalid file descriptor fd = %d: %m",
> +                         fd);

...while you use it for argument validation.  Hmm.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]