Re: [Qemu-block] [PATCH v4 02/11] block: Accept node-name for block-commit

[Max Reitz]

On 14.07.2016 15:28, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This converts
> block-commit to accept a node-name without lifting the restriction that
> we're operating at a root node.
> 
> As libvirt makes use of the DeviceNotFound error class, we must add
> explicit code to retain this behaviour because qmp_get_root_bs() only
> returns GenericErrors.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  blockdev.c           | 23 +++++++++++------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 739906e..22bf03d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3094,7 +3094,6 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> -    BlockBackend *blk;
>      BlockDriverState *bs;
>      BlockDriverState *base_bs, *top_bs;
>      AioContext *aio_context;
> @@ -3113,22 +3112,22 @@ void qmp_block_commit(bool has_job_id, const char 
> *job_id, const char *device,
>       *  live commit feature versions; for this to work, we must make sure to
>       *  perform the device lookup before any generic errors that may occur 
> in a
>       *  scenario in which all optional arguments are omitted. */
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", device);
> +    bs = qmp_get_root_bs(device, &local_err);
> +    if (!bs) {
> +        bs = bdrv_lookup_bs(device, device, NULL);
> +        if (!bs) {
> +            error_free(local_err);
> +            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                      "Device '%s' not found", device);
> +        } else {
> +            error_propagate(errp, local_err);
> +        }

I would have liked a comment here why this code exists; including an
explanation what libvirt uses DeviceNotFound for (i.e. for probing
whether qemu supports active commit).

Care to add that later?

Max

>          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)) {
> -        error_setg(errp, "Device '%s' has no medium", device);
> -        goto out;
> -    }
> -    bs = blk_bs(blk);
> -
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, errp)) {
>          goto out;
>      }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d9958d5..61f8fd7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1016,7 +1016,7 @@
>  # @job-id: #optional identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
> -# @device:  the name of the device
> +# @device:  the device name or node-name of a root node
>  #
>  # @base:   #optional The file name of the backing image to write data into.
>  #                    If not specified, this is the deepest backing image
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 68fb925..047b6f1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1166,7 +1166,7 @@ Arguments:
>  
>  - "job-id": Identifier for the newly-created block job. If omitted,
>              the device name will be used. (json-string, optional)
> -- "device": The device's ID, must be unique (json-string)
> +- "device": The device name or node-name of a root node (json-string)
>  - "base": The file name of the backing image to write data into.
>            If not specified, this is the deepest backing image
>            (json-string, optional)
>

signature.asc
Description: OpenPGP digital signature