[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: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [PATCH] Fix -snapshot deleting CDROM images |
Date: |
Sun, 25 Jul 2010 20:57:12 +0000 |
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.
> Why not fix bdrv_open_common()?
I've just submitted a new version with a different approach.
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.