[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: |
Max Reitz |
Subject: |
Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option |
Date: |
Wed, 6 Nov 2019 13:51:42 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
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 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
>
signature.asc
Description: OpenPGP digital signature
- Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option,
Max Reitz <=
Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option, Markus Armbruster, 2019/11/08