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 <arm...@redhat.com>
> ---
> 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
