[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix -snapshot deleting CDROM images
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] Fix -snapshot deleting CDROM images |
Date: |
Mon, 26 Jul 2010 08:42:14 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Blue Swirl <address@hidden> writes:
> On Sat, Jul 24, 2010 at 12:03 PM, Markus Armbruster <address@hidden> wrote:
>> Blue Swirl <address@hidden> writes:
>>
>>> Command line flag '-snapshot' was setting the drive flag 'snapshot'
>>> for all drives. Therefore also CDROM devices were incorrectly marked
>>> with BDRV_O_SNAPSHOT. Thus the backing images were accidentally deleted
>>> at bdrv_open time, for example when changing the image with monitor
>>> 'change' command.
>>>
>>> Fix by adding a separate 'global_snapshot' drive flag for use when the
>>> command line flag '-snapshot' is used. Also add some extra checks
>>> and suppress a kraxelian notation.
>>
>> This patch doesn't feel right to me.
>>
>> The bug you observed is that snapshot=on does something stupid for a
>> certain kind of drive: bdrv_open_common() removes a "temporary" file
>> that isn't temporary. That bug needs fixing. Your patch does not fix
>> it.
>>
>> Instead, it attempts to avoid the bug: snapshot=on now fails with
>> media=cdrom, and the new -drive option global_snapshot=on gets silently
>> ignored with media=cdrom.
>>
>> Why is media=cdrom the only case where the bug can bite?
>
> Because other media types are not removable.
Not true: if=floppy.
Furthermore, "removable" is ultimately determined by the guest device.
Check out commit 7d0d6950. You can't predict whether a BlockDriverState
will be used as removable or not.
>> Why not fix bdrv_open_common()?
>
> I've just submitted a new version with a different approach.
Thanks, I'll have a look.
> I think the following case is still suspicious: the only device is
> changed to one whose format does not support snapshots. It's unrelated
> to this bug though.
>
> Another annoyance I noticed is that changing block.h forces all files
> to be recompiled. I didn't find the culprit easily.
Yes, I noticed the undisciplined use of block.h and block_int.h, too.
The latter should really be limited to block/.