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

[Kevin Wolf]

Am 18.01.2021 um 18:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 18.01.2021 17:18, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 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>

> > > diff --git a/blockdev.c b/blockdev.c
> > > index b5f11c524b..96c96f8ba6 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -1587,10 +1587,6 @@ static void 
> > > external_snapshot_prepare(BlkActionState *common,
> > >           goto out;
> > >       }
> > > -    /* This removes our old bs and adds the new bs. This is an operation 
> > > that
> > > -     * can fail, so we need to do it in .prepare; undoing it for abort is
> > > -     * always possible. */
> > 
> > This comment is still relevant, it's unrelated to the bdrv_ref().
> 
> I described in commit message, why I dislike this comment :) I can
> keep it of course, it's not critical

Ah, right, I missed this bit in the commit message (or forgot it until I
reached this hunk) and thought it was an accidental removal.

If it's intentional, no reason to change the patch.

Kevin