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 <[email protected]> >>>>> --- >>>>> 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 <[email protected]> >>>>> + * Kevin Wolf <[email protected]> >>>>> + * >>>>> + * 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
