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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

Right.  I was thinking about the dangling pointer only.

My patch works as advertized: it fixes the crash.  But zapping
bs_snapshots in bdrv_close() is even better.  I'll respin.


>>      /* 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.

Separate patch.  I hate it when people squash code motion and change
together.