All of lore.kernel.org
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: pkrempa@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org
Subject: Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option
Date: Wed, 6 Nov 2019 13:51:42 +0100	[thread overview]
Message-ID: <25f74aa7-0856-a396-fb98-f6b7a337b965@redhat.com> (raw)
In-Reply-To: <20191017130204.16131-7-kwolf@redhat.com>


[-- Attachment #1.1: Type: text/plain, Size: 7174 bytes --]

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 <kwolf@redhat.com>
> ---
>  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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2019-11-06 12:53 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-17 13:01 [RFC PATCH 00/18] Add qemu-storage-daemon Kevin Wolf
2019-10-17 13:01 ` [RFC PATCH 01/18] qemu-storage-daemon: Add barebone tool Kevin Wolf
2019-10-24 13:50   ` Eric Blake
2019-11-13 14:12     ` Kevin Wolf
2019-11-06 12:11   ` Max Reitz
2019-11-07 16:21   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 02/18] qemu-storage-daemon: Add --object option Kevin Wolf
2019-11-07 20:36   ` Markus Armbruster
2019-11-14 12:05     ` Kevin Wolf
2019-11-18  9:10       ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 03/18] stubs: Add arch_type Kevin Wolf
2019-10-17 13:01 ` [RFC PATCH 04/18] stubs: Add blk_by_qdev_id() Kevin Wolf
2019-11-08  9:03   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 05/18] qemu-storage-daemon: Add --blockdev option Kevin Wolf
2019-11-08 13:29   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option Kevin Wolf
2019-11-06 12:51   ` Max Reitz [this message]
2019-11-06 19:25     ` Eric Blake
2019-11-07  8:33       ` Kevin Wolf
2019-11-07 13:45         ` Eric Blake
2019-11-07 15:27           ` Kevin Wolf
2019-11-07 15:36             ` Eric Blake
2019-11-08 15:36     ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 07/18] blockdev-nbd: Boxed argument type for nbd-server-add Kevin Wolf
2019-10-17 13:01 ` [RFC PATCH 08/18] qemu-storage-daemon: Add --export option Kevin Wolf
2019-11-06 13:11   ` Max Reitz
2019-11-06 13:34     ` Kevin Wolf
2019-11-06 13:39       ` Max Reitz
2019-11-08 15:57       ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 09/18] qemu-storage-daemon: Add main loop Kevin Wolf
2019-11-08 16:02   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 10/18] qemu-storage-daemon: Add --chardev option Kevin Wolf
2019-11-08 16:27   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 11/18] monitor: Move monitor option parsing to monitor/monitor.c Kevin Wolf
2019-10-17 13:01 ` [RFC PATCH 12/18] stubs: Update monitor stubs for qemu-storage-daemon Kevin Wolf
2019-11-08 16:45   ` Markus Armbruster
2019-10-17 13:01 ` [RFC PATCH 13/18] qapi: Create module 'monitor' Kevin Wolf
2019-11-11  9:36   ` Markus Armbruster
2019-10-17 13:02 ` [RFC PATCH 14/18] monitor: Create monitor/qmp-cmds-monitor.c Kevin Wolf
2019-10-17 13:02 ` [RFC PATCH 15/18] qapi: Support empty modules Kevin Wolf
2019-11-12  8:29   ` Markus Armbruster
2019-10-17 13:02 ` [RFC PATCH 16/18] qapi: Create 'pragma' module Kevin Wolf
2019-11-12  9:41   ` Markus Armbruster
2019-10-17 13:02 ` [RFC PATCH 17/18] monitor: Move qmp_query_qmp_schema to qmp-cmds-monitor.c Kevin Wolf
2019-10-17 13:02 ` [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option Kevin Wolf
2019-11-06 14:32   ` Max Reitz
2019-11-07 10:12     ` Kevin Wolf
2019-11-07 10:44       ` Max Reitz
2019-11-08  8:59   ` Markus Armbruster
2019-11-12 14:25   ` Markus Armbruster
2019-11-13 10:58     ` Kevin Wolf
2019-11-13 13:53       ` Markus Armbruster
2019-10-24 11:33 ` [RFC PATCH 00/18] Add qemu-storage-daemon Kevin Wolf
2019-10-24 13:55 ` Vladimir Sementsov-Ogievskiy
2019-11-14 10:44   ` Kevin Wolf
2019-11-05 15:52 ` Stefan Hajnoczi
2019-11-06 14:37 ` Max Reitz
2019-11-06 14:58   ` Kevin Wolf
2019-11-06 15:35     ` Max Reitz
2019-11-06 17:13     ` Eric Blake
2019-11-21 10:32     ` Stefan Hajnoczi
2019-11-21 11:08       ` Kevin Wolf
2019-12-12 11:16         ` Stefan Hajnoczi
2019-11-07 10:33 ` Daniel P. Berrangé
2019-11-07 12:03   ` Kevin Wolf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=25f74aa7-0856-a396-fb98-f6b7a337b965@redhat.com \
    --to=mreitz@redhat.com \
    --cc=armbru@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=pkrempa@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.