Re: [Qemu-block] [PATCH 45/54] commit: Add filter-node-name to block-commit

[Max Reitz]

On 21.02.2017 15:58, Kevin Wolf wrote:
> Management tools need to be able to know about every node in the graph
> and need a way to address them. This new option to block-commit allows
> the client to set a node-name for the automatically inserted filter
> driver, and at the same time serves as a witness that this version of
> qemu does automatically insert a filter driver.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/commit.c            |  6 +++---
>  block/mirror.c            |  3 ++-
>  block/replication.c       |  2 +-
>  blockdev.c                | 10 +++++++---
>  include/block/block_int.h | 13 ++++++++++---
>  qapi/block-core.json      |  8 +++++++-
>  qemu-img.c                |  4 ++--
>  7 files changed, 32 insertions(+), 14 deletions(-)

[...]

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 563b30c..a57c0bf 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -780,13 +780,16 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @on_error: The action to take upon error.
>   * @backing_file_str: String to use as the backing file in @top's overlay
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @top. NULL means
> + * that a node name should be autogenerated.
>   * @errp: Error object.
>   *
>   */
>  void commit_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *base, BlockDriverState *top, int64_t 
> speed,
>                    BlockdevOnError on_error, const char *backing_file_str,
> -                  Error **errp);
> +                  const char *filter_node_name, Error **errp);
>  /**
>   * commit_active_start:
>   * @job_id: The id of the newly-created job, or %NULL to use the
> @@ -797,6 +800,9 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>   *                  See @BlockJobCreateFlags
>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>   * @on_error: The action to take upon error.
> + * @filter_node_name: The node name that should be assigned to the filter
> + * driver that the commit job inserts into the graph above @bs. NULL means 
> that

Maybe s/commit/mirror/, but just maybe.

> + * a node name should be autogenerated.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
>   * @errp: Error object.
> @@ -806,8 +812,9 @@ void commit_start(const char *job_id, BlockDriverState 
> *bs,
>  void commit_active_start(const char *job_id, BlockDriverState *bs,
>                           BlockDriverState *base, int creation_flags,
>                           int64_t speed, BlockdevOnError on_error,
> -                         BlockCompletionFunc *cb,
> -                         void *opaque, Error **errp, bool auto_complete);
> +                         const char *filter_node_name,
> +                         BlockCompletionFunc *cb, void *opaque, Error **errp,
> +                         bool auto_complete);
>  /*
>   * mirror_start:
>   * @job_id: The id of the newly-created job, or %NULL to use the
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 893fa34..36d38b9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1304,6 +1304,11 @@
>  #
>  # @speed:  #optional the maximum speed, in bytes per second
>  #
> +# @filter-node-name: #optional the node name that should be assigned to the
> +#                    filter driver that the commit job inserts into the graph
> +#                    above @device. If this option is not given, a node name 
> is

Above @top, actually.

> +#                    autogenerated. (Since: 2.9)
> +#

I'm much less enthusiastic about this than about the previous patch.
This is because this node appears to me just to be some kind of a hack
to silence op blockers and it doesn't seem obvious where to go with this.

For mirror, it's obvious. In the future, the mirror filter will get the
target as a second child and then it will actually perform all of the
mirror work and indeed be something useful.

For commit, no such immediate plans exist, as far as I'm aware. Well,
for active commit it's just the same, of course. Maybe we can model
non-active commit in the same way: Place it above @top, keep everything
writable and then add @base as a second child. Then it would actually
make sense and deserve a user-specifiable node name.

OK, now that I actually see a way to make use of the new node, this
patch suddenly looks better to me.

So with s/@device/@top/:

Reviewed-by: Max Reitz <address@hidden>

>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
>  #          If @device does not exist, DeviceNotFound
> @@ -1323,7 +1328,8 @@
>  ##
>  { 'command': 'block-commit',
>    'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
> -            '*backing-file': 'str', '*speed': 'int' } }
> +            '*backing-file': 'str', '*speed': 'int',
> +            '*filter-node-name': 'str' } }
>  
>  ##
>  # @drive-backup:

signature.asc
Description: OpenPGP digital signature