| Message ID | 1280944550-6502-4-git-send-email-miguel.filho@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 04.08.2010 19:55, schrieb Miguel Di Ciurcio Filho: > When savevm is run using an previously saved snapshot id or name, it will > delete the original and create a new one, using the same id and name and not > prompting the user of what just happened. > > This behaviour is not good, IMHO. > > We add a '-f' parameter to savevm, to really force that to happen, in case the > user really wants to. > > New behavior: > (qemu) savevm snap1 > An snapshot named 'snap1' already exists > > (qemu) savevm -f snap1 > > We do better error reporting in case '-f' is used too than before and don't > reuse the previous id. > > Note: This patch depends on "savevm: Generate a name when run without one" > > Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com> I think what this patch is doing is not enough. It only takes bs_snapshots into consideration, but will continue to silently overwrite snapshots in other images. This is where I would consider this check most valuable. I'd like to have this full check implemented before applying this patch. By the way, I've also hit yet another case which is similar, an ID conflict. Assume I have hda with only one snapshot with ID 1 and hdb with snapshots with IDs 1, 2 and 3. savevm will pick hda as bs_snapshots, decide that ID 2 is free, start creating the snapshot and fail when it tries to snapshot hdb. Not requesting a fix for the latter, though, with UUIDs this is going to be fixed anyway. > --- > qemu-monitor.hx | 7 ++++--- > savevm.c | 19 ++++++++++++++----- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > index 2af3de6..683ac73 100644 > --- a/qemu-monitor.hx > +++ b/qemu-monitor.hx > @@ -275,9 +275,10 @@ ETEXI > > { > .name = "savevm", > - .args_type = "name:s?", > - .params = "[tag|id]", > - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", > + .args_type = "force:-f,name:s?", > + .params = "[-f] [tag|id]", > + .help = "save a VM snapshot. If no tag is provided, a new one is created" > + "\n\t\t\t -f to overwrite an snapshot if it already exists", > .mhandler.cmd = do_savevm, > }, > > diff --git a/savevm.c b/savevm.c > index 025bee6..f0a4b78 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1805,6 +1805,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) > struct tm tm; > #endif > const char *name = qdict_get_try_str(qdict, "name"); > + int force = qdict_get_try_bool(qdict, "force", 0); > > /* Verify if there is a device that doesn't support snapshots and is writable */ > bs = NULL; > @@ -1848,12 +1849,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) > > if (name) { > ret = bdrv_snapshot_find(bs, old_sn, name); > - if (ret >= 0) { > - pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > - pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); The id_str copy is completely dropped. Before the change, if you overwrite a snapshot, you'd get a new one with the same ID. Now it's assigned a new ID. This is probably a good thing, as it's no longer compatible with an older snapshot saved on a second disk which is currently not attached. But it should be clearly mentioned in the commit message. Kevin
diff --git a/qemu-monitor.hx b/qemu-monitor.hx index 2af3de6..683ac73 100644 --- a/qemu-monitor.hx +++ b/qemu-monitor.hx @@ -275,9 +275,10 @@ ETEXI { .name = "savevm", - .args_type = "name:s?", - .params = "[tag|id]", - .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .args_type = "force:-f,name:s?", + .params = "[-f] [tag|id]", + .help = "save a VM snapshot. If no tag is provided, a new one is created" + "\n\t\t\t -f to overwrite an snapshot if it already exists", .mhandler.cmd = do_savevm, }, diff --git a/savevm.c b/savevm.c index 025bee6..f0a4b78 100644 --- a/savevm.c +++ b/savevm.c @@ -1805,6 +1805,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) struct tm tm; #endif const char *name = qdict_get_try_str(qdict, "name"); + int force = qdict_get_try_bool(qdict, "force", 0); /* Verify if there is a device that doesn't support snapshots and is writable */ bs = NULL; @@ -1848,12 +1849,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) if (name) { ret = bdrv_snapshot_find(bs, old_sn, name); - if (ret >= 0) { - pstrcpy(sn->name, sizeof(sn->name), old_sn->name); - pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); - } else { - pstrcpy(sn->name, sizeof(sn->name), name); + if (ret == 0) { + if (force) { + ret = del_existing_snapshots(mon, name); + if (ret < 0) { + monitor_printf(mon, "Error deleting snapshot '%s', error: %d\n", name, ret); + goto the_end; + } + } else { + monitor_printf(mon, "An snapshot named '%s' already exists\n", name); + goto the_end; + } } + + pstrcpy(sn->name, sizeof(sn->name), name); } else { #ifdef _WIN32 ptm = localtime(&tb.time);
When savevm is run using an previously saved snapshot id or name, it will delete the original and create a new one, using the same id and name and not prompting the user of what just happened. This behaviour is not good, IMHO. We add a '-f' parameter to savevm, to really force that to happen, in case the user really wants to. New behavior: (qemu) savevm snap1 An snapshot named 'snap1' already exists (qemu) savevm -f snap1 We do better error reporting in case '-f' is used too than before and don't reuse the previous id. Note: This patch depends on "savevm: Generate a name when run without one" Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com> --- qemu-monitor.hx | 7 ++++--- savevm.c | 19 ++++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-)