diff mbox series

[RFC,07/22] block/export: Remove magic from block-export-add

Message ID 20200813162935.210070-8-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
nbd-server-add tries to be convenient and adds two questionable
features that we don't want to share in block-export-add, even for NBD
exports:

1. When requesting a writable export of a read-only device, the export
   is silently downgraded to read-only. This should be an error in the
   context of block-export-add.

2. When using a BlockBackend name, unplugging the device from the guest
   will automatically stop the NBD server, too. This may sometimes be
   what you want, but it could also be very surprising. Let's keep
   things explicit with block-export-add. If the user wants to stop the
   export, they should tell us so.

Move these things into the nbd-server-add QMP command handler so that
they apply only there.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/nbd.h   |  3 ++-
 block/export/export.c | 44 ++++++++++++++++++++++++++++++++++++++-----
 blockdev-nbd.c        | 10 ++++------
 nbd/server.c          | 19 ++++++++++++-------
 qemu-nbd.c            |  3 +--
 5 files changed, 58 insertions(+), 21 deletions(-)

Comments

Max Reitz Aug. 17, 2020, 11:41 a.m. UTC | #1
On 13.08.20 18:29, Kevin Wolf wrote:
> nbd-server-add tries to be convenient and adds two questionable
> features that we don't want to share in block-export-add, even for NBD
> exports:
> 
> 1. When requesting a writable export of a read-only device, the export
>    is silently downgraded to read-only. This should be an error in the
>    context of block-export-add.
> 
> 2. When using a BlockBackend name, unplugging the device from the guest
>    will automatically stop the NBD server, too. This may sometimes be
>    what you want, but it could also be very surprising. Let's keep
>    things explicit with block-export-add. If the user wants to stop the
>    export, they should tell us so.
> 
> Move these things into the nbd-server-add QMP command handler so that
> they apply only there.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/nbd.h   |  3 ++-
>  block/export/export.c | 44 ++++++++++++++++++++++++++++++++++++++-----
>  blockdev-nbd.c        | 10 ++++------
>  nbd/server.c          | 19 ++++++++++++-------
>  qemu-nbd.c            |  3 +--
>  5 files changed, 58 insertions(+), 21 deletions(-)

[...]

> diff --git a/block/export/export.c b/block/export/export.c
> index 3d0dacb3f2..2d5f92861c 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c

[...]

> @@ -34,24 +36,56 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
>      return NULL;
>  }
>  
> -void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> +static BlockExport *blk_exp_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;
> +        return NULL;
>      }
>  
> -    drv->create(export, errp);
> +    return drv->create(export, errp);
> +}
> +
> +void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> +{
> +    blk_exp_add(export, errp);
>  }

Interesting.  I would have added it this way from the start then (with a
note that we’ll need it later).

>  void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
>  {
> -    BlockExportOptions export = {
> +    BlockExport *export;
> +    BlockDriverState *bs;
> +    BlockBackend *on_eject_blk;
> +
> +    BlockExportOptions export_opts = {
>          .type = BLOCK_EXPORT_TYPE_NBD,
>          .u.nbd = *arg,

This copies *arg’s contents...

>      };
> -    qmp_block_export_add(&export, errp);
> +
> +    /*
> +     * nbd-server-add doesn't complain when a read-only device should be
> +     * exported as writable, but simply downgrades it. This is an error with
> +     * block-export-add.
> +     */
> +    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
> +    if (bs && bdrv_is_read_only(bs)) {
> +        arg->writable = false;

...and here you only modify the original *arg, but not
export_opts.u.nbd.  So I don’t think this will have any effect.

> +    }
> +
> +    export = blk_exp_add(&export_opts, errp);
> +    if (!export) {
> +        return;
> +    }
> +
> +    /*
> +     * nbd-server-add removes the export when the named BlockBackend used for
> +     * @device goes away.
> +     */
> +    on_eject_blk = blk_by_name(arg->device);
> +    if (on_eject_blk) {
> +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> +    }
>  }

The longer it gets, the more I think maybe it should be in some NBD file
like blockdev-nbd.c after all.

[...]

> diff --git a/nbd/server.c b/nbd/server.c
> index 92360d1f08..0b84fd30e2 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1506,11 +1506,22 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>      aio_context_release(aio_context);
>  }
>  
> +void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
> +{
> +    NBDExport *nbd_exp = container_of(exp, NBDExport, common);
> +    assert(exp->drv == &blk_exp_nbd);
> +

I think asserting that the nbd_exp->eject_notifier is unused so far
would make sense (e.g. just checking that eject_notifier_blk is NULL).

Max

> +    blk_ref(blk);
> +    nbd_exp->eject_notifier_blk = blk;
> +    nbd_exp->eject_notifier.notify = nbd_eject_notifier;
> +    blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
> +}
> +
Kevin Wolf Aug. 17, 2020, 12:49 p.m. UTC | #2
Am 17.08.2020 um 13:41 hat Max Reitz geschrieben:
> On 13.08.20 18:29, Kevin Wolf wrote:
> > nbd-server-add tries to be convenient and adds two questionable
> > features that we don't want to share in block-export-add, even for NBD
> > exports:
> > 
> > 1. When requesting a writable export of a read-only device, the export
> >    is silently downgraded to read-only. This should be an error in the
> >    context of block-export-add.
> > 
> > 2. When using a BlockBackend name, unplugging the device from the guest
> >    will automatically stop the NBD server, too. This may sometimes be
> >    what you want, but it could also be very surprising. Let's keep
> >    things explicit with block-export-add. If the user wants to stop the
> >    export, they should tell us so.
> > 
> > Move these things into the nbd-server-add QMP command handler so that
> > they apply only there.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/nbd.h   |  3 ++-
> >  block/export/export.c | 44 ++++++++++++++++++++++++++++++++++++++-----
> >  blockdev-nbd.c        | 10 ++++------
> >  nbd/server.c          | 19 ++++++++++++-------
> >  qemu-nbd.c            |  3 +--
> >  5 files changed, 58 insertions(+), 21 deletions(-)
> 
> [...]
> 
> > diff --git a/block/export/export.c b/block/export/export.c
> > index 3d0dacb3f2..2d5f92861c 100644
> > --- a/block/export/export.c
> > +++ b/block/export/export.c
> 
> [...]
> 
> > @@ -34,24 +36,56 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
> >      return NULL;
> >  }
> >  
> > -void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> > +static BlockExport *blk_exp_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;
> > +        return NULL;
> >      }
> >  
> > -    drv->create(export, errp);
> > +    return drv->create(export, errp);
> > +}
> > +
> > +void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> > +{
> > +    blk_exp_add(export, errp);
> >  }
> 
> Interesting.  I would have added it this way from the start then (with a
> note that we’ll need it later).
> 
> >  void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
> >  {
> > -    BlockExportOptions export = {
> > +    BlockExport *export;
> > +    BlockDriverState *bs;
> > +    BlockBackend *on_eject_blk;
> > +
> > +    BlockExportOptions export_opts = {
> >          .type = BLOCK_EXPORT_TYPE_NBD,
> >          .u.nbd = *arg,
> 
> This copies *arg’s contents...
> 
> >      };
> > -    qmp_block_export_add(&export, errp);
> > +
> > +    /*
> > +     * nbd-server-add doesn't complain when a read-only device should be
> > +     * exported as writable, but simply downgrades it. This is an error with
> > +     * block-export-add.
> > +     */
> > +    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
> > +    if (bs && bdrv_is_read_only(bs)) {
> > +        arg->writable = false;
> 
> ...and here you only modify the original *arg, but not
> export_opts.u.nbd.  So I don’t think this will have any effect.

I thought I had tested this... Well, good catch, thanks.

> > +    }
> > +
> > +    export = blk_exp_add(&export_opts, errp);
> > +    if (!export) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * nbd-server-add removes the export when the named BlockBackend used for
> > +     * @device goes away.
> > +     */
> > +    on_eject_blk = blk_by_name(arg->device);
> > +    if (on_eject_blk) {
> > +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> > +    }
> >  }
> 
> The longer it gets, the more I think maybe it should be in some NBD file
> like blockdev-nbd.c after all.

Fair enough. Though I think blockdev-nbd.c in the root directory is
something that shouldn't even exist.

But I guess I can just leave the functions where they are and we can
move the file another day.

> [...]
> 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 92360d1f08..0b84fd30e2 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1506,11 +1506,22 @@ static void nbd_eject_notifier(Notifier *n, void *data)
> >      aio_context_release(aio_context);
> >  }
> >  
> > +void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
> > +{
> > +    NBDExport *nbd_exp = container_of(exp, NBDExport, common);
> > +    assert(exp->drv == &blk_exp_nbd);
> > +
> 
> I think asserting that the nbd_exp->eject_notifier is unused so far
> would make sense (e.g. just checking that eject_notifier_blk is NULL).

Makes sense.

Kevin
Max Reitz Aug. 17, 2020, 1:22 p.m. UTC | #3
On 17.08.20 14:49, Kevin Wolf wrote:
> Am 17.08.2020 um 13:41 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> nbd-server-add tries to be convenient and adds two questionable
>>> features that we don't want to share in block-export-add, even for NBD
>>> exports:
>>>
>>> 1. When requesting a writable export of a read-only device, the export
>>>    is silently downgraded to read-only. This should be an error in the
>>>    context of block-export-add.
>>>
>>> 2. When using a BlockBackend name, unplugging the device from the guest
>>>    will automatically stop the NBD server, too. This may sometimes be
>>>    what you want, but it could also be very surprising. Let's keep
>>>    things explicit with block-export-add. If the user wants to stop the
>>>    export, they should tell us so.
>>>
>>> Move these things into the nbd-server-add QMP command handler so that
>>> they apply only there.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  include/block/nbd.h   |  3 ++-
>>>  block/export/export.c | 44 ++++++++++++++++++++++++++++++++++++++-----
>>>  blockdev-nbd.c        | 10 ++++------
>>>  nbd/server.c          | 19 ++++++++++++-------
>>>  qemu-nbd.c            |  3 +--
>>>  5 files changed, 58 insertions(+), 21 deletions(-)

[...]

>>> +    }
>>> +
>>> +    export = blk_exp_add(&export_opts, errp);
>>> +    if (!export) {
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * nbd-server-add removes the export when the named BlockBackend used for
>>> +     * @device goes away.
>>> +     */
>>> +    on_eject_blk = blk_by_name(arg->device);
>>> +    if (on_eject_blk) {
>>> +        nbd_export_set_on_eject_blk(export, on_eject_blk);
>>> +    }
>>>  }
>>
>> The longer it gets, the more I think maybe it should be in some NBD file
>> like blockdev-nbd.c after all.
> 
> Fair enough. Though I think blockdev-nbd.c in the root directory is
> something that shouldn't even exist.

Absolutely.  But unless you (or someone™ else) doesn’t do anything about
it, we may as well continue to (ab)use it. O:)

Max
Eric Blake Aug. 19, 2020, 7:50 p.m. UTC | #4
cc: Peter Krempa

On 8/13/20 11:29 AM, Kevin Wolf wrote:
> nbd-server-add tries to be convenient and adds two questionable
> features that we don't want to share in block-export-add, even for NBD
> exports:
> 
> 1. When requesting a writable export of a read-only device, the export
>     is silently downgraded to read-only. This should be an error in the
>     context of block-export-add.

I'd be happy for this to be an error even with nbd-export-add; I don't 
think it would harm any of libvirt's existing usage (either for storage 
migration, or for incremental backups).

Side note: In the past, I had a proposal to enhance the NBD Protocol to 
allow a client to advertise to the server its intent on being a 
read-only or read-write client.  Not relevant to this patch, but this 
part of the commit message reminds me that I should revisit that topic 
(Rich and I recently hit another case in nbdkit where such an extension 
would be nice, when it comes to using NBD's multi-conn for better 
performance on a read-only connection, but only if the server knows the 
client intends to be read-only)

> 
> 2. When using a BlockBackend name, unplugging the device from the guest
>     will automatically stop the NBD server, too. This may sometimes be
>     what you want, but it could also be very surprising. Let's keep
>     things explicit with block-export-add. If the user wants to stop the
>     export, they should tell us so.

Here, keeping the nbd command different from the block-export command 
seems tolerable.  On the other hand, I wonder if Peter needs to change 
anything in libvirt's incremental backup code to handle this sudden 
disappearance of an NBD device during a disk hot-unplug (that is, either 
the presence of an ongoing pull-mode backup should block disk unplug, or 
libvirt needs a way to guarantee that an ongoing backup NBD device 
remains in spite of subsequent disk actions on the guest).  Depending on 
libvirt's needs, we may want to revisit the nbd command to have the same 
policy as block-export-add, plus an introspectible feature notation.

> 
> Move these things into the nbd-server-add QMP command handler so that
> they apply only there.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/block/nbd.h   |  3 ++-

> +void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> +{
> +    blk_exp_add(export, errp);
>   }
>   
>   void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
>   {
> -    BlockExportOptions export = {
> +    BlockExport *export;
> +    BlockDriverState *bs;
> +    BlockBackend *on_eject_blk;
> +
> +    BlockExportOptions export_opts = {
>           .type = BLOCK_EXPORT_TYPE_NBD,
>           .u.nbd = *arg,
>       };
> -    qmp_block_export_add(&export, errp);
> +
> +    /*
> +     * nbd-server-add doesn't complain when a read-only device should be
> +     * exported as writable, but simply downgrades it. This is an error with
> +     * block-export-add.

I'd be happy with either marking this deprecated now (and fixing it in 
two releases), or declaring it a bug in nbd-server-add now (and fixing 
it outright).

> +     */
> +    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
> +    if (bs && bdrv_is_read_only(bs)) {
> +        arg->writable = false;
> +    }
> +
> +    export = blk_exp_add(&export_opts, errp);
> +    if (!export) {
> +        return;
> +    }
> +
> +    /*
> +     * nbd-server-add removes the export when the named BlockBackend used for
> +     * @device goes away.
> +     */
> +    on_eject_blk = blk_by_name(arg->device);
> +    if (on_eject_blk) {
> +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> +    }

Wait - is the magic export removal tied only to exporting a drive name, 
and not a node name?  So as long as libvirt is using only node names 
whwen adding exports, a drive being unplugged won't interfere?

Overall, the change makes sense to me, although I'd love to see if we 
could go further on the writable vs. read-only issue.
Kevin Wolf Aug. 20, 2020, 11:05 a.m. UTC | #5
Am 19.08.2020 um 21:50 hat Eric Blake geschrieben:
> cc: Peter Krempa
> 
> On 8/13/20 11:29 AM, Kevin Wolf wrote:
> > nbd-server-add tries to be convenient and adds two questionable
> > features that we don't want to share in block-export-add, even for NBD
> > exports:
> > 
> > 1. When requesting a writable export of a read-only device, the export
> >     is silently downgraded to read-only. This should be an error in the
> >     context of block-export-add.
> 
> I'd be happy for this to be an error even with nbd-export-add; I don't think
> it would harm any of libvirt's existing usage (either for storage migration,
> or for incremental backups).
> 
> Side note: In the past, I had a proposal to enhance the NBD Protocol to
> allow a client to advertise to the server its intent on being a read-only or
> read-write client.  Not relevant to this patch, but this part of the commit
> message reminds me that I should revisit that topic (Rich and I recently hit
> another case in nbdkit where such an extension would be nice, when it comes
> to using NBD's multi-conn for better performance on a read-only connection,
> but only if the server knows the client intends to be read-only)
> 
> > 
> > 2. When using a BlockBackend name, unplugging the device from the guest
> >     will automatically stop the NBD server, too. This may sometimes be
> >     what you want, but it could also be very surprising. Let's keep
> >     things explicit with block-export-add. If the user wants to stop the
> >     export, they should tell us so.
> 
> Here, keeping the nbd command different from the block-export command seems
> tolerable.  On the other hand, I wonder if Peter needs to change anything in
> libvirt's incremental backup code to handle this sudden disappearance of an
> NBD device during a disk hot-unplug (that is, either the presence of an
> ongoing pull-mode backup should block disk unplug, or libvirt needs a way to
> guarantee that an ongoing backup NBD device remains in spite of subsequent
> disk actions on the guest).  Depending on libvirt's needs, we may want to
> revisit the nbd command to have the same policy as block-export-add, plus an
> introspectible feature notation.

As long as we can keep the compatibility code local to qmp_nbd_*(), I
don't think it's too bad. In particular because it's already written.

Instead of adjusting libvirt to changes in the nbd-* commands, I'd
rather have it change over to block-export-*. I would like to see the
nbd-server-add/remove commands deprecated soon after we have the
replacements.

> > 
> > Move these things into the nbd-server-add QMP command handler so that
> > they apply only there.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   include/block/nbd.h   |  3 ++-
> 
> > +void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> > +{
> > +    blk_exp_add(export, errp);
> >   }
> >   void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
> >   {
> > -    BlockExportOptions export = {
> > +    BlockExport *export;
> > +    BlockDriverState *bs;
> > +    BlockBackend *on_eject_blk;
> > +
> > +    BlockExportOptions export_opts = {
> >           .type = BLOCK_EXPORT_TYPE_NBD,
> >           .u.nbd = *arg,
> >       };
> > -    qmp_block_export_add(&export, errp);
> > +
> > +    /*
> > +     * nbd-server-add doesn't complain when a read-only device should be
> > +     * exported as writable, but simply downgrades it. This is an error with
> > +     * block-export-add.
> 
> I'd be happy with either marking this deprecated now (and fixing it in two
> releases), or declaring it a bug in nbd-server-add now (and fixing it
> outright).

How about deprecating nbd-server-add completely?

> > +     */
> > +    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
> > +    if (bs && bdrv_is_read_only(bs)) {
> > +        arg->writable = false;
> > +    }
> > +
> > +    export = blk_exp_add(&export_opts, errp);
> > +    if (!export) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * nbd-server-add removes the export when the named BlockBackend used for
> > +     * @device goes away.
> > +     */
> > +    on_eject_blk = blk_by_name(arg->device);
> > +    if (on_eject_blk) {
> > +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> > +    }
> 
> Wait - is the magic export removal tied only to exporting a drive name, and
> not a node name?  So as long as libvirt is using only node names whwen
> adding exports, a drive being unplugged won't interfere?

Yes, seems so. It's the existing behaviour, I'm only moving the code
around.

> Overall, the change makes sense to me, although I'd love to see if we could
> go further on the writable vs. read-only issue.

If nbd-server-add will be going away relatively soon, it's probably not
worth the trouble. But if you have reasons to keep it, maybe we should
consider it.

Kevin
Eric Blake Aug. 20, 2020, 2:41 p.m. UTC | #6
On 8/20/20 6:05 AM, Kevin Wolf wrote:

> As long as we can keep the compatibility code local to qmp_nbd_*(), I
> don't think it's too bad. In particular because it's already written.
> 
> Instead of adjusting libvirt to changes in the nbd-* commands, I'd
> rather have it change over to block-export-*. I would like to see the
> nbd-server-add/remove commands deprecated soon after we have the
> replacements.

Makes sense to me. So deprecate nbd-server-add in 5.2, and require 
block-export in 6.1.


>>> +    /*
>>> +     * nbd-server-add doesn't complain when a read-only device should be
>>> +     * exported as writable, but simply downgrades it. This is an error with
>>> +     * block-export-add.
>>
>> I'd be happy with either marking this deprecated now (and fixing it in two
>> releases), or declaring it a bug in nbd-server-add now (and fixing it
>> outright).
> 
> How about deprecating nbd-server-add completely?

Works for me. Keeping the warts backwards-compatible in nbd-server-add 
is more palatable if we know we are going to drop it wholesale down the 
road.

>>> +    /*
>>> +     * nbd-server-add removes the export when the named BlockBackend used for
>>> +     * @device goes away.
>>> +     */
>>> +    on_eject_blk = blk_by_name(arg->device);
>>> +    if (on_eject_blk) {
>>> +        nbd_export_set_on_eject_blk(export, on_eject_blk);
>>> +    }
>>
>> Wait - is the magic export removal tied only to exporting a drive name, and
>> not a node name?  So as long as libvirt is using only node names whwen
>> adding exports, a drive being unplugged won't interfere?
> 
> Yes, seems so. It's the existing behaviour, I'm only moving the code
> around.
> 
>> Overall, the change makes sense to me, although I'd love to see if we could
>> go further on the writable vs. read-only issue.
> 
> If nbd-server-add will be going away relatively soon, it's probably not
> worth the trouble. But if you have reasons to keep it, maybe we should
> consider it.

No, I'm fine with the idea of getting rid of nbd-server-add, at which 
point changing it before removal is pointless.
Peter Krempa Aug. 20, 2020, 3:28 p.m. UTC | #7
On Thu, Aug 20, 2020 at 09:41:14 -0500, Eric Blake wrote:
> On 8/20/20 6:05 AM, Kevin Wolf wrote:
> 
> > As long as we can keep the compatibility code local to qmp_nbd_*(), I
> > don't think it's too bad. In particular because it's already written.
> > 
> > Instead of adjusting libvirt to changes in the nbd-* commands, I'd
> > rather have it change over to block-export-*. I would like to see the
> > nbd-server-add/remove commands deprecated soon after we have the
> > replacements.
> 
> Makes sense to me. So deprecate nbd-server-add in 5.2, and require
> block-export in 6.1.
> 
> 
> > > > +    /*
> > > > +     * nbd-server-add doesn't complain when a read-only device should be
> > > > +     * exported as writable, but simply downgrades it. This is an error with
> > > > +     * block-export-add.
> > > 
> > > I'd be happy with either marking this deprecated now (and fixing it in two
> > > releases), or declaring it a bug in nbd-server-add now (and fixing it
> > > outright).
> > 
> > How about deprecating nbd-server-add completely?
> 
> Works for me. Keeping the warts backwards-compatible in nbd-server-add is
> more palatable if we know we are going to drop it wholesale down the road.
> 
> > > > +    /*
> > > > +     * nbd-server-add removes the export when the named BlockBackend used for
> > > > +     * @device goes away.
> > > > +     */
> > > > +    on_eject_blk = blk_by_name(arg->device);
> > > > +    if (on_eject_blk) {
> > > > +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> > > > +    }
> > > 
> > > Wait - is the magic export removal tied only to exporting a drive name, and
> > > not a node name?  So as long as libvirt is using only node names whwen
> > > adding exports, a drive being unplugged won't interfere?
> > 
> > Yes, seems so. It's the existing behaviour, I'm only moving the code
> > around.
> > 
> > > Overall, the change makes sense to me, although I'd love to see if we could
> > > go further on the writable vs. read-only issue.
> > 
> > If nbd-server-add will be going away relatively soon, it's probably not
> > worth the trouble. But if you have reasons to keep it, maybe we should
> > consider it.
> 
> No, I'm fine with the idea of getting rid of nbd-server-add, at which point
> changing it before removal is pointless.

I agree that this is a better approach. While it's technically possible
to retrofit old commands since QMP schema introspection allows granilar
detection of what's happening in this regard using a new command is IMO
better. Specifically for APPS which might not use QMP introspection to
the extent libvirt does.
diff mbox series

Patch

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 3846d2bac8..ffca3be78f 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -333,7 +333,8 @@  NBDExport *nbd_export_new(BlockDriverState *bs,
                           const char *name, const char *desc,
                           const char *bitmap, bool readonly, bool shared,
                           void (*close)(NBDExport *), bool writethrough,
-                          BlockBackend *on_eject_blk, Error **errp);
+                          Error **errp);
+void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
 void nbd_export_close(NBDExport *exp);
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp);
 void nbd_export_get(NBDExport *exp);
diff --git a/block/export/export.c b/block/export/export.c
index 3d0dacb3f2..2d5f92861c 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -13,6 +13,8 @@ 
 
 #include "qemu/osdep.h"
 
+#include "block/block.h"
+#include "sysemu/block-backend.h"
 #include "block/export.h"
 #include "block/nbd.h"
 #include "qapi/error.h"
@@ -34,24 +36,56 @@  static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
     return NULL;
 }
 
-void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+static BlockExport *blk_exp_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;
+        return NULL;
     }
 
-    drv->create(export, errp);
+    return drv->create(export, errp);
+}
+
+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+    blk_exp_add(export, errp);
 }
 
 void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
 {
-    BlockExportOptions export = {
+    BlockExport *export;
+    BlockDriverState *bs;
+    BlockBackend *on_eject_blk;
+
+    BlockExportOptions export_opts = {
         .type = BLOCK_EXPORT_TYPE_NBD,
         .u.nbd = *arg,
     };
-    qmp_block_export_add(&export, errp);
+
+    /*
+     * nbd-server-add doesn't complain when a read-only device should be
+     * exported as writable, but simply downgrades it. This is an error with
+     * block-export-add.
+     */
+    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
+    if (bs && bdrv_is_read_only(bs)) {
+        arg->writable = false;
+    }
+
+    export = blk_exp_add(&export_opts, errp);
+    if (!export) {
+        return;
+    }
+
+    /*
+     * nbd-server-add removes the export when the named BlockBackend used for
+     * @device goes away.
+     */
+    on_eject_blk = blk_by_name(arg->device);
+    if (on_eject_blk) {
+        nbd_export_set_on_eject_blk(export, on_eject_blk);
+    }
 }
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 16cda3b052..019c37c0bc 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -152,7 +152,6 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
 {
     BlockExportOptionsNbd *arg = &exp_args->u.nbd;
     BlockDriverState *bs = NULL;
-    BlockBackend *on_eject_blk;
     NBDExport *exp = NULL;
     AioContext *aio_context;
 
@@ -182,8 +181,6 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
         return NULL;
     }
 
-    on_eject_blk = blk_by_name(arg->device);
-
     bs = bdrv_lookup_bs(arg->device, arg->device, errp);
     if (!bs) {
         return NULL;
@@ -195,13 +192,14 @@  BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp)
     if (!arg->has_writable) {
         arg->writable = false;
     }
-    if (bdrv_is_read_only(bs)) {
-        arg->writable = false;
+    if (bdrv_is_read_only(bs) && arg->writable) {
+        error_setg(errp, "Cannot export read-only node as writable");
+        goto out;
     }
 
     exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,
                          !arg->writable, !arg->writable,
-                         NULL, false, on_eject_blk, errp);
+                         NULL, false, errp);
     if (!exp) {
         goto out;
     }
diff --git a/nbd/server.c b/nbd/server.c
index 92360d1f08..0b84fd30e2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1506,11 +1506,22 @@  static void nbd_eject_notifier(Notifier *n, void *data)
     aio_context_release(aio_context);
 }
 
+void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)
+{
+    NBDExport *nbd_exp = container_of(exp, NBDExport, common);
+    assert(exp->drv == &blk_exp_nbd);
+
+    blk_ref(blk);
+    nbd_exp->eject_notifier_blk = blk;
+    nbd_exp->eject_notifier.notify = nbd_eject_notifier;
+    blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);
+}
+
 NBDExport *nbd_export_new(BlockDriverState *bs,
                           const char *name, const char *desc,
                           const char *bitmap, bool readonly, bool shared,
                           void (*close)(NBDExport *), bool writethrough,
-                          BlockBackend *on_eject_blk, Error **errp)
+                          Error **errp)
 {
     AioContext *ctx;
     BlockBackend *blk;
@@ -1618,12 +1629,6 @@  NBDExport *nbd_export_new(BlockDriverState *bs,
     exp->ctx = ctx;
     blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
 
-    if (on_eject_blk) {
-        blk_ref(on_eject_blk);
-        exp->eject_notifier_blk = on_eject_blk;
-        exp->eject_notifier.notify = nbd_eject_notifier;
-        blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
-    }
     QTAILQ_INSERT_TAIL(&exports, exp, next);
     nbd_export_get(exp);
     return exp;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 818c3f5d46..e348d5d6d8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1058,8 +1058,7 @@  int main(int argc, char **argv)
 
     export = nbd_export_new(bs, export_name,
                             export_description, bitmap, readonly, shared > 1,
-                            nbd_export_closed, writethrough, NULL,
-                            &error_fatal);
+                            nbd_export_closed, writethrough, &error_fatal);
 
     if (device) {
 #if HAVE_NBD_DEVICE