[v2,1/1] qemu-nbd: add the option to use pre-created server socket
diff mbox

Message ID 1475578981-24126-1-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Oct. 4, 2016, 11:03 a.m. UTC
From: Denis Plotnikov <dplotnikov@virtuozzo.com>

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 <dplotnikov@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 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

Comments

Daniel P. Berrangé Oct. 4, 2016, 11:09 a.m. UTC | #1
On Tue, Oct 04, 2016 at 02:03:01PM +0300, Denis V. Lunev wrote:
> From: Denis Plotnikov <dplotnikov@virtuozzo.com>
> 
> 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 <dplotnikov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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

It seems you've ignored the suggestion to implement the systemd socket
activation protocol, which would avoid the need for any new command
line parameters, and would let qemu-nbd "just work" with systemd units
too.


Regards,
Daniel
Denis V. Lunev Oct. 4, 2016, 11:13 a.m. UTC | #2
On 10/04/2016 02:03 PM, Denis V. Lunev wrote:
> From: Denis Plotnikov <dplotnikov@virtuozzo.com>
>
> 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 <dplotnikov@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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, --socket=@var{path}
>  Use a unix socket with path @var{path}
> +@item --server-sock-fd=@var{fd_num}
> +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
Denis V. Lunev Oct. 4, 2016, 11:15 a.m. UTC | #3
On 10/04/2016 02:09 PM, Daniel P. Berrange wrote:
> On Tue, Oct 04, 2016 at 02:03:01PM +0300, Denis V. Lunev wrote:
>> From: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>
>> 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 <dplotnikov@virtuozzo.com>
>> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  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
> It seems you've ignored the suggestion to implement the systemd socket
> activation protocol, which would avoid the need for any new command
> line parameters, and would let qemu-nbd "just work" with systemd units
> too.
>
>
> Regards,
> Daniel
thank you very much. I have missed that letter.
Pls disregard this sending.

ya, we will start over ;)

Den

Patch
diff mbox

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, --socket=@var{path}
 Use a unix socket with path @var{path}
+@item --server-sock-fd=@var{fd_num}
+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