Patchwork Fix -snapshot deleting CDROM images

login
register
mail settings
Submitter Blue Swirl
Date July 23, 2010, 7:54 p.m.
Message ID <AANLkTimzTgyPUwVBBKQdiccM41LYEUkaQVp0Z+rfvm7c@mail.gmail.com>
Download mbox | patch
Permalink /patch/59831/
State New
Headers show

Comments

Blue Swirl - July 23, 2010, 7:54 p.m.
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(-)
Markus Armbruster - July 24, 2010, 12:03 p.m.
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()?

[...]
Blue Swirl - July 25, 2010, 8:57 p.m.
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.
Markus Armbruster - July 26, 2010, 6:42 a.m.
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/.

Patch

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;
 }