Re: [PATCH v3 04/36] block: bdrv_append(): don't consume reference

[Alberto Garcia <berto@igalia.com>]

On Wed 17 Mar 2021 03:34:57 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> We have too much comments for this feature. It seems better just don't
> do it. Most of real users (tests don't count) have to create additional
> reference.
>
> Drop also comment in external_snapshot_prepare:
>  - bdrv_append doesn't "remove" old bs in common sense, it sounds
>    strange
>  - the fact that bdrv_append can fail is obvious from the context
>  - the fact that we must rollback all changes in transaction abort is
>    known (it's the direct role of abort)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

> @@ -4645,36 +4640,22 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>   * bs_new must not be attached to a BlockBackend.
>   *
>   * This function does not create any image files.
> - *
> - * bdrv_append() takes ownership of a bs_new reference and unrefs it because
> - * that's what the callers commonly need. bs_new will be referenced by the old
> - * parents of bs_top after bdrv_append() returns. If the caller needs to keep a
> - * reference of its own, it must call bdrv_ref().
>   */
>  int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>                  Error **errp)

You could still mention explicitly that the old parents of @bs_top will
add a new reference to @bs_new.

Berto