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

[Peter Maydell]

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):


> @@ -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.)

thanks
-- PMM