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

[Vladimir Sementsov-Ogievskiy]

11.12.2020 19:05, Max Reitz wrote:
> 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)?

I just wanted to deprecate 'backing-file' together with base and base-node as a 
next step. If user wants to set backing file unrelated to current 
backing-chain, is it correct at all? It's a direct violation of what's going 
on, and I doubt that other parts of Qemu working with backing-file are prepared 
for such situation. User can do it by hand later.. Anyway, we'll have three 
releases deprecation period for people to come and cry that this is a really 
needed option, so we can support it later on demand.

> (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.)

If we decide that 'backing-file' is really needed, than yes we should require backing-fmt 
to be specified together with backing-file when using new "bottom" interface.

> [...]
>
> > 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


--
Best regards,
Vladimir