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

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

[-- Attachment #1: Type: text/plain, Size: 4636 bytes --]

Am 17.08.2020 um 12:03 hat Max Reitz geschrieben:
> 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.

I guess I can see one reason why we may want to switch to the
registration style eventually: If we we want to make export drivers
optional modules which may or may not be present.

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

Hm, do we have precedence for "public, but not really" variables?
Normally I expect public symbols to be declared in a header file.

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

Might be nicer indeed. It would be incompatible with a registration
model, though, so if we're not sure yet what we want to have in the long
term, maybe the more neutral way is to leave it as it is.

> > +    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.)

Same thing actually. This works as long as all drivers are always
present.

Now I understand that the current state is somewhat inconsistent in that
it uses a simple array of things that are always present, but has
functions that work as if it were dynamic. I don't mind this
inconsistency very much, but if you do, I guess I could implement a
registration type thing right away.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]