Re: [Qemu-devel] [PATCH v6 for 2.1 05/10] block: Accept node-name arguments for block-commit

[Benoît Canet]

The Tuesday 17 Jun 2014 à 17:53:53 (-0400), Jeff Cody wrote :
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 
> The filename and node-name are mutually exclusive to each other;
> i.e.:
>     "top" and "top-node-name" are mutually exclusive (enforced)
>     "base" and "base-node-name" are mutually exclusive (enforced)
> 
> The preferred and recommended method now of using block-commit
> is to specify the BDS to operate on via their node-name arguments.
> 
> This also adds an explicit check that base_bs is in the chain of
> top_bs, because if they are referenced by their unique node-name
> arguments, the discovery process does not intrinsically verify
> they are in the same chain.
> 
> Reviewed-by: Eric Blake <address@hidden>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c           | 54 
> ++++++++++++++++++++++++++++++++++++++++++++--------
>  qapi/block-core.json | 37 +++++++++++++++++++++++++----------
>  qmp-commands.hx      | 31 ++++++++++++++++++++++--------
>  3 files changed, 96 insertions(+), 26 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a9a3b2f..44f0cc4 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1911,12 +1911,15 @@ void qmp_block_stream(const char *device, bool 
> has_base,
>  
>  void qmp_block_commit(const char *device,
>                        bool has_base, const char *base,
> +                      bool has_base_node_name, const char *base_node_name,
>                        bool has_top, const char *top,
> +                      bool has_top_node_name, const char *top_node_name,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> -    BlockDriverState *bs;
> -    BlockDriverState *base_bs, *top_bs;
> +    BlockDriverState *bs = NULL;
> +    BlockDriverState *base_bs = NULL;
> +    BlockDriverState *top_bs = NULL;
>      Error *local_err = NULL;
>      /* This will be part of the QMP command, if/when the
>       * BlockdevOnError change for blkmirror makes it in
> @@ -1930,6 +1933,16 @@ void qmp_block_commit(const char *device,
>      /* drain all i/o before commits */
>      bdrv_drain_all();
>  
> +    /* argument combination validation */
> +    if (has_base && has_base_node_name) {
> +        error_setg(errp, "'base' and 'base-node-name' are mutually 
> exclusive");
> +        return;
> +    }
> +    if (has_top && has_top_node_name) {
> +        error_setg(errp, "'top' and 'top-node-name' are mutually exclusive");
> +        return;
> +    }
> +
>      /* Important Note:
>       *  libvirt relies on the DeviceNotFound error class in order to probe 
> for
>       *  live commit feature versions; for this to work, we must make sure to
> @@ -1941,14 +1954,16 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> -        return;
> -    }
> -
>      /* default top_bs is the active layer */
>      top_bs = bs;
>  
> -    if (has_top && top) {
> +    if (has_top_node_name) {
> +        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_top && top) {
>          if (strcmp(bs->filename, top) != 0) {
>              top_bs = bdrv_find_backing_image(bs, top);
>          }
> @@ -1959,7 +1974,13 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> -    if (has_base && base) {
> +    if (has_base_node_name) {
> +        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_base && base) {
>          base_bs = bdrv_find_backing_image(top_bs, base);
>      } else {
>          base_bs = bdrv_find_base(top_bs);
> @@ -1976,6 +1997,23 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    /* Verify that 'base' is in the same chain as 'top' */
> +    if (!bdrv_chain_contains(top_bs, base_bs)) {
> +        error_setg(errp, "'base' and 'top' are not in the same chain");
> +        return;
> +    }
> +
> +    /* This should technically be caught in commit_start, but
> +     * check here for validation completeness */
> +    if (!bdrv_chain_contains(bs, top_bs)) {
> +        error_setg(errp, "'%s' and 'top' are not in the same chain", device);
> +        return;
> +    }
> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> +        return;
> +    }
> +
>      if (top_bs == bs) {
>          commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
>                              bs, &local_err);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6ddce4f..dddaa13 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -685,14 +685,29 @@
>  # Live commit of data from overlay image nodes into backing nodes - i.e.,
>  # writes data between 'top' and 'base' into 'base'.
>  #
> -# @device:  the name of the device
> +# @device:                 The name of the device.
>  #
> -# @base:   #optional The file name of the backing image to write data into.
> -#                    If not specified, this is the deepest backing image
> +# For 'base', either @base or @base-node-name may be set but not both. If
> +# neither is specified, this is the deepest backing image.
>  #
> -# @top:    #optional The file name of the backing image within the image 
> chain,
> -#                    which contains the topmost data to be committed down. If
> -#                    not specified, this is the active layer.
> +# @base:          #optional The file name of the backing image to write data
> +#                           into.
> +#
> +# @base-node-name #optional The name of the block driver state node of the
> +#                           backing image to write data into.
> +#                           (Since 2.1)
> +#
> +# For 'top', either @top or @top-node-name must be set but not both. If
> +# neither is specified, this is the active layer.
> +#
> +# @top:           #optional The file name of the backing image within the 
> image
> +#                           chain, which contains the topmost data to be
> +#                           committed down.
> +#
> +# @top-node-name: #optional The block driver state node name of the backing
> +#                           image within the image chain, which contains the
> +#                           topmost data to be committed down.
> +#                           (Since 2.1)
>  #
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by 
> itself,
> @@ -711,17 +726,19 @@
>  #
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
> -#          If @device does not exist, DeviceNotFound
> +#          If @device does not exist or cannot be determined, DeviceNotFound
>  #          If image commit is not supported by this device, NotSupported
> -#          If @base or @top is invalid, a generic error is returned
> +#          If @base, @top, @base-node-name, @top-node-name invalid, 
> GenericError
>  #          If @speed is invalid, InvalidParameter
> +#          If both @base and @base-node-name are specified, GenericError
> +#          If both @top and @top-node-name are specified, GenericError
>  #
>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
> -  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> -            '*speed': 'int' } }
> +  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> +            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
>  
>  ##
>  # @drive-backup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4a8e71a..52af928 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -985,7 +985,7 @@ EQMP
>  
>      {
>          .name       = "block-commit",
> -        .args_type  = "device:B,base:s?,top:s?,speed:o?",
> +        .args_type  = 
> "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_commit,
>      },
>  
> @@ -998,13 +998,28 @@ data between 'top' and 'base' into 'base'.
>  
>  Arguments:
>  
> -- "device": The device's ID, must be unique (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)
> -- "top":  The file name of the backing image within the image chain,
> -          which contains the topmost data to be committed down. If
> -          not specified, this is the active layer. (json-string, optional)
> +- "device":         The device's ID, must be unique (json-string)
> +
> +For 'base', either base or base-node-name may be set but not both. If
> +neither is specified, this is the deepest backing image
> +
> +- "base":           The file name of the backing image to write data into.
> +                    (json-string, optional)
> +- "base-node-name": The name of the block driver state node of the
> +                    backing image to write data into.
> +                    (json-string, optional) (Since 2.1)
> +
> +For 'top', either top or top-node-name must be set but not both. If
> +neither is specified, this is the active layer
> +
> +- "top":            The file name of the backing image within the image 
> chain,
> +                    which contains the topmost data to be committed down.
> +                    (json-string, optional)
> +
> +- "top-node-name":  The block driver state node name of the backing
> +                    image within the image chain, which contains the
> +                    topmost data to be committed down.
> +                    (json-string, optional) (Since 2.1)
>  
>            If top == base, that is an error.
>            If top == active, the job will not be completed by itself,
> -- 
> 1.9.3
> 
Reviewed-by: Benoit Canet <address@hidden>