Re: [PATCH v14 10/13] qapi: block-stream: add "bottom" argument

[Max Reitz]

On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
> The code already don't freeze base node and we try to make it prepared
> for the situation when base node is changed during the operation. In
> other words, block-stream doesn't own base node.
>
> Let's introduce a new interface which should replace the current one,
> which will in better relations with the code. Specifying bottom node
> instead of base, and requiring it to be non-filter gives us the
> following benefits:
>
>   - drop difference between above_base and base_overlay, which will be
>     renamed to just bottom, when old interface dropped
>
>   - clean way to work with parallel streams/commits on the same backing
>     chain, which otherwise become a problem when we introduce a filter
>     for stream job
>
>   - cleaner interface. Nobody will surprised the fact that base node may
>     disappear during block-stream, when there is no word about "base" in
>     the interface.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json           |  8 +++--
>   include/block/block_int.h      |  1 +
>   block/monitor/block-hmp-cmds.c |  3 +-
>   block/stream.c                 | 50 +++++++++++++++++++---------
>   blockdev.c                     | 61 ++++++++++++++++++++++++++++------
>   5 files changed, 94 insertions(+), 29 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 04055ef50c..5d6681a35d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2522,6 +2522,10 @@
>   # @base-node: the node name of the backing file.
>   #             It cannot be set if @base is also set. (Since 2.8)
>   #
> +# @bottom: the last node in the chain that should be streamed into
> +#          top. It cannot be set any of @base, @base-node or @backing-file

s/set any/set if any/

But what’s the problem with backing-file?  The fact that specifying 
backing-file means that stream will look for that filename in the 
backing chain when the job is done (so if you use @bottom, we generally 
don’t want to rely on the presence of any nodes below it)?

(If so, I would have thought that we actually want the user to specify 
backing-file so we don’t have to look down below @bottom to look for a 
filename.  Perhaps a @backing-fmt parameter would help.)

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 70900f4f77..e0e19db88b 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>           bdrv_refresh_filename(base_bs);
>       }
>   
> -    /* Check for op blockers in the whole chain between bs and base */
> -    for (iter = bs; iter && iter != base_bs;
> +    if (has_bottom) {
> +        bottom_bs = bdrv_lookup_bs(NULL, bottom, errp);
> +        if (!bottom_bs) {
> +            goto out;
> +        }
> +        if (!bottom_bs->drv) {
> +            error_setg(errp, "Node '%s' is not open", bottom);
> +            goto out;
> +        }
> +        if (bottom_bs->drv->is_filter) {
> +            error_setg(errp, "Node '%s' is filter, use non-filter node"
> +                       "as 'bottom'", bottom);

Missing a space between “node” and “as”.  (Also, probably two articles, 
i.e. “Node '%s' is a filter, use a non-filter node...”.)

The rest looks good to me, but I’m withholding my R-b because I haven’t 
understood why using @bottom precludes giving @backing-file.

Max