Re: [Qemu-devel] [PATCH v9 07/11] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf <kwolf@redhat.com>]

Am 04.04.2016 um 15:43 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name as well as a device name.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and
> no longer returns DeviceNotFound, iotest 030 is updated to expect
> GenericError instead.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c             | 31 +++++++++++++++++++++++--------
>  qapi/block-core.json   | 10 +++++++---
>  tests/qemu-iotests/030 |  2 +-
>  3 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 2e7712e..bfdc0e3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2989,6 +2989,7 @@ void qmp_block_stream(const char *device,
>      BlockBackend *blk;
>      BlockDriverState *bs;
>      BlockDriverState *base_bs = NULL;
> +    BlockDriverState *active;
>      AioContext *aio_context;
>      Error *local_err = NULL;
>      const char *base_name = NULL;
> @@ -2997,21 +2998,19 @@ void qmp_block_stream(const char *device,
>          on_error = BLOCKDEV_ON_ERROR_REPORT;
>      }
>  
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", device);
> +    bs = bdrv_lookup_bs(device, device, errp);
> +    if (!bs) {
>          return;
>      }
>  
> -    aio_context = blk_get_aio_context(blk);
> +    aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> -    if (!blk_is_available(blk)) {
> +    blk = blk_by_name(device);
> +    if (blk && !blk_is_available(blk)) {
>          error_setg(errp, "Device '%s' has no medium", device);
>          goto out;
>      }
> -    bs = blk_bs(blk);
>  
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>          goto out;
> @@ -3027,6 +3026,22 @@ void qmp_block_stream(const char *device,
>          base_name = base;
>      }
>  
> +    /* Look for the top-level node that contains 'bs' in its chain */
> +    active = NULL;
> +    do {
> +        active = bdrv_next(active);
> +    } while (active && !bdrv_chain_contains(active, bs));
> +
> +    if (active == NULL) {
> +        error_setg(errp, "Cannot find top level node for '%s'", device);
> +        goto out;
> +    }

Hm... On the one hand, I really like that you don't expect the user to
provide the active layer in QMP. This allows us to remove this wart once
we have the new op blockers.

On the other hand, this code assumes that there is only a single
top-level node. This isn't necessarily true any more these days. Do we
need to set blockers on _all_ root nodes that have the node in their
backing chain?

Kevin