Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable

[Stefan Hajnoczi]

On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> > From: Stefan Hajnoczi <address@hidden>
> > 
> > The bs_snapshots global variable points to the BlockDriverState which
> > will be used to save vmstate.  This is really a savevm.c concept but was
> > moved into block.c:bdrv_snapshots() when it became clear that hotplug
> > could result in a dangling pointer.
> > 
> > While auditing the block layer's global state I came upon bs_snapshots
> > and realized that a variable is not necessary here.  Simply find the
> > first BlockDriverState capable of internal snapshots each time this is
> > needed.
> > 
> > The behavior of bdrv_snapshots() is preserved across hotplug because new
> > drives are always appended to the bdrv_states list.  This means that
> > calling the new find_vmstate_bs() function is idempotent - it returns
> > the same BlockDriverState unless it was hot-unplugged.
> > 
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Reviewed-by: Wenchao Xia <address@hidden>
> > Signed-off-by: Wenchao Xia <address@hidden>
> > ---
> >  block.c               |   28 ----------------------------
> >  include/block/block.h |    1 -
> >  savevm.c              |   19 +++++++++++++++----
> >  3 files changed, 15 insertions(+), 33 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 3f87489..478a3b2 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> >  static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> >      QLIST_HEAD_INITIALIZER(bdrv_drivers);
> >  
> > -/* The device to use for VM snapshots */
> > -static BlockDriverState *bs_snapshots;
> > -
> >  /* If non-zero, use only whitelisted block drivers */
> >  static int use_bdrv_whitelist;
> >  
> > @@ -1357,9 +1354,6 @@ void bdrv_close(BlockDriverState *bs)
> >      notifier_list_notify(&bs->close_notifiers, bs);
> >  
> >      if (bs->drv) {
> > -        if (bs == bs_snapshots) {
> > -            bs_snapshots = NULL;
> > -        }
> >          if (bs->backing_hd) {
> >              bdrv_delete(bs->backing_hd);
> >              bs->backing_hd = NULL;
> > @@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs)
> >  
> >      bdrv_close(bs);
> >  
> > -    assert(bs != bs_snapshots);
> >      g_free(bs);
> >  }
> >  
> > @@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const 
> > BlockDevOps *ops,
> >  {
> >      bs->dev_ops = ops;
> >      bs->dev_opaque = opaque;
> > -    if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
> > -        bs_snapshots = NULL;
> > -    }
> 
> This hunk isn't replaced by any other code. If I understand correctly
> what it's doing, it prevented you from saving the VM state to a
> removable device, which would be allowed after this patch.
> 
> Is this a change we want to make? Why isn't it described in the commit
> message?

My understanding of this change is different.  Markus is on CC so maybe
he can confirm.

The point of bs_snapshots = NULL is not to prevent you from saving
snapshots.  It's simply to reset the pointer to the next snapshottable
device (used by bdrv_snapshots()).

See the bdrv_close() hunk above which does the same thing, as well as
bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.

So what this hunk does is to reset the bdrv_snapshots() iterator when a
removable device is hooked up to an emulated storage controller.  It's
no longer necessary since this patch drops the global state
(bs_snapshots) and users will always iterate from scratch.

The whole stateful approach was not necessary.

Stefan