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 15:29:38 +0200	[thread overview]
Message-ID: <20200817132938.GM11402@linux.fritz.box> (raw)
In-Reply-To: <0a8939b5-4441-e76e-44c5-b27e69eba3b8@redhat.com>

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

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_*().

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.

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

Kevin

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