qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option


From: Eric Blake
Subject: Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option
Date: Wed, 6 Nov 2019 13:25:58 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1

On 11/6/19 6:51 AM, Max Reitz wrote:
On 17.10.19 15:01, Kevin Wolf wrote:
Add a --nbd-server option to qemu-storage-daemon to start the built-in
NBD server right away. It maps the arguments for nbd-server-start to the
command line.

Well, it doesn’t quite, because nbd-server-start takes a
SocketAddressLegacy, and this takes a SocketAddress.

On one hand I can understand why you would do it differently (especially
for command-line options), but on the other I find it a bit problematic
to have --nbd-server be slightly different from nbd-server-start when
both are intended to be the same.

My biggest problem though lies in the duplication in the QAPI schema.
If NbdServerOptions.addr were a SocketAddressLegacy, we could let
nbd-server-start’s options just be of type NbdServerOptions and thus get
rid of the duplication.

I would love to somehow deprecate the use of SocketAddressLegacy and get QMP nbd-server-start to accept SocketAddress instead. Maybe it could be done by adding a new nbd-server-begin command in 5.0 with a saner wire layout, and deprecating nbd-server-start at that time; by the 5.2 release, we could then drop nbd-server-start. But we're too late for 4.2.


I suspect in practice it’s all not that big of a problem.  I can’t call
it bad if --nbd-server is just nicer to use.  And the biggest problem
with duplication in the QAPI schema is that nbd-server-start and
--nbd-server might get out of sync.  But realistically, I don’t see that
happen, because if nbd-server-start changes, nbd_server_start() will
change, too, so we’ll get compile errors in nbd_server_start_options().

*shrug*

But I do think the commit message should explain why we can’t just use
NbdServerOptions for nbd-server-start.

Max

Example (only with required options):

     --nbd-server addr.type=inet,addr.host=localhost,addr.port=10809

Signed-off-by: Kevin Wolf <address@hidden>
---
  qapi/block.json       | 18 ++++++++++++++++++
  include/block/nbd.h   |  1 +
  blockdev-nbd.c        |  5 +++++
  qemu-storage-daemon.c | 26 +++++++++++++++++++++++++-
  Makefile.objs         |  2 +-
  5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 145c268bb6..7fe0cf6538 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -215,6 +215,24 @@
              '*id': 'str',
              '*force': 'bool' } }
+##
+# @NbdServerOptions:
+#
+# @addr: Address on which to listen.
+# @tls-creds: ID of the TLS credentials object (since 2.6).
+# @tls-authz: ID of the QAuthZ authorization object used to validate
+#             the client's x509 distinguished name. This object is
+#             is only resolved at time of use, so can be deleted and
+#             recreated on the fly while the NBD server is active.
+#             If missing, it will default to denying access (since 4.0).
+#
+# Since: 4.2
+##
+{ 'struct': 'NbdServerOptions',
+  'data': { 'addr': 'SocketAddress',
+            '*tls-creds': 'str',
+            '*tls-authz': 'str'} }
+
  ##
  # @nbd-server-start:
  #
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9..2a7441491a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -353,6 +353,7 @@ void nbd_client_put(NBDClient *client);
void nbd_server_start(SocketAddress *addr, const char *tls_creds,
                        const char *tls_authz, Error **errp);
+void nbd_server_start_options(NbdServerOptions *arg, Error **errp);
/* nbd_read
   * Reads @size bytes from @ioc. Returns 0 on success.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 6a8b206e1d..d4c1fd4166 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -132,6 +132,11 @@ void nbd_server_start(SocketAddress *addr, const char 
*tls_creds,
      nbd_server = NULL;
  }
+void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
+{
+    nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz, errp);
+}
+
  void qmp_nbd_server_start(SocketAddressLegacy *addr,
                            bool has_tls_creds, const char *tls_creds,
                            bool has_tls_authz, const char *tls_authz,
diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
index 904e3c3a46..51882452f3 100644
--- a/qemu-storage-daemon.c
+++ b/qemu-storage-daemon.c
@@ -25,11 +25,14 @@
  #include "qemu/osdep.h"
#include "block/block.h"
+#include "block/nbd.h"
  #include "crypto/init.h"
#include "qapi/error.h"
-#include "qapi/qapi-visit-block-core.h"
+#include "qapi/qapi-commands-block.h"
  #include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-visit-block.h"
+#include "qapi/qapi-visit-block-core.h"
  #include "qapi/qobject-input-visitor.h"
#include "qemu-common.h"
@@ -64,6 +67,12 @@ static void help(void)
  "             [,driver specific parameters...]\n"
  "                         configure a block backend\n"
  "\n"
+"  --nbd-server addr.type=inet,addr.host=<host>,addr.port=<port>\n"
+"               [,tls-creds=<id>][,tls-authz=<id>]\n"
+"  --nbd-server addr.type=unix,addr.path=<path>\n"
+"               [,tls-creds=<id>][,tls-authz=<id>]\n"
+"                         start an NBD server for exporting block nodes\n"
+"\n"
  "  --object <properties>  define a QOM object such as 'secret' for\n"
  "                         passwords and/or encryption keys\n"
  "\n"
@@ -74,6 +83,7 @@ QEMU_HELP_BOTTOM "\n",
  enum {
      OPTION_OBJECT = 256,
      OPTION_BLOCKDEV,
+    OPTION_NBD_SERVER,
  };
static QemuOptsList qemu_object_opts = {
@@ -95,6 +105,7 @@ static int process_options(int argc, char *argv[], Error 
**errp)
          {"help", no_argument, 0, 'h'},
          {"object", required_argument, 0, OPTION_OBJECT},
          {"blockdev", required_argument, 0, OPTION_BLOCKDEV},
+        {"nbd-server", required_argument, 0, OPTION_NBD_SERVER},
          {"version", no_argument, 0, 'V'},
          {"trace", required_argument, NULL, 'T'},
          {0, 0, 0, 0}
@@ -152,6 +163,19 @@ static int process_options(int argc, char *argv[], Error 
**errp)
                  qapi_free_BlockdevOptions(options);
                  break;
              }
+        case OPTION_NBD_SERVER:
+            {
+                Visitor *v;
+                NbdServerOptions *options;
+
+                v = qobject_input_visitor_new_str(optarg, NULL, &error_fatal);
+                visit_type_NbdServerOptions(v, NULL, &options, &error_fatal);
+                visit_free(v);
+
+                nbd_server_start_options(options, &error_fatal);
+                qapi_free_NbdServerOptions(options);
+                break;
+            }
          }
      }
      if (optind != argc) {
diff --git a/Makefile.objs b/Makefile.objs
index 00fdf54500..cc262e445f 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -42,7 +42,7 @@ io-obj-y = io/
  # used for system emulation, too, but specified separately there)
storage-daemon-obj-y = block/
-storage-daemon-obj-y += blockdev.o iothread.o
+storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o
######################################################################
  # Target independent part of system emulation. The long term path is to




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




reply via email to

[Prev in Thread] Current Thread [Next in Thread]