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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4845 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]