Re: [RFC PATCH 04/22] block/export: Add BlockExport infrastructure and block-export-add

From: Eric Blake <eblake@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 04/22] block/export: Add BlockExport infrastructure and block-export-add
Date: Wed, 19 Aug 2020 13:31:45 -0500	[thread overview]
Message-ID: <3573ab66-884e-d237-cd5e-12bd67b05d8c@redhat.com> (raw)
In-Reply-To: <20200813162935.210070-5-kwolf@redhat.com>

On 8/13/20 11:29 AM, Kevin Wolf wrote:
> We want to have a common set of commands for all types of block exports.
> Currently, this is only NBD, but we're going to add more types.
> 
> This patch adds the basic BlockExport and BlockExportDriver structs and
> a QMP command block-export-add that creates a new export based on the
> given BlockExportOptions.
> 
> qmp_nbd_server_add() becomes a wrapper around qmp_block_export_add().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

Seeing if I can spot anything beyond Max's fine points:

> +++ b/qapi/block-export.json
> @@ -170,3 +170,12 @@
>         'nbd': 'BlockExportOptionsNbd'
>      } }
>   
> +##
> +# @block-export-add:
> +#
> +# Creates a new block export.
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'block-export-add',
> +  'data': 'BlockExportOptions', 'boxed': true }

So if I read patch 3 correctly, the difference between nbd-server-add 
and block-export-add is that the latter includes a "type":"nbd" member. 
(If we ever play with allowing a default discriminator value in QAPI, we 
could declare the default for "type" to be NBD, and then the two would 
be identical - except that since you are adding a new command designed 
to extend to more than just nbd, I don't think keeping nbd as default 
makes sense)

> +++ b/block/export/export.c

> +void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
> +{
> +    BlockExportOptions export = {
> +        .type = BLOCK_EXPORT_TYPE_NBD,
> +        .u.nbd = *arg,
> +    };
> +    qmp_block_export_add(&export, errp);
> +}

And indeed, this matches that analysis.

> @@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
>   
>    out:
>       aio_context_release(aio_context);
> +    /* TODO Remove the cast: Move to server.c which can access fields of exp */
> +    return (BlockExport*) exp;

Should this use container_of()?  Ah, the TODO says you want to, but 
can't because exp is an opaque type in this file...

>   }
>   
>   void qmp_nbd_server_remove(const char *name,
> diff --git a/nbd/server.c b/nbd/server.c
> index bee2ef8bd1..774325dbe5 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -18,6 +18,8 @@
>    */
>   
>   #include "qemu/osdep.h"
> +
> +#include "block/export.h"
>   #include "qapi/error.h"
>   #include "qemu/queue.h"
>   #include "trace.h"
> @@ -80,6 +82,7 @@ struct NBDRequestData {
>   };
>   
>   struct NBDExport {
> +    BlockExport common;

...but at least the cast is accurate.

Overall, the idea looks sane.  I'm happy if you want to move 
blockdev-nbd.c out of the top level.

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