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

[Max Reitz]

On 04.04.2016 15:43, Alberto Garcia wrote:
> 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 <address@hidden>
> ---
>  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));

Alternatively, you could iterate up directly from @bs. Just look for the
BdrvChild in bs->parents with .role == &child_backing.

More interesting question: What happens if you have e.g. a qcow2 file as
a quorum child, and then want to stream inside of the qcow2 backing chain?

So maybe you should first walk up along &child_backing and then along
&child_file/&child_format. I think a generic bdrv_get_root_bs() wouldn't
hurt.

> +
> +    if (active == NULL) {
> +        error_setg(errp, "Cannot find top level node for '%s'", device);
> +        goto out;
> +    }
> +
> +    /* Check for op blockers in the top-level node too */
> +    if (bdrv_op_is_blocked(active, BLOCK_OP_TYPE_STREAM, errp)) {
> +        goto out;
> +    }
> +
>      /* 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) {
> @@ -3038,7 +3053,7 @@ void qmp_block_stream(const char *device,
>      /* backing_file string overrides base bs filename */
>      base_name = has_backing_file ? backing_file : base_name;
>  
> -    stream_start(bs, NULL, base_bs, base_name, has_speed ? speed : 0,
> +    stream_start(bs, active, base_bs, base_name, has_speed ? speed : 0,

stream_start() behaves differently for active == NULL and active == bs.
I think both kinds of behavior work, but it looks weird for active ==
bs. Should we pass NULL in that case instead?

Max

>                   on_error, block_job_cb, bs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 59107ed..e20193a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1405,6 +1405,10 @@
>  # with query-block-jobs.  The operation can be stopped before it has 
> completed
>  # using the block-job-cancel command.
>  #
> +# The node that receives the data is called the top image, can be located
> +# in any part of the whole chain and can be specified using its device
> +# or node name.
> +#
>  # If a base file is specified then sectors are not copied from that base 
> file and
>  # its backing chain.  When streaming completes the image file will have the 
> base
>  # file as its backing file.  This can be used to stream a subset of the 
> backing
> @@ -1413,12 +1417,12 @@
>  # On successful completion the image file is updated to drop the backing file
>  # and the BLOCK_JOB_COMPLETED event is emitted.
>  #
> -# @device: the device name
> +# @device: the device or node name of the top image
>  #
>  # @base:   #optional the common backing file name
>  #
> -# @backing-file: #optional The backing file string to write into the active
> -#                          layer. This filename is not validated.
> +# @backing-file: #optional The backing file string to write into the top
> +#                          image. This filename is not validated.
>  #
>  #                          If a pathname string is such that it cannot be
>  #                          resolved by QEMU, that means that subsequent QMP 
> or
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 3ac2443..107049b 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -126,7 +126,7 @@ class TestSingleDrive(iotests.QMPTestCase):
>  
>      def test_device_not_found(self):
>          result = self.vm.qmp('block-stream', device='nonexistent')
> -        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
> +        self.assert_qmp(result, 'error/class', 'GenericError')
>  
>  
>  class TestSmallerBackingFile(iotests.QMPTestCase):
>

signature.asc
Description: OpenPGP digital signature