| Message ID | AANLkTimzTgyPUwVBBKQdiccM41LYEUkaQVp0Z+rfvm7c@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Blue Swirl <blauwirbel@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? Why not fix bdrv_open_common()? [...]
On Sat, Jul 24, 2010 at 12:03 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@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.
Blue Swirl <blauwirbel@gmail.com> writes: > On Sat, Jul 24, 2010 at 12:03 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Blue Swirl <blauwirbel@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/.
diff --git a/blockdev.c b/blockdev.c index 0a9dec3..1272a0f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -150,7 +150,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error) int on_read_error, on_write_error; const char *devaddr; DriveInfo *dinfo; - int snapshot = 0; + int snapshot = 0, global_snapshot = 0; int ret; *fatal_error = 1; @@ -178,6 +178,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error) secs = qemu_opt_get_number(opts, "secs", 0); snapshot = qemu_opt_get_bool(opts, "snapshot", 0); + global_snapshot = qemu_opt_get_bool(opts, "global_snapshot", 0); ro = qemu_opt_get_bool(opts, "readonly", 0); file = qemu_opt_get(opts, "file"); @@ -268,6 +269,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error) } } + if (media == MEDIA_CDROM && snapshot) { + /* Can't support snapshots */ + fprintf(stderr, "qemu: '%s' invalid media for snapshot\n", devname); + return NULL; + } + if (media != MEDIA_CDROM && global_snapshot) { + /* Can't support snapshots: ignore -snapshot for CDROMs */ + snapshot = global_snapshot; + } if ((buf = qemu_opt_get(opts, "cache")) != NULL) { if (!strcmp(buf, "off") || !strcmp(buf, "none")) { bdrv_flags |= BDRV_O_NOCACHE; diff --git a/qemu-config.c b/qemu-config.c index 95abe61..e8ecec8 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -48,6 +48,9 @@ QemuOptsList qemu_drive_opts = { .name = "snapshot", .type = QEMU_OPT_BOOL, },{ + .name = "global_snapshot", + .type = QEMU_OPT_BOOL, + },{ .name = "file", .type = QEMU_OPT_STRING, .help = "disk image", diff --git a/vl.c b/vl.c index ba6ee11..2804855 100644 --- a/vl.c +++ b/vl.c @@ -646,8 +646,9 @@ static int drive_init_func(QemuOpts *opts, void *opaque) static int drive_enable_snapshot(QemuOpts *opts, void *opaque) { - if (NULL == qemu_opt_get(opts, "snapshot")) { - qemu_opt_set(opts, "snapshot", "on"); + if (qemu_opt_get(opts, "snapshot") == NULL && + qemu_opt_get(opts, "global_snapshot") == NULL) { + qemu_opt_set(opts, "global_snapshot", "on"); } return 0; }
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. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- blockdev.c | 12 +++++++++++- qemu-config.c | 3 +++ vl.c | 5 +++-- 3 files changed, 17 insertions(+), 3 deletions(-)