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

[Max Reitz]

On 17.08.20 15:29, Kevin Wolf wrote:
> Am 17.08.2020 um 15:19 hat Max Reitz geschrieben:
>> 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.
> 
> Actually, there could be a conclusion: The declaration shouldn't be in
> include/block/export.h, but in include/block/nbd.h. We already include
> both headers in block/export/export.c because of qmp_nbd_*().

Sounds good.

> Of course, you already requests that I leave the other NBD-related stuff
> in blockdev-nbd.c rather than moving it there, so the use of blk_exp_nbd
> would be the only reason that remains for export.c to include nbd.h.

Aww.  That’s too bad.

> But it might still be better than having it in export.h.

I’ll take the easy way out and leave it to you.

Max

signature.asc
Description: OpenPGP digital signature