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

[Kevin Wolf]

Am 03.05.2021 um 11:45 hat Max Reitz geschrieben:
> 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.

True, that's a good point.

Kevin