Re: [PULL 37/64] block/snapshot: Fix fallback

[Max Reitz]

On 03.05.21 11:40, Kevin Wolf wrote:
> Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben:
> > On Mon, 7 Sept 2020 at 12:11, Kevin Wolf <kwolf@redhat.com> wrote:
> > > From: Max Reitz <mreitz@redhat.com>
> > >
> > > If the top node's driver does not provide snapshot functionality and we
> > > want to fall back to a node down the chain, we need to snapshot all
> > > non-COW children.  For simplicity's sake, just do not fall back if there
> > > is more than one such child.  Furthermore, we really only can fall back
> > > to bs->file and bs->backing, because bdrv_snapshot_goto() has to modify
> > > the child link (notably, set it to NULL).
> > >
> > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >
> > Hi; Coverity thinks it's found a problem with this code
> > (CID 1452774):
>
> Cc: Max as the patch author

Yes, I’m writing a patch to add a comment.

> > > @@ -205,39 +258,46 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >           return ret;
> > >       }
> > >
> > > -    if (bs->file) {
> > > -        BlockDriverState *file;
> > > -        QDict *options = qdict_clone_shallow(bs->options);
> > > +    fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
> > > +    if (fallback_ptr) {
> > > +        QDict *options;
> > >           QDict *file_options;
> > >           Error *local_err = NULL;
> > > +        BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
> > > +        char *subqdict_prefix = g_strdup_printf("%s.", (*fallback_ptr)->name);
> > > +
> > > +        options = qdict_clone_shallow(bs->options);
> > >
> > > -        file = bs->file->bs;
> > >           /* Prevent it from getting deleted when detached from bs */
> > > -        bdrv_ref(file);
> > > +        bdrv_ref(fallback_bs);
> > >
> > > -        qdict_extract_subqdict(options, &file_options, "file.");
> > > +        qdict_extract_subqdict(options, &file_options, subqdict_prefix);
> > >           qobject_unref(file_options);
> > > -        qdict_put_str(options, "file", bdrv_get_node_name(file));
> > > +        g_free(subqdict_prefix);
> > > +
> > > +        qdict_put_str(options, (*fallback_ptr)->name,
> > > +                      bdrv_get_node_name(fallback_bs));
> > >
> > >           if (drv->bdrv_close) {
> > >               drv->bdrv_close(bs);
> > >           }
> > > -        bdrv_unref_child(bs, bs->file);
> > > -        bs->file = NULL;
> > >
> > > -        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
> > > +        bdrv_unref_child(bs, *fallback_ptr);
> > > +        *fallback_ptr = NULL;
> >
> > Here we set *fallback_ptr to NULL...
> >
> > > +
> > > +        ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
> > >           open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
> > >           qobject_unref(options);
> > >           if (open_ret < 0) {
> > > -            bdrv_unref(file);
> > > +            bdrv_unref(fallback_bs);
> > >               bs->drv = NULL;
> > >               /* A bdrv_snapshot_goto() error takes precedence */
> > >               error_propagate(errp, local_err);
> > >               return ret < 0 ? ret : open_ret;
> > >           }
> > >
> > > -        assert(bs->file->bs == file);
> > > -        bdrv_unref(file);
> > > +        assert(fallback_bs == (*fallback_ptr)->bs);
> >
> > ...but here we dereference *fallback_ptr, and Coverity doesn't see
> > anything that it recognizes as being able to change it.
> >
> > > +        bdrv_unref(fallback_bs);
> > >           return ret;
> > >       }
> >
> > False positive, or real issue? (If a false positive, a comment
> > explaining what's going on wouldn't go amiss -- as a human reader
> > I'm kind of confused about whether there's some kind of hidden
> > magic going on here.)
>
> I think it's a false positive because drv->bdrv_open() is supposed to
> give it a non-NULL value again. Not sure if we can make the assumption
> in every case without checking it, but it feels reasonable to require
> that drv->bdrv_open() would return failure otherwise. Max?

Yes.  I think it’s sensible to add an *fallback_ptr non-NULL check to 
the assert condition (i.e.,

assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);

), because the intention of the condition is already to verify that 
.bdrv_open() has opened the right node.  So we might say what’s missing 
is to also assert that it has opened any node at all, but if we’re fine 
with asserting that it has opened the right node (which we did since 
7a9e51198c24), we should definitely be fine with asserting that it has 
opened any node at all.

Max