Miguel Di Ciurcio Filho <miguel.fi...@gmail.com> writes: > This patch address two issues. > > 1) 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.
Debatable. > We add a '-f' parameter to savevm, to really force that to happen, in case the > user really wants to. Incompatible change, looks like it'll break libvirt. Doesn't mean we can't do it ever, but right now may not be the best time. Perhaps after savevm & friends are fully functional in QMP. > 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. > > 2) When savevm is run without a name or id, the name stays blank. > > This is a first step to hide the internal id, because I don't see a reason to > expose this kind of internals to the user. Huh? > The new behavior is when savevm is run without parameters a name will be > created automaticaly, so the snapshot is accessible to the user without > needing > the id when loadvm is run. Aha, now it makes sense. Suggest to swap the paragraphs. > (qemu) savevm > (qemu) info snapshots > ID TAG VM SIZE DATE VM CLOCK > 1 vm-20100728134640 978K 2010-07-28 13:46:40 00:00:08.603 > > We use a name with the format 'vm-YYYYMMDDHHMMSS'. > > TODO: I have no clue on how to create a timestamp string when using Windows. > > Signed-off-by: Miguel Di Ciurcio Filho <miguel.fi...@gmail.com> > --- > qemu-monitor.hx | 9 ++++--- > savevm.c | 59 ++++++++++++++++++++++++++++++++---------------------- > 2 files changed, 40 insertions(+), 28 deletions(-) > > diff --git a/qemu-monitor.hx b/qemu-monitor.hx > index 9c27b31..94e8178 100644 > --- a/qemu-monitor.hx > +++ b/qemu-monitor.hx > @@ -275,14 +275,15 @@ 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] [name]", > + .help = "save a VM snapshot. If no name is provided, a new one > will be generated" > + "\n\t\t\t -f to overwrite an snapshot if it already > exists", > .mhandler.cmd = do_savevm, > }, > > STEXI > -...@item savevm [...@var{tag}|@var{id}] > +...@item savevm [-f] [...@var{tag}] > @findex savevm > Create a snapshot of the whole virtual machine. If @var{tag} is > provided, it is used as human readable identifier. If there is already > diff --git a/savevm.c b/savevm.c > index fb38e8a..ae6f29f 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1785,7 +1785,7 @@ static int del_existing_snapshots(Monitor *mon, const > char *name) > > void do_savevm(Monitor *mon, const QDict *qdict) > { > - BlockDriverState *bs, *bs1; > + BlockDriverState *bs_vm_state, *bs; These renames make the patch harder to review. > QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; > int ret; > QEMUFile *f; > @@ -1796,7 +1796,9 @@ void do_savevm(Monitor *mon, const QDict *qdict) > #else > struct timeval tv; > #endif > + struct tm tm; > 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; > @@ -1813,8 +1815,8 @@ void do_savevm(Monitor *mon, const QDict *qdict) > } > } > > - bs = bdrv_snapshots(); > - if (!bs) { > + bs_vm_state = bdrv_snapshots(); > + if (!bs_vm_state) { > monitor_printf(mon, "No block device can accept snapshots\n"); > return; > } > @@ -1825,15 +1827,6 @@ void do_savevm(Monitor *mon, const QDict *qdict) > vm_stop(0); > > memset(sn, 0, sizeof(*sn)); > - 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); > - } > - } > > /* fill auxiliary fields */ > #ifdef _WIN32 > @@ -1847,13 +1840,31 @@ void do_savevm(Monitor *mon, const QDict *qdict) > #endif > sn->vm_clock_nsec = qemu_get_clock(vm_clock); > > - /* Delete old snapshots of the same name */ > - if (name && del_existing_snapshots(mon, name) < 0) { > - goto the_end; > + if (name) { > + ret = bdrv_snapshot_find(bs_vm_state, old_sn, 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 { > + /* TODO: handle Windows */ > + localtime_r(&tv.tv_sec, &tm); > + strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm); > } > > /* save the VM state */ > - f = qemu_fopen_bdrv(bs, 1); > + f = qemu_fopen_bdrv(bs_vm_state, 1); > if (!f) { > monitor_printf(mon, "Could not open VM state file\n"); > goto the_end; > @@ -1867,23 +1878,23 @@ void do_savevm(Monitor *mon, const QDict *qdict) > } > > /* create the snapshots */ > - > - bs1 = NULL; > - while ((bs1 = bdrv_next(bs1))) { > - if (bdrv_can_snapshot(bs1)) { > + bs = NULL; > + while ((bs = bdrv_next(bs))) { > + if (bdrv_can_snapshot(bs)) { > /* Write VM state size only to the image that contains the state > */ > - sn->vm_state_size = (bs == bs1 ? vm_state_size : 0); > - ret = bdrv_snapshot_create(bs1, sn); > + sn->vm_state_size = (bs_vm_state == bs ? vm_state_size : 0); > + ret = bdrv_snapshot_create(bs, sn); > if (ret < 0) { > monitor_printf(mon, "Error while creating snapshot on > '%s'\n", > - bdrv_get_device_name(bs1)); > + bdrv_get_device_name(bs)); > } > } > } > > the_end: > - if (saved_vm_running) > + if (saved_vm_running) { > vm_start(); > + } I don't like style changes mixed up with functional changes in the same patch. It's fine if you have to touch the code anyway, but here you don't. > } > > int load_vmstate(const char *name)