[Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device

[Kevin Wolf]

Am 25.06.2010 18:53, schrieb Markus Armbruster:
> savevm.c keeps a pointer to the snapshot block device.  If you manage
> to get that device deleted, the pointer dangles, and the next snapshot
> operation will crash & burn.  Unplugging a guest device that uses it
> does the trick:
> 
>     $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...]
>     QEMU 0.12.50 monitor - type 'help' for more information
>     (qemu) info snapshots
>     No available block device supports snapshots
>     (qemu) drive_add auto if=none,file=tmp.qcow2
>     OK
>     (qemu) device_add usb-storage,id=foo,drive=none1
>     (qemu) info snapshots
>     Snapshot devices: none1
>     Snapshot list (from none1):
>     ID        TAG                 VM SIZE                DATE       VM CLOCK
>     (qemu) device_del foo
>     (qemu) info snapshots
>     Snapshot devices:
>     Segmentation fault (core dumped)
> 
> Move management of that pointer to block.c, and zap it when the device
> it points to goes away.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block.c  |   25 +++++++++++++++++++++++++
>  block.h  |    1 +
>  savevm.c |   31 ++++---------------------------
>  3 files changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5e0ffa0..34055e0 100644
> --- a/block.c
> +++ b/block.c
> @@ -63,6 +63,9 @@ 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;
>  
> @@ -660,6 +663,9 @@ void bdrv_close_all(void)
>  void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->peer);
> +    if (bs == bs_snapshots) {
> +        bs_snapshots = NULL;
> +    }

This should probably be in bdrv_close() instead. A BlockDriverState can
be closed, but not deleted yet; it can't handle snapshots in this state,
though.

>      /* remove from list, if necessary */
>      if (bs->device_name[0] != '\0') {
> @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>      return 1;
>  }
>  
> +BlockDriverState *bdrv_snapshots(void)
> +{
> +    BlockDriverState *bs;
> +
> +    if (bs_snapshots)
> +        return bs_snapshots;

I know that this function is just moved with no changes, but while we're
at it and you need to respin anyway, can we add braces here?

> +
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs)) {
> +            goto ok;
> +        }
> +    }
> +    return NULL;
> + ok:
> +    bs_snapshots = bs;
> +    return bs;
> +}

And instead of a goto we could do the right thing directly in the if block.

Kevin