Am 01.05.2021 um 00:30 hat Peter Maydell geschrieben:
> On Mon, 7 Sept 2020 at 12:11, Kevin Wolf <kw...@redhat.com> wrote:
> >
> > From: Max Reitz <mre...@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 <vsement...@virtuozzo.com>
> > Signed-off-by: Max Reitz <mre...@redhat.com>
> > Reviewed-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>
> > Reviewed-by: Kevin Wolf <kw...@redhat.com>
>
> Hi; Coverity thinks it's found a problem with this code
> (CID 1452774):
Cc: Max as the patch author
> > @@ -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?
Kevin
