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

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org
Subject: Re: [RFC PATCH 04/22] block/export: Add BlockExport infrastructure and block-export-add
Date: Mon, 17 Aug 2020 12:03:19 +0200	[thread overview]
Message-ID: <7ed669db-7a75-fb25-4ce6-52369ea01b4b@redhat.com> (raw)
In-Reply-To: <20200813162935.210070-5-kwolf@redhat.com>

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

On 13.08.20 18:29, 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>
> ---
>  qapi/block-export.json     |  9 ++++++
>  include/block/export.h     | 32 +++++++++++++++++++++
>  include/block/nbd.h        |  3 +-
>  block/export/export.c      | 57 ++++++++++++++++++++++++++++++++++++++
>  blockdev-nbd.c             | 19 ++++++++-----
>  nbd/server.c               | 15 +++++++++-
>  Makefile.objs              |  6 ++--
>  block/Makefile.objs        |  2 ++
>  block/export/Makefile.objs |  1 +
>  9 files changed, 132 insertions(+), 12 deletions(-)
>  create mode 100644 include/block/export.h
>  create mode 100644 block/export/export.c
>  create mode 100644 block/export/Makefile.objs

Nothing of too great importance below.  But it’s an RFC, so comments I
will give.

> diff --git a/block/export/export.c b/block/export/export.c
> new file mode 100644
> index 0000000000..3d0dacb3f2
> --- /dev/null
> +++ b/block/export/export.c
> @@ -0,0 +1,57 @@
> +/*
> + * Common block export infrastructure
> + *
> + * Copyright (c) 2012, 2020 Red Hat, Inc.
> + *
> + * Authors:
> + * Paolo Bonzini <pbonzini@redhat.com>
> + * Kevin Wolf <kwolf@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "block/export.h"
> +#include "block/nbd.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-block-export.h"
> +
> +static const BlockExportDriver* blk_exp_drivers[] = {
                                 ^^
Sternenplatzierung *hust*

> +    &blk_exp_nbd,
> +};

Not sure whether I like this better than the block driver way of
registering block drivers with a constructor.  It requires writing less
code, at the expense of making the variable global.  So I think there’s
no good reason to prefer the block driver approach.

Maybe my hesitance comes from the variable being declared (as extern) in
a header file (block/export.h).  I think I would prefer it if we put
that external reference only here in this file.  Would that work, or do
you have other plans that require blk_exp_nbd to be visible outside of
nbd/server.c and this file here?

> +static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(blk_exp_drivers); i++) {
> +        if (blk_exp_drivers[i]->type == type) {
> +            return blk_exp_drivers[i];
> +        }
> +    }

How bad would it be to define blk_exp_drivers as
blk_exp_drivers[BLOCK_EXPORT_TYPE__MAX] and use the BlockExportType as
the driver index so we don’t have to loop here?

Not that it matters performance-wise.  Just something I wondered.

> +    return NULL;

Why not e.g. g_assert_not_reached()?

(If the BlockExportType were used as the index, I’d assert that
type < ARRAY_SIZE(blk_exp_drivers) && blk_exp_drivers[type] != NULL.  I
don’t think there’s a reason for graceful handling.)

> +}

[...]

> +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);
> +}

Hm.  I think I’d’ve kept this in blockdev-nbd.c, actually, but, well.
It’ll stay a one-off.

> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 98ee1b6170..a1dc11bdd7 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c

[...]

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

*hust*

(But if it’s moved soon anyway so we can use &exp->common, then whatever.)

Max

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