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

[Eric Blake <eblake@redhat.com>]

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 <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
>>
> 
> 

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