Re: [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf <kwolf@redhat.com>]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 11 +++++++++--
>  qapi/block-core.json | 10 +++++++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f13fc69..57c8329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>                        bool has_on_error, BlockdevOnError on_error,
>                        Error **errp)
>  {
> -    BlockDriverState *bs;
> +    BlockDriverState *bs, *iter;
>      BlockDriverState *base_bs = NULL;
>      AioContext *aio_context;
>      Error *local_err = NULL;
> @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
>          on_error = BLOCKDEV_ON_ERROR_REPORT;
>      }
>  
> -    bs = qmp_get_root_bs(device, errp);
> +    bs = bdrv_lookup_bs(device, device, errp);
>      if (!bs) {
>          return;
>      }
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>          goto out;
>      }

Added a bit more context.

This check is redundant now...

>      if (has_base) {
>          base_bs = bdrv_find_backing_image(bs, base);
>          if (base_bs == NULL) {
>              error_setg(errp, QERR_BASE_NOT_FOUND, base);
>              goto out;
>          }
>          assert(bdrv_get_aio_context(base_bs) == aio_context);
>          base_name = base;
>      }
>  
> +    /* Check for op blockers in the whole chain between bs and base */
> +    for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
> +        if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
> +            goto out;
> +        }
> +    }

...because you start with iter = bs here.

Another thought I had while looking at the previous few patches is
whether all the op blocker checks wouldn't be better moved to the actual
block job code (i.e. stream_start in this case). In this case it doesn't
matter much because this is the only caller of stream_start(), but for
other jobs the situation is more complicated and it would be easy for a
caller to forget the checks.

Probably also matter for another series, but I wanted to mention it.

> +
>      /* if we are streaming the entire chain, the result will have no backing
>       * file, and specifying one is therefore an error */
>      if (base_bs == NULL && has_backing_file) {

Kevin