From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59669 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oq5Qo-0004J1-U7 for qemu-devel@nongnu.org; Mon, 30 Aug 2010 10:34:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oq5Lb-0007ot-N9 for qemu-devel@nongnu.org; Mon, 30 Aug 2010 10:28:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2654) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oq5Lb-0007oh-Bk for qemu-devel@nongnu.org; Mon, 30 Aug 2010 10:28:31 -0400 Message-ID: <4C7BC015.5060405@redhat.com> Date: Mon, 30 Aug 2010 16:28:37 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1280944550-6502-1-git-send-email-miguel.filho@gmail.com> <1280944550-6502-4-git-send-email-miguel.filho@gmail.com> In-Reply-To: <1280944550-6502-4-git-send-email-miguel.filho@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 3/3] savevm: prevent snapshot overwriting List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Miguel Di Ciurcio Filho Cc: armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com 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 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