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

[Vladimir Sementsov-Ogievskiy]

22.12.2020 19:07, Max Reitz wrote:
> On 16.12.20 07:17, 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           | 12 ++++---
> >   include/block/block_int.h      |  1 +
> >   block/monitor/block-hmp-cmds.c |  3 +-
> >   block/stream.c                 | 50 +++++++++++++++++++---------
> >   blockdev.c                     | 59 ++++++++++++++++++++++++++++------
> >   5 files changed, 94 insertions(+), 31 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index b8094a5ec7..cb0066fd5c 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2517,10 +2517,14 @@
> >   # @device: the device or node name of the top image
> >   #
> >   # @base: the common backing file name.
> > -#        It cannot be set if @base-node is also set.
> > +#        It cannot be set if @base-node or @bottom is also set.
> >   #
> >   # @base-node: the node name of the backing file.
> > -#             It cannot be set if @base is also set. (Since 2.8)
> > +#             It cannot be set if @base or @bottom is also set. (Since 2.8)
> > +#
> > +# @bottom: the last node in the chain that should be streamed into
> > +#          top. It cannot be set if @base or @base-node is also set.
> > +#          It cannot be filter node. (Since 6.0)
>
> As far as I can make out, one of the results of our discussion on v14 was that 
> when using backing-file + bottom, we want to require the user to specify 
> backing-fmt as well.  Now, backing-fmt isn’t present yet. Doesn’t that mean we 
> have to make bottom + backing-file an error until we have backing-fmt (like it 
> was in v14)?

See my answer on 09. I just have some doubts around backing-fmt and decided to 
keep it as is.

I don't think that we really need backing-fmt. We shouldn't have use-cases when 
backing-fmt is set to something another than final base node. Therefore, using 
format_name of final base node is a correct thing to do. So, I don't see the 
reason now for introducing new option.

> Or do you consider the change to patch 9 sufficient to make at least the case 
> work for which backing-file was purportedly introduced, i.e. FD passing?  
> (Patch 9’s new version will just take the format of the base post-streaming, 
> which would be most likely a correct guess when using FD passing.)

Yes, I decided just to keep logic around backing-fmt as is. Hmm, definitely I 
should have described my decision in cover letter, for you not being surprised.

> (I.e., now with patch 9 being more liberal in guessing, perhaps you decided to 
> no longer make backing-fmt mandatory after all.)
>
> Max


--
Best regards,
Vladimir