Re: [Qemu-devel] [PATCH v2 1/1] qemu-nbd: add the option to use pre-created server socket

[Denis V. Lunev]

On 10/04/2016 02:03 PM, Denis V. Lunev wrote:
> From: Denis Plotnikov <address@hidden>
>
> The NBD server socket was created by qemu-nbd code. This could lead to the
> race issue when the management layer started qemu-nbd server and allowed
> a client to connect to the server, the client tried to connect to the
> server but failed because qemu-nbd had not managed to open the listening
> server socket by the time the client has finished its trying to connect.
>
> Creating a listening socket before starting of qemu-ndb and passing the
> socket fd to be used as the server socket to qemu-nbd as an option solves
> this issue.
>
> It also helps to resolve the situation when qemu-nbd started by some
> management software should report the port number qemu-nbd bound to, to
> another piece of software. Right now, there is no good way to do that
> except to start the qemu-nbd to make sure that the certain port has been
> actually aquired. Otherwise, it is definitely racy to try to define the
> port first and then start the qemu-nbd, hoping that the port defined is
> still available. Passing of the pre-created file descriptor resolves this
> situation as well.
>
> As a plus, pre-creating of the server socket adds some degree of freedom in
> setting of the socket properties. It is so, because once qemu-nbd gets
> the socket fd, it starts using it as is, without any additional
> modifications.
>
> Signed-off-by: Denis Plotnikov <address@hidden>
> Reviewed-by: Roman Kagan <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> ---
>  qemu-nbd.c    | 95 
> +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  qemu-nbd.texi |  6 ++++
>  2 files changed, 89 insertions(+), 12 deletions(-)
>
> Changes from v1:
> - commit message improvements
> - Eric's style nits applied
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 99297a5..3b6319e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -48,9 +48,13 @@
>  #define QEMU_NBD_OPT_OBJECT        260
>  #define QEMU_NBD_OPT_TLSCREDS      261
>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
> +#define QEMU_NBD_OPT_SRV_SOCKET_FD 263
>  
>  #define MBR_SIZE 512
>  
> +#define STR(s) _STR(s)
> +#define _STR(s) #s
> +
>  static NBDExport *exp;
>  static bool newproto;
>  static int verbose;
> @@ -78,6 +82,8 @@ static void usage(const char *name)
>  "  -b, --bind=IFACE          interface to bind to (default `0.0.0.0')\n"
>  "  -k, --socket=PATH         path to the unix socket\n"
>  "                            (default '"SOCKET_PATH"')\n"
> +"      --server-sock-fd=NUM  pre-opened listening server socket\n"
> +"                            file descriptor\n"
>  "  -e, --shared=NUM          device can be shared by NUM clients (default 
> '1')\n"
>  "  -t, --persistent          don't exit on the last connection\n"
>  "  -v, --verbose             display extra debugging information\n"
> @@ -382,9 +388,8 @@ 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_sock_fd(const char *sockpath,
> +                                        const char *bindto, const char *port)
>  {
>      SocketAddress *saddr;
>  
> @@ -459,6 +464,41 @@ 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 = STR(NBD_DEFAULT_PORT);
> +    }
> +}
> +
> +/*
> ++ * Check socket parameters compatibility when sock_fd is given
> ++ */
> +static const char *sock_fd_validate_opts(char *device, char *sockpath,
> +                                         const char *address, const char 
> *port)
> +{
> +    if (device != NULL) {
> +        return "NBD device can't be set when socket fd is specified";
> +    }
> +
> +    if (sockpath != NULL) {
> +        return "Unix socket can't be set when socket fd is specified";
> +    }
> +
> +    if (address != NULL) {
> +        return "The interface can't be set when socket fd is specified";
> +    }
> +
> +    if (port != NULL) {
> +        return "TCP port number can't be set when socket fd is specified";
> +    }
> +
> +    return NULL;
> +}
>  
>  int main(int argc, char **argv)
>  {
> @@ -467,7 +507,7 @@ int main(int argc, char **argv)
>      off_t dev_offset = 0;
>      uint16_t nbdflags = 0;
>      bool disconnect = false;
> -    const char *bindto = "0.0.0.0";
> +    const char *bindto = NULL;
>      const char *port = NULL;
>      char *sockpath = NULL;
>      char *device = NULL;
> @@ -503,6 +543,8 @@ int main(int argc, char **argv)
>          { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
>          { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>          { "trace", required_argument, NULL, 'T' },
> +        { "server-sock-fd", required_argument, NULL,
> +          QEMU_NBD_OPT_SRV_SOCKET_FD },
>          { NULL, 0, NULL, 0 }
>      };
>      int ch;
> @@ -524,6 +566,7 @@ int main(int argc, char **argv)
>      bool imageOpts = false;
>      bool writethrough = true;
>      char *trace_file = NULL;
> +    int sock_fd = -1;
>  
>      /* The client thread uses SIGTERM to interrupt the server.  A signal
>       * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> @@ -714,6 +757,15 @@ int main(int argc, char **argv)
>              g_free(trace_file);
>              trace_file = trace_opt_parse(optarg);
>              break;
> +        case QEMU_NBD_OPT_SRV_SOCKET_FD:
> +            sock_fd = qemu_parse_fd(optarg);
> +
> +            if (sock_fd < 0) {
> +                error_report("Invalid socket fd value: "
> +                             "it must be a non-negative integer");
> +                exit(EXIT_FAILURE);
> +            }
> +            break;
>          }
>      }
>  
> @@ -735,6 +787,17 @@ int main(int argc, char **argv)
>      trace_init_file(trace_file);
>      qemu_set_log(LOG_TRACE);
>  
> +    if (sock_fd != -1) {
> +        const char *err_msg = sock_fd_validate_opts(device, sockpath,
> +                                                    bindto, port);
> +        if (err_msg != NULL) {
> +            error_report("%s", err_msg);
> +            exit(EXIT_FAILURE);
> +        }
> +    } else {
> +        setup_address_and_port(&bindto, &port);
> +    }
> +
>      if (tlscredsid) {
>          if (sockpath) {
>              error_report("TLS is only supported with IPv4/IPv6");
> @@ -838,7 +901,22 @@ int main(int argc, char **argv)
>          snprintf(sockpath, 128, SOCKET_PATH, basename(device));
>      }
>  
> -    saddr = nbd_build_socket_address(sockpath, bindto, port);
> +    if (sock_fd == -1) {
> +        server_ioc = qio_channel_socket_new();
> +        saddr = nbd_build_sock_fd(sockpath, bindto, port);
> +        if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 
> 0) {
> +            object_unref(OBJECT(server_ioc));
> +            error_report_err(local_err);
> +            return 1;
> +        }
> +    } else {
> +        server_ioc = qio_channel_socket_new_fd(sock_fd, &local_err);
> +        if (server_ioc == NULL) {
> +            error_report("Failed to use the given server socket fd: %s",
> +                         error_get_pretty(local_err));
> +            exit(EXIT_FAILURE);
> +        }
> +    }
>  
>      if (qemu_init_main_loop(&local_err)) {
>          error_report_err(local_err);
> @@ -921,13 +999,6 @@ int main(int argc, char **argv)
>          newproto = true;
>      }
>  
> -    server_ioc = qio_channel_socket_new();
> -    if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
> -        object_unref(OBJECT(server_ioc));
> -        error_report_err(local_err);
> -        return 1;
> -    }
> -
>      if (device) {
>          int ret;
>  
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 91ebf04..4c9ea00 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -34,6 +34,12 @@ The offset into the image
>  The interface to bind to (default @samp{0.0.0.0})
>  @item -k, address@hidden
>  Use a unix socket with path @var{path}
> address@hidden address@hidden
> +A server socket fd to be used for client connections.
> +The socket fd must be configured and switched to listening
> +state before passing to the program. The program uses
> +the given socket fd as is, without any additional
> +modifications.
>  @item --image-opts
>  Treat @var{filename} as a set of image options, instead of a plain
>  filename. If this flag is specified, the @var{-f} flag should
pls disregard