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 <kw...@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 <pbonz...@redhat.com> > >>> + * Kevin Wolf <kw...@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
signature.asc
Description: PGP signature