Blue Swirl <blauwir...@gmail.com> writes: > 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.
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/.
