On Sat, Jul 24, 2010 at 12:03 PM, Markus Armbruster <arm...@redhat.com> wrote: > Blue Swirl <blauwir...@gmail.com> 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.
