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

[Max Reitz]

On 17.08.20 14:45, Kevin Wolf wrote:
> 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.

Good point.

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

Hm, yes.

tl;dr: I was wrong about a local external reference being nicer.  But I
believe there is a difference between externally-facing header files
(e.g. block.h) and internal header files (e.g. block_int.h).  I don’t
know which of those block/export.h is supposed to be.

(And of course it doesn’t even matter at all, really.)


non-tl;dr:

We have a similar case for bdrv_{file,raw,qcow2}, but those are at least
in a *_int.h.  I can’t say I like that style.

OK, let me try to figure out what my problem with this is.

I think if a module (in this case the NBD export code) exports
something, it should be available in the respective header (i.e., some
NBD header), not in some other header.  A module’s header should present
what it exports to the rest of the code.  The export code probably
doesn’t want to export the NBD driver object, it wants to import it,
actually.  So if it should be in a header file, it should be in an NBD
header.

Now none of our block drivers has a header file for exporting symbols to
the rest of the block code, which is why their symbols have been put
into block_int.h.  I think that’s cutting corners, but can be defended
by saying that block_int.h is not for exporting anything, but just
collects stuff internal to the block layer, so it kind of fits there.

(Still, technically, I believe bdrv_{file,raw,qcow2} should be exported
by each respective block driver in a driver-specific header file.  If
that isn’t the case, it doesn’t really matter to me whether it’s put
into a dedicated header file to collect internal stuff (block_int.h) or
just imported locally (with an external declaration) where it’s used.
Probably the dedicated header file is cleaner after all, right.)

Maybe block/export.h is the same in that it’s just supposed to collect
symbols used internally by the export code, then it isn’t wrong to put
it there.  But if it’s a header file that may be used by non-export code
to use export functionality, then it would be wrong.

But whatever.

Now I have sorted out my feelings, and they don’t give any result at
all, but it was kind of therapeutic for me.

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

Yes, true.

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

Sounds all reasonable.  Thus, I’d leave it like you did it and care
about a registration model if/when we need it.

Reviewed-by: Max Reitz <mreitz@redhat.com>

signature.asc
Description: OpenPGP digital signature