Re: [Qemu-devel] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

Am 30.09.2015 um 16:45 hat Max Reitz geschrieben:
> On 29.09.2015 15:51, Kevin Wolf wrote:
> > Am 23.09.2015 um 18:36 hat Max Reitz geschrieben:
> >> On 17.09.2015 15:48, Kevin Wolf wrote:
> >>>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> >>>  {
> >>> -    bdrv_swap(bs_new, bs_top);
> >>> +    assert(!bdrv_requests_pending(bs_top));
> >>> +    assert(!bdrv_requests_pending(bs_new));
> >>> +
> >>> +    bdrv_ref(bs_top);
> >>> +    change_parent_backing_link(bs_top, bs_new);
> >>> +
> >>> +    /* Some fields always stay on top of the backing file chain */
> >>> +    swap_feature_fields(bs_top, bs_new);
> >>> +
> >>> +    bdrv_set_backing_hd(bs_new, bs_top);
> >>> +    bdrv_unref(bs_top);
> >>>  
> >>> -    /* The contents of 'tmp' will become bs_top, as we are
> >>> -     * swapping bs_new and bs_top contents. */
> >>> -    bdrv_set_backing_hd(bs_top, bs_new);
> >>> +    /* bs_new is now referenced by its new parents, we don't need the
> >>> +     * additional reference any more. */
> >>> +    bdrv_unref(bs_new);
> >>>  }
> >>
> >> Before, all pointers to @bs_new were moved to @bs_top. Now, they stay at
> >> @bs_new. I suppose we are assuming there are no pointers to @bs_new,
> >> should we assert that, and/or point it out in the documentation?
> > 
> > How would you assert something like this?
> 
> Of course, I have no idea.
> 
> >                                           Also, I think it's currently
> > true, but there's no reason why it should stay so. The important part is
> > just that it's true while applying the patch because the semantics
> > changes. Once it's applied, we have sane behaviour and can make use of
> > it.
> 
> The thing is that this is exactly the reason for the bug Berto found.
> external_snapshot_commit() keeps a reference to state->new_bs (@bs_new)
> and uses it afterwards for bdrv_reopen(), whereas it should be using
> state->old_bs (@bs_top) after this series.

Yes, the interface changes. That means that you need to review all
callers. There aren't that many, so it's doable.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]