| Message ID | 20200813162935.210070-5-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
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. 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? > +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. > + 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.) > +} [...] > +void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) > +{ > + BlockExportOptions export = { > + .type = BLOCK_EXPORT_TYPE_NBD, > + .u.nbd = *arg, > + }; > + qmp_block_export_add(&export, errp); > +} Hm. I think I’d’ve kept this in blockdev-nbd.c, actually, but, well. It’ll stay a one-off. > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 98ee1b6170..a1dc11bdd7 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c [...] > @@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) > > out: > aio_context_release(aio_context); > + /* TODO Remove the cast: Move to server.c which can access fields of exp */ > + return (BlockExport*) exp; *hust* (But if it’s moved soon anyway so we can use &exp->common, then whatever.) Max
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. > 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. > > +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. > > + 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. Kevin
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>
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
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
On 8/13/20 11:29 AM, 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> > --- Seeing if I can spot anything beyond Max's fine points: > +++ b/qapi/block-export.json > @@ -170,3 +170,12 @@ > 'nbd': 'BlockExportOptionsNbd' > } } > > +## > +# @block-export-add: > +# > +# Creates a new block export. > +# > +# Since: 5.2 > +## > +{ 'command': 'block-export-add', > + 'data': 'BlockExportOptions', 'boxed': true } So if I read patch 3 correctly, the difference between nbd-server-add and block-export-add is that the latter includes a "type":"nbd" member. (If we ever play with allowing a default discriminator value in QAPI, we could declare the default for "type" to be NBD, and then the two would be identical - except that since you are adding a new command designed to extend to more than just nbd, I don't think keeping nbd as default makes sense) > +++ b/block/export/export.c > +void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) > +{ > + BlockExportOptions export = { > + .type = BLOCK_EXPORT_TYPE_NBD, > + .u.nbd = *arg, > + }; > + qmp_block_export_add(&export, errp); > +} And indeed, this matches that analysis. > @@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) > > out: > aio_context_release(aio_context); > + /* TODO Remove the cast: Move to server.c which can access fields of exp */ > + return (BlockExport*) exp; Should this use container_of()? Ah, the TODO says you want to, but can't because exp is an opaque type in this file... > } > > void qmp_nbd_server_remove(const char *name, > diff --git a/nbd/server.c b/nbd/server.c > index bee2ef8bd1..774325dbe5 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -18,6 +18,8 @@ > */ > > #include "qemu/osdep.h" > + > +#include "block/export.h" > #include "qapi/error.h" > #include "qemu/queue.h" > #include "trace.h" > @@ -80,6 +82,7 @@ struct NBDRequestData { > }; > > struct NBDExport { > + BlockExport common; ...but at least the cast is accurate. Overall, the idea looks sane. I'm happy if you want to move blockdev-nbd.c out of the top level.
diff --git a/qapi/block-export.json b/qapi/block-export.json index 9332076a05..40369814b4 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -170,3 +170,12 @@ 'nbd': 'BlockExportOptionsNbd' } } +## +# @block-export-add: +# +# Creates a new block export. +# +# Since: 5.2 +## +{ 'command': 'block-export-add', + 'data': 'BlockExportOptions', 'boxed': true } diff --git a/include/block/export.h b/include/block/export.h new file mode 100644 index 0000000000..b1d7325403 --- /dev/null +++ b/include/block/export.h @@ -0,0 +1,32 @@ +/* + * Declarations for block exports + * + * 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. + */ + +#ifndef BLOCK_EXPORT_H +#define BLOCK_EXPORT_H + +#include "qapi/qapi-types-block-export.h" + +typedef struct BlockExport BlockExport; + +typedef struct BlockExportDriver { + BlockExportType type; + BlockExport *(*create)(BlockExportOptions *, Error **); +} BlockExportDriver; + +struct BlockExport { + const BlockExportDriver *drv; +}; + +extern const BlockExportDriver blk_exp_nbd; + +#endif diff --git a/include/block/nbd.h b/include/block/nbd.h index 262f6da2ce..c8c5cb6b61 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -20,7 +20,7 @@ #ifndef NBD_H #define NBD_H -#include "qapi/qapi-types-block-export.h" +#include "block/export.h" #include "io/channel-socket.h" #include "crypto/tlscreds.h" #include "qapi/error.h" @@ -328,6 +328,7 @@ int nbd_errno_to_system_errno(int err); typedef struct NBDExport NBDExport; typedef struct NBDClient NBDClient; +BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp); NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, uint64_t size, const char *name, const char *desc, const char *bitmap, bool readonly, bool shared, 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[] = { + &blk_exp_nbd, +}; + +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]; + } + } + return NULL; +} + +void qmp_block_export_add(BlockExportOptions *export, Error **errp) +{ + const BlockExportDriver *drv; + + drv = blk_exp_find_driver(export->type); + if (!drv) { + error_setg(errp, "No driver found for the requested export type"); + return; + } + + drv->create(export, errp); +} + +void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) +{ + BlockExportOptions export = { + .type = BLOCK_EXPORT_TYPE_NBD, + .u.nbd = *arg, + }; + qmp_block_export_add(&export, errp); +} diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 98ee1b6170..a1dc11bdd7 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -148,17 +148,20 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, qapi_free_SocketAddress(addr_flat); } -void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) +BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) { + BlockExportOptionsNbd *arg = &exp_args->u.nbd; BlockDriverState *bs = NULL; BlockBackend *on_eject_blk; - NBDExport *exp; + NBDExport *exp = NULL; int64_t len; AioContext *aio_context; + assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); + if (!nbd_server) { error_setg(errp, "NBD server not running"); - return; + return NULL; } if (!arg->has_name) { @@ -167,24 +170,24 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) if (strlen(arg->name) > NBD_MAX_STRING_SIZE) { error_setg(errp, "export name '%s' too long", arg->name); - return; + return NULL; } if (arg->description && strlen(arg->description) > NBD_MAX_STRING_SIZE) { error_setg(errp, "description '%s' too long", arg->description); - return; + return NULL; } if (nbd_export_find(arg->name)) { error_setg(errp, "NBD server already has export named '%s'", arg->name); - return; + return NULL; } on_eject_blk = blk_by_name(arg->device); bs = bdrv_lookup_bs(arg->device, arg->device, errp); if (!bs) { - return; + return NULL; } aio_context = bdrv_get_aio_context(bs); @@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) out: aio_context_release(aio_context); + /* TODO Remove the cast: Move to server.c which can access fields of exp */ + return (BlockExport*) exp; } void qmp_nbd_server_remove(const char *name, diff --git a/nbd/server.c b/nbd/server.c index bee2ef8bd1..774325dbe5 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -18,6 +18,8 @@ */ #include "qemu/osdep.h" + +#include "block/export.h" #include "qapi/error.h" #include "qemu/queue.h" #include "trace.h" @@ -80,6 +82,7 @@ struct NBDRequestData { }; struct NBDExport { + BlockExport common; int refcount; void (*close)(NBDExport *exp); @@ -1512,10 +1515,15 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, { AioContext *ctx; BlockBackend *blk; - NBDExport *exp = g_new0(NBDExport, 1); + NBDExport *exp; uint64_t perm; int ret; + exp = g_new0(NBDExport, 1); + exp->common = (BlockExport) { + .drv = &blk_exp_nbd, + }; + /* * NBD exports are used for non-shared storage migration. Make sure * that BDRV_O_INACTIVE is cleared and the image is ready for write @@ -1731,6 +1739,11 @@ void nbd_export_put(NBDExport *exp) } } +const BlockExportDriver blk_exp_nbd = { + .type = BLOCK_EXPORT_TYPE_NBD, + .create = nbd_export_create, +}; + void nbd_export_close_all(void) { NBDExport *exp, *next; diff --git a/Makefile.objs b/Makefile.objs index d22b3b45d7..9b864ca046 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -14,7 +14,7 @@ chardev-obj-y = chardev/ authz-obj-y = authz/ block-obj-y = block/ nbd/ scsi/ -block-obj-y += block.o blockjob.o job.o +block-obj-y += block.o blockjob.o job.o blockdev-nbd.o block-obj-y += qemu-io-cmds.o block-obj-$(CONFIG_REPLICATION) += replication.o @@ -31,7 +31,7 @@ endif # CONFIG_SOFTMMU or CONFIG_TOOLS # used for system emulation, too, but specified separately there) storage-daemon-obj-y = block/ monitor/ qapi/ qom/ storage-daemon/ -storage-daemon-obj-y += blockdev.o blockdev-nbd.o iothread.o job-qmp.o +storage-daemon-obj-y += blockdev.o iothread.o job-qmp.o storage-daemon-obj-$(CONFIG_WIN32) += os-win32.o storage-daemon-obj-$(CONFIG_POSIX) += os-posix.o @@ -41,7 +41,7 @@ storage-daemon-obj-$(CONFIG_POSIX) += os-posix.o # single QEMU executable should support all CPUs and machines. ifeq ($(CONFIG_SOFTMMU),y) -common-obj-y = blockdev.o blockdev-nbd.o block/ +common-obj-y = blockdev.o block/ common-obj-y += bootdevice.o iothread.o common-obj-y += dump/ common-obj-y += job-qmp.o diff --git a/block/Makefile.objs b/block/Makefile.objs index 19c6f371c9..55b45c2f7d 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -44,6 +44,8 @@ block-obj-y += crypto.o block-obj-y += aio_task.o block-obj-y += backup-top.o block-obj-y += filter-compress.o + +block-obj-y += export/ common-obj-y += monitor/ block-obj-y += monitor/ diff --git a/block/export/Makefile.objs b/block/export/Makefile.objs new file mode 100644 index 0000000000..0c170ee6f1 --- /dev/null +++ b/block/export/Makefile.objs @@ -0,0 +1 @@ +block-obj-y += export.o
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