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
pgprprOtFoEQf.pgp
Description: PGP signature
