Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

[Vladimir Sementsov-Ogievskiy]

27.10.2020 19:42, Andrey Shinkevich wrote:
> On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:
> > 27.10.2020 19:01, Andrey Shinkevich wrote:
> > > On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:
> > > > 22.10.2020 21:13, Andrey Shinkevich wrote:
> > > > > Avoid writing a filter JSON file name and a filter format name to QCOW2
> > > > > image when the backing file is changed after the block stream job.
> > > > > A user is still able to assign the 'backing-file' parameter for a
> > > > > block-stream job keeping in mind the possible issue mentioned above.
> > > > > If the user does not specify the 'backing-file' parameter, QEMU will
> > > > > assign it automatically.
> > > > >
> > > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > > > ---
> > > > >   block/stream.c | 15 +++++++++++++--
> > > > >   blockdev.c     |  9 ++-------
> > > > >   2 files changed, 15 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/block/stream.c b/block/stream.c
> > > > > index e0540ee..1ba74ab 100644
> > > > > --- a/block/stream.c
> > > > > +++ b/block/stream.c
> > > > > @@ -65,6 +65,7 @@ static int stream_prepare(Job *job)
> > > > >       BlockDriverState *bs = blk_bs(bjob->blk);
> > > > >       BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
> > > > >       BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
> > > > > +    BlockDriverState *base_unfiltered = NULL;
> > > > >       Error *local_err = NULL;
> > > > >       int ret = 0;
> > > > > @@ -75,8 +76,18 @@ static int stream_prepare(Job *job)
> > > > >           const char *base_id = NULL, *base_fmt = NULL;
> > > > >           if (base) {
> > > > >               base_id = s->backing_file_str;
> > > > > -            if (base->drv) {
> > > > > -                base_fmt = base->drv->format_name;
> > > > > +            if (base_id) {
> > > > > +                if (base->drv) {
> > > > > +                    base_fmt = base->drv->format_name;
> > > >
> > > > hmm. this doesn't make real sense: so, we assume that user specified 
> > > > backing_file_str, which may not relate to base, but we use 
> > > > base->drv->format_name? But it may be name of the filter driver, which would be 
> > > > wrong..
> > > >
> > > > Any ideas?
> > > >
> > > > 1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..
> > >
> > > I would choose this item #1 but have to check the probing code logic... 
> > > Particularly, I do not remember now if the probing is able to recognize a 
> > > protocol.
> > > The logic for the format_name in the QEMU existent code (I has kept it here in 
> > > the patch) is a slippery way for an imprudent user. That's why I staked on the 
> > > backing_file_str deprication in the previous version.
> > >
> > > > 2. we can do probing now
> > > > 3. we can at least check, if backing_file_str == 
> > >
> > > Not bad for the sanity check but we will search a node by the file name again - 
> > > not good ((
> >
> > Not search, but only check one very likely option.
>
> Yes, just strcmp(). And why a user may not merely specify a desired backing 
> file as the base?

*shrung*

> > Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. 
> > wrong, but it is preexisting), we can:
> >
> >   - add backing-format argument to qapi as pair for backing-file
> >   - deprecate using backing-file without backing-format.
> >
> > Then, after deprecation period we'll have correct code. This may be done in 
> > separate.
> >
> > > > base_unfiltered->filename, in this case we can use 
> > > > base_unfiltered->drv->format_name
> > > >
> > > >
> > > > > +                }
> > > > > +            } else {
> > > > > +                base_unfiltered = bdrv_skip_filters(base);
> > > > > +                if (base_unfiltered) {
> > > > > +                    base_id = base_unfiltered->filename;
> > > > > +                    if (base_unfiltered->drv) {
> > > > > +                        base_fmt = base_unfiltered->drv->format_name;
> > > > > +                    }
> > > > > +                }
> > > > >               }
> > > > >           }
> > > > >           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
> > > > > diff --git a/blockdev.c b/blockdev.c
> > > > > index c917625..0e9c783 100644
> > > > > --- a/blockdev.c
> > > > > +++ b/blockdev.c
> > >
> > > [...]
> > >
> > > > > -    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> > > > > +    stream_start(has_job_id ? job_id : NULL, bs, base_bs,
> > > > > +                 has_backing_file ? backing_file : NULL,
> > > >
> > > > backing_file should be NULL if has_backing_file is false, so you can use just 
> > > > backing_file instead of ternary operator.
> > >
> > > Yes, if reliable. I has kept the conformation with the ternary operator at the 
> > > first parameter above.
> > >
> > > Andrey
> > >
> > > > >                    job_flags, has_speed ? speed : 0, on_error,
> > > > >                    filter_node_name, &local_err);
> > > > >       if (local_err) {


--
Best regards,
Vladimir