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

[Max Reitz]

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);
> +}
> +

signature.asc
Description: OpenPGP digital signature