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

[Daniel P. Berrange]

On Thu, Feb 02, 2017 at 02:51:59PM +0000, 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.
> ---
>  qemu-nbd.c | 174 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 162 insertions(+), 12 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index c734f62..bfa52c3 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -386,9 +386,9 @@ static void nbd_update_server_watch(void)
>  }
>  
>  
> -static SocketAddress *nbd_build_socket_address(const char *sockpath,
> -                                               const char *bindto,
> -                                               const char *port)
> +static SocketAddress *nbd_build_socket_fd(const char *sockpath,
> +                                          const char *bindto,
> +                                          const char *port)

I don't think this needs renaming - its still returning an address
rather than an FD

>  {
>      SocketAddress *saddr;
>  
> @@ -463,6 +463,131 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char 
> *id, Error **errp)
>      return creds;
>  }
>  
> +static void setup_address_and_port(const char **address, const char **port)
> +{
> +    if (*address == NULL) {
> +        *address = "0.0.0.0";
> +    }
> +
> +    if (*port == NULL) {
> +        *port = g_strdup_printf("%d", NBD_DEFAULT_PORT);;
> +    }
> +}
> +
> +#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */
> +
> +#ifndef _WIN32
> +/*
> + * Check if socket activation was requested via use of the
> + * LISTEN_FDS and LISTEN_PID environment variables.
> + *
> + * Returns 0 if no socket activation, or the number of FDs.
> + */
> +static unsigned int check_socket_activation(void)
> +{
> +    const char *s;
> +    unsigned int pid;
> +    unsigned int nr_fds;
> +    unsigned int i;
> +    int fd;
> +
> +    s = getenv("LISTEN_PID");
> +    if (s == NULL) {
> +        return 0;
> +    }
> +    if (sscanf(s, "%u", &pid) != 1) {

IIRC qemu_strtoul would be preferred for this.

> +        if (verbose) {
> +            fprintf(stderr, "malformed %s environment variable (ignored)\n",
> +                    "LISTEN_PID");
> +        }
> +        return 0;
> +    }
> +    if (pid != getpid()) {
> +        if (verbose) {
> +            fprintf(stderr, "%s was not for us (ignored)\n",
> +                    "LISTEN_PID");
> +        }
> +        return 0;
> +    }
> +
> +    s = getenv("LISTEN_FDS");
> +    if (s == NULL) {
> +        return 0;
> +    }
> +    if (sscanf(s, "%u", &nr_fds) != 1) {

And this.

> +        if (verbose) {
> +            fprintf(stderr, "malformed %s environment variable (ignored)\n",
> +                    "LISTEN_FDS");
> +        }
> +        return 0;
> +    }
> +
> +    /* A limitation of current qemu-nbd is that it can only listen on
> +     * a single socket.  When that limitation is lifted, we can change
> +     * this function to allow LISTEN_FDS > 1, and remove the assertion
> +     * in the main function below.
> +     */
> +    if (nr_fds > 1) {
> +        error_report("qemu-nbd does not support socket activation with %s > 
> 1",
> +                     "LISTEN_FDS");
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    /* So these are not passed to any child processes we might start. */
> +    unsetenv("LISTEN_FDS");
> +    unsetenv("LISTEN_PID");
> +
> +    /* 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) {
> +            /* 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);
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    return nr_fds;
> +}
> +
> +#else /* !_WIN32 */
> +static unsigned int check_socket_activation(void)
> +{
> +    return 0;
> +}
> +#endif

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|