diff mbox series

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

Message ID 20200813162935.210070-5-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
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

Comments

Max Reitz Aug. 17, 2020, 10:03 a.m. UTC | #1
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
Kevin Wolf Aug. 17, 2020, 12:45 p.m. UTC | #2
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
Max Reitz Aug. 17, 2020, 1:19 p.m. UTC | #3
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>
Kevin Wolf Aug. 17, 2020, 1:29 p.m. UTC | #4
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
Max Reitz Aug. 17, 2020, 1:53 p.m. UTC | #5
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
Eric Blake Aug. 19, 2020, 6:31 p.m. UTC | #6
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 mbox series

Patch

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