Kevin Wolf <kw...@redhat.com> 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 <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.
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.
