| Message ID | e15048016ebe0f9f30310d129933f29649a94e42.1364565637.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/29/2013 08:12 AM, Pavel Hrdina wrote: > QMP command "vm-snapshot-save" has also extra optional force parameter > to specify whether replace existing snapshot or not. It also returns > information about created snapshot. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > hmp-commands.hx | 4 ++-- > hmp.c | 11 +++++++++ > hmp.h | 1 + > include/sysemu/sysemu.h | 1 - > qapi-schema.json | 22 ++++++++++++++++++ > qmp-commands.hx | 43 ++++++++++++++++++++++++++++++++++ > savevm.c | 62 +++++++++++++++++++++++++++++++------------------ > 7 files changed, 118 insertions(+), 26 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> Rest of series already has my review from earlier versions, so I only gave them a cursory glance this time around, but assume they are still okay.
On 29.3.2013 17:12, Eric Blake wrote: > On 03/29/2013 08:12 AM, Pavel Hrdina wrote: >> QMP command "vm-snapshot-save" has also extra optional force parameter >> to specify whether replace existing snapshot or not. It also returns >> information about created snapshot. >> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> hmp-commands.hx | 4 ++-- >> hmp.c | 11 +++++++++ >> hmp.h | 1 + >> include/sysemu/sysemu.h | 1 - >> qapi-schema.json | 22 ++++++++++++++++++ >> qmp-commands.hx | 43 ++++++++++++++++++++++++++++++++++ >> savevm.c | 62 +++++++++++++++++++++++++++++++------------------ >> 7 files changed, 118 insertions(+), 26 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Rest of series already has my review from earlier versions, so I only > gave them a cursory glance this time around, but assume they are still okay. > Thanks a lot for your reviews, Pavel
Patch no longer applies. Pavel Hrdina <phrdina@redhat.com> writes: > QMP command "vm-snapshot-save" has also extra optional force parameter > to specify whether replace existing snapshot or not. It also returns > information about created snapshot. Took me a minute to realize that you're comparing it to HMP command savevm. > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > hmp-commands.hx | 4 ++-- > hmp.c | 11 +++++++++ > hmp.h | 1 + > include/sysemu/sysemu.h | 1 - > qapi-schema.json | 22 ++++++++++++++++++ > qmp-commands.hx | 43 ++++++++++++++++++++++++++++++++++ > savevm.c | 62 +++++++++++++++++++++++++++++++------------------ > 7 files changed, 118 insertions(+), 26 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 3d98604..382b87d 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -310,7 +310,7 @@ ETEXI > .args_type = "name:s?", > .params = "[tag|id]", > .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", > - .mhandler.cmd = do_savevm, > + .mhandler.cmd = hmp_vm_snapshot_save, > }, > > STEXI > @@ -318,7 +318,7 @@ STEXI > @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 > -a snapshot with the same tag or ID, it is replaced. More info at > +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at > @ref{vm_snapshots}. > ETEXI > Markup fix squashed in. Acceptable, because it's really minor. > diff --git a/hmp.c b/hmp.c > index dbe9b90..b38b6ce 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1433,3 +1433,14 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict) > qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err); > hmp_handle_error(mon, &local_err); > } > + > +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) > +{ > + const char *name = qdict_get_try_str(qdict, "name"); > + Error *err = NULL; > + SnapshotInfo *info = NULL; > + > + info = qmp_vm_snapshot_save(!!name, name, true, true, &err); > + qapi_free_SnapshotInfo(info); > + hmp_handle_error(mon, &err); > +} > diff --git a/hmp.h b/hmp.h > index 80e8b41..1bb8590 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -86,5 +86,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict); > void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict); > void hmp_chardev_add(Monitor *mon, const QDict *qdict); > void hmp_chardev_remove(Monitor *mon, const QDict *qdict); > +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict); > > #endif > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 9e6a4a9..87b82d7 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify); > > void qemu_add_machine_init_done_notifier(Notifier *notify); > > -void do_savevm(Monitor *mon, const QDict *qdict); > int load_vmstate(const char *name); > void do_delvm(Monitor *mon, const QDict *qdict); > void do_info_snapshots(Monitor *mon, const QDict *qdict); > diff --git a/qapi-schema.json b/qapi-schema.json > index f629a24..cbb73d9 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3453,3 +3453,25 @@ > # Since: 1.5 > ## > { 'command': 'query-tpm', 'returns': ['TPMInfo'] } > + > +## > +# @vm-snapshot-save: > +# > +# Create a snapshot of the whole virtual machine. If tag is provided as @name, > +# it is used as human readable identifier. If there is already a snapshot > +# with the same tag or id, the force argument needs to be true to replace it. "tag or id"? HMP command savevm's argument is matched against both QEMUSnapshotInfo member id_str (a.k.a. id) and name (a.k.a. tag). Do we really want that kind of overloading in QMP? Shouldn't we make it tag vs. id explicit there? > +# > +# The VM is automatically stopped and resumed and saving a snapshot can take > +# a long time. > +# > +# @name: #optional tag of new snapshot or tag|id of existing snapshot Should we make this mandatory? We have to keep the argument optional in HMP, but that needn't concern us here. > +# > +# @force: #optional specify whether existing snapshot is replaced or not, > +# default is false > +# > +# Returns: SnapshotInfo on success > +# > +# Since: 1.5 > +## > +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'}, > + 'returns': 'SnapshotInfo' } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 1e0e11e..119e7a5 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1445,6 +1445,49 @@ Example: > > EQMP > { > + .name = "vm-snapshot-save", > + .args_type = "name:s?,force:b?", > + .params = "[name] [force]", > + .help = "save a VM snapshot, to replace existing snapshot use force parameter", > + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save > + }, > + > +SQMP > +vm-snapshot-save > +------ > + > +Create a snapshot of the whole virtual machine. If tag is provided as name, > +it is used as human readable identifier. If there is already a snapshot with > +the same tag, the force argument needs to be true to replace it. > + > +The VM is automatically stopped and resumed and saving a snapshot can take > +a long time. > + > +Arguments: > + > +- "name": tag of new snapshot or tag|id of existing snapshot > + (json-string, optional) > + > +- "force": specify whether existing snapshot is replaced or not, > + default is false (json-bool, optional) > + > +Example: > + > +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } > +<- { > + "return": { > + "id": "1", > + "name": "my_snapshot", > + "date-sec": 1364480534, > + "date-nsec": 978215000, > + "vm-clock-sec": 5, > + "vm-clock-nsec": 153620449, > + "vm-state-size": 5709953 > + } > + } > + > +EQMP > + { > .name = "qmp_capabilities", > .args_type = "", > .params = "", > diff --git a/savevm.c b/savevm.c > index 3c1ac9e..7b127eb 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2249,17 +2249,18 @@ static int del_existing_snapshots(const char *name, Error **errp) > return 0; > } > > -void do_savevm(Monitor *mon, const QDict *qdict) > +SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name, > + bool has_force, bool force, Error **errp) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; > + SnapshotInfo *info = NULL; > int ret; > QEMUFile *f; > int saved_vm_running; > uint64_t vm_state_size; > qemu_timeval tv; > struct tm tm; > - const char *name = qdict_get_try_str(qdict, "name"); > Error *local_err = NULL; > > /* Verify if there is a device that doesn't support snapshots and is writable */ > @@ -2271,16 +2272,16 @@ void do_savevm(Monitor *mon, const QDict *qdict) > } > > if (!bdrv_can_snapshot(bs)) { > - monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n", > - bdrv_get_device_name(bs)); > - return; > + error_setg(errp, "device '%s' is writable but does not support " > + "snapshots", bdrv_get_device_name(bs)); > + return NULL; Any particular reason for de-capitalizing "Device"? More of the same below. > } > } > > bs = bdrv_snapshots(); > if (!bs) { > - monitor_printf(mon, "No block device can accept snapshots\n"); > - return; > + error_setg(errp, "no block device can accept snapshots"); > + return NULL; > } > > saved_vm_running = runstate_is_running(); > @@ -2294,11 +2295,23 @@ void do_savevm(Monitor *mon, const QDict *qdict) > sn->date_nsec = tv.tv_usec * 1000; > sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); > > - if (name) { > + if (has_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); > + if (has_force && force) { > + pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > + pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > + > + /* Delete old snapshots of the same name */ > + if (del_existing_snapshots(name, &local_err) < 0) { > + error_propagate(errp, local_err); > + goto the_end; > + } > + } else { > + error_setg(errp, "snapshot '%s' exists, for override use " > + "'force' parameter", name); > + goto the_end; > + } > } else { > pstrcpy(sn->name, sizeof(sn->name), name); > } } else { /* cast below needed for OpenBSD where tv_sec is still 'long' */ localtime_r((const time_t *)&tv.tv_sec, &tm); > @@ -2308,24 +2321,17 @@ void do_savevm(Monitor *mon, const QDict *qdict) > strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm); Interestingly, we do not check whether this auto-generated name already exists. Not your fault, but could use fixing. > } > > - /* Delete old snapshots of the same name */ > - if (name && del_existing_snapshots(name, &local_err) < 0) { > - monitor_printf(mon, "%s\n", error_get_pretty(local_err)); > - error_free(local_err); > - goto the_end; > - } > - Your code motion adds two conditions for calling del_existing_snapshots(): 1. has_force && force: feature. I guess I'd add the feature in a separate patch, to keep this one as simple as possible. Your choice. 2. bdrv_snapshot_find() succeeds. Care to explain why this one's correct? > /* save the VM state */ > f = qemu_fopen_bdrv(bs, 1); > if (!f) { > - monitor_printf(mon, "Could not open VM state file\n"); > + error_setg(errp, "failed to open '%s' file", bdrv_get_device_name(bs)); > goto the_end; > } > - ret = qemu_savevm_state(f, NULL); > + ret = qemu_savevm_state(f, &local_err); > vm_state_size = qemu_ftell(f); > qemu_fclose(f); > if (ret < 0) { > - monitor_printf(mon, "Error %d while writing VM\n", ret); > + error_propagate(errp, local_err); > goto the_end; > } > > @@ -2336,17 +2342,27 @@ void do_savevm(Monitor *mon, const QDict *qdict) > if (bdrv_can_snapshot(bs1)) { > /* 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, NULL); > + ret = bdrv_snapshot_create(bs1, sn, &local_err); > if (ret < 0) { > - monitor_printf(mon, "Error while creating snapshot on '%s'\n", > - bdrv_get_device_name(bs1)); > + error_propagate(errp, local_err); When we go through this failure... > } > } > } > > + info = g_malloc0(sizeof(SnapshotInfo)); > + info->id = g_strdup(sn->id_str); > + info->name = g_strdup(sn->name); > + info->date_nsec = sn->date_nsec; > + info->date_sec = sn->date_sec; > + info->vm_state_size = vm_state_size; > + info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000; > + info->vm_clock_sec = sn->vm_clock_nsec / 1000000000; > + > the_end: > if (saved_vm_running) > vm_start(); > + > + return info; ... we return both info and an Error. Stupid question: is that okay? > } > > void qmp_xen_save_devices_state(const char *filename, Error **errp)
On 04/09/2013 10:04 AM, Markus Armbruster wrote: >> + >> +## >> +# @vm-snapshot-save: >> +# >> +# Create a snapshot of the whole virtual machine. If tag is provided as @name, >> +# it is used as human readable identifier. If there is already a snapshot >> +# with the same tag or id, the force argument needs to be true to replace it. > > "tag or id"? > > HMP command savevm's argument is matched against both QEMUSnapshotInfo > member id_str (a.k.a. id) and name (a.k.a. tag). Do we really want that > kind of overloading in QMP? Shouldn't we make it tag vs. id explicit > there? The qcow2 file format already allows for the creation of a snapshot with an id but no tag. We've been back and forth about whether QMP should allow a user to be able to create all valid qcow2 files (and hence allow the omission of tag). But things really get confusing if you allow the creation of a tag that matches an existing id (create a snapshot, then create another snapshot with the tag '1'); or even if a tag can match a potential future id (create a snapshot with a tag named '2', then create another snapshot). So the lookup should always check for BOTH tag and id, even though the command is supplying only a 'tag'; and if a tag is omitted, a unique id will always be returned. > >> +# >> +# The VM is automatically stopped and resumed and saving a snapshot can take >> +# a long time. >> +# >> +# @name: #optional tag of new snapshot or tag|id of existing snapshot > > Should we make this mandatory? We have to keep the argument optional in > HMP, but that needn't concern us here. If we mandate that a tag always be supplied, then we cannot create qcow2 files with a snapshot that lacks a tag using just QMP; but even if we do that, we STILL have to support use of existing qcow2 files that were created by earlier qemu and/or other tools that have snapshots without a tag.
Eric Blake <eblake@redhat.com> writes: > On 04/09/2013 10:04 AM, Markus Armbruster wrote: > >>> + >>> +## >>> +# @vm-snapshot-save: >>> +# >>> +# Create a snapshot of the whole virtual machine. If tag is >>> provided as @name, >>> +# it is used as human readable identifier. If there is already a snapshot >>> +# with the same tag or id, the force argument needs to be true to >>> replace it. >> >> "tag or id"? >> >> HMP command savevm's argument is matched against both QEMUSnapshotInfo >> member id_str (a.k.a. id) and name (a.k.a. tag). Do we really want that >> kind of overloading in QMP? Shouldn't we make it tag vs. id explicit >> there? > > The qcow2 file format already allows for the creation of a snapshot with > an id but no tag. We've been back and forth about whether QMP should > allow a user to be able to create all valid qcow2 files (and hence allow > the omission of tag). But things really get confusing if you allow the I don't have all that context; I hope I'm not wasting everyone's time. > creation of a tag that matches an existing id (create a snapshot, then > create another snapshot with the tag '1'); or even if a tag can match a > potential future id (create a snapshot with a tag named '2', then create > another snapshot). So the lookup should always check for BOTH tag and > id, even though the command is supplying only a 'tag'; and if a tag is > omitted, a unique id will always be returned. > >> >>> +# >>> +# The VM is automatically stopped and resumed and saving a snapshot can take >>> +# a long time. >>> +# >>> +# @name: #optional tag of new snapshot or tag|id of existing snapshot >> >> Should we make this mandatory? We have to keep the argument optional in >> HMP, but that needn't concern us here. > > If we mandate that a tag always be supplied, then we cannot create qcow2 > files with a snapshot that lacks a tag using just QMP; but even if we do Are you sure you can do that with the proposed patches? If I read them correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given. > that, we STILL have to support use of existing qcow2 files that were > created by earlier qemu and/or other tools that have snapshots without a > tag. I understand the need to deal with existing qcow2 files lacking tags. I understand the desire to be able to create such files via QMP. I don't share it, though :) For block driver method bdrv_snapshot_create() both tag and ID are optional. If ID is missing, it picks one. qcow2 picks max. existing ID + 1. If tag is missing, the snapshot doesn't get one. When savevm overwrites an existing snapshot, it reuses both tag and ID. One of them matches @name. When savevm creates a new snapshot, @name becomes the tag. If @name isn't given, we make one up. ID is left to the block driver. vm-snapshot-save behaves the same. Say we have two snapshots, with IDs 1 and 5. Say I want to overwrite the first one, and chose to identify it by its ID. Since I got dirt on my monitor, I misread ID 7. Since neither ID nor tag 7 exist, I accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2 block driver). Next new snapshot will inevitably get ID 7 and the confusion is complete. All because the stupid command doesn't let me express myself clearly: I mean *ID* 7, *not* tag 7! I think I'd try the following for QMP: * Drop the capability to overwrite existing snapshot. No real loss, as it's not an atomic overwrite: it first deletes the old one, then creates the new one, and if create fails, the old one's still gone. * Take parameter @tag and @id. Fail if @tag is given and matches any existing tag. If it's not given, we make one up that doesn't match any existing tag. Fail if @id is given and matches any existing ID. If it's not given, we make one up that doesn't match any existing ID. * Seriously consider making @tag mandatory.
On 04/09/2013 11:23 AM, Markus Armbruster wrote: >> If we mandate that a tag always be supplied, then we cannot create qcow2 >> files with a snapshot that lacks a tag using just QMP; but even if we do > > Are you sure you can do that with the proposed patches? If I read them > correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given. HMP is higher-level than QMP, so having HMP default to always generating a tag seems like a reasonable thing. In my opinion, it's okay for higher layers to ensure a name even when lower layers leave it optional. Besides, HMP aims to ease user interface and it is easier to work with named snapshots than it is to work with anonymous ones, even if that means HMP generates a name. We already have instances where HMP is intentionally less powerful than QMP, because if you are using the human interface, you don't need to be bothered by the subtleties - and if you need the full power, then you use the lower-level interface. On the other hand, we could always change our mind later to have HMP create anonymous snapshots (by adding another flag to HMP 'savevm') instead of generating a name. Where we would get into trouble is if QMP enforces a name but we change our mind and later want HMP to again expose the possibility of anonymous snapshots. Also, if QMP doesn't expose the full power of the file format on creation, but still has to support the full power of the file format on the loadvm side, it becomes that much harder to test that the loadvm support is correct in the presence of anonymous snapshots. > >> that, we STILL have to support use of existing qcow2 files that were >> created by earlier qemu and/or other tools that have snapshots without a >> tag. > > I understand the need to deal with existing qcow2 files lacking tags. > > I understand the desire to be able to create such files via QMP. I > don't share it, though :) I'm on the fence - I could live with the requirement that QMP requires a name, and that creation of anonymous snapshots requires other tools. Libvirt will always pass a name, for what it is worth, regardless of whether HMP generates a name, and regardless of whether QMP requires a name. But testing nameless support is harder if it requires an extra tool to create a nameless snapshot in the first place. > > For block driver method bdrv_snapshot_create() both tag and ID are > optional. If ID is missing, it picks one. qcow2 picks max. existing ID > + 1. If tag is missing, the snapshot doesn't get one. > > When savevm overwrites an existing snapshot, it reuses both tag and ID. > One of them matches @name. > > When savevm creates a new snapshot, @name becomes the tag. If @name > isn't given, we make one up. ID is left to the block driver. > > vm-snapshot-save behaves the same. > > Say we have two snapshots, with IDs 1 and 5. Say I want to overwrite > the first one, and chose to identify it by its ID. Since I got dirt on > my monitor, I misread ID 7. Since neither ID nor tag 7 exist, I > accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2 > block driver). Next new snapshot will inevitably get ID 7 and the > confusion is complete. All because the stupid command doesn't let me > express myself clearly: I mean *ID* 7, *not* tag 7! Yep - another instance of the confusion that current HMP 'savevm' has, and that I want to get rid of by adding -f as a force option at a minimum to the HMP layer. > > I think I'd try the following for QMP: > > * Drop the capability to overwrite existing snapshot. No real loss, as > it's not an atomic overwrite: it first deletes the old one, then > creates the new one, and if create fails, the old one's still gone. We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you are saying that the HMP command would then be a sequence of _two_ QMP calls - delete then create, where QMP create _always_ fails if a snapshot already exists with that name. Makes sense - it just means that the 'force' semantics are now put as a burden on the caller, instead of on QMP. > > * Take parameter @tag and @id. > > Fail if @tag is given and matches any existing tag. If it's not > given, we make one up that doesn't match any existing tag. Generating an id makes sense. And the command must return the id that got used (whether passed in by the user or generated). > > Fail if @id is given and matches any existing ID. If it's not given, > we make one up that doesn't match any existing ID. I don't know if I like the idea of QMP generating a tag, vs. leaving the snapshot anonymous. But note that the answer to the next question determines whether we can bypass this question, since if... > > * Seriously consider making @tag mandatory. ...tag is made mandatory, then QMP never has to generate a tag. I'm still on the fence on whether to always have a tag in place (whether by QMP generation or by mandatory argument) vs. allowing anonymous snapshots. But again, libvirt always names its snapshots, and I trust your judgment as maintainer of this area of code in which way we should go.
[Note Cc: Kevin] Eric Blake <eblake@redhat.com> writes: > On 04/09/2013 11:23 AM, Markus Armbruster wrote: >>> If we mandate that a tag always be supplied, then we cannot create qcow2 >>> files with a snapshot that lacks a tag using just QMP; but even if we do >> >> Are you sure you can do that with the proposed patches? If I read them >> correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given. > > HMP is higher-level than QMP, so having HMP default to always generating > a tag seems like a reasonable thing. In my opinion, it's okay for > higher layers to ensure a name even when lower layers leave it optional. > Besides, HMP aims to ease user interface and it is easier to work with > named snapshots than it is to work with anonymous ones, even if that > means HMP generates a name. We already have instances where HMP is > intentionally less powerful than QMP, because if you are using the human > interface, you don't need to be bothered by the subtleties - and if you > need the full power, then you use the lower-level interface. On the > other hand, we could always change our mind later to have HMP create > anonymous snapshots (by adding another flag to HMP 'savevm') instead of > generating a name. We agree on HMP. > Where we would get into trouble is if QMP enforces a name but we change > our mind and later want HMP to again expose the possibility of anonymous > snapshots. The proposed patches don't let you create anonymous snapshots via QMP, as far as I can tell. The same default tag is used. > Also, if QMP doesn't expose the full power of the file > format on creation, but still has to support the full power of the file > format on the loadvm side, it becomes that much harder to test that the > loadvm support is correct in the presence of anonymous snapshots. Are anonymous snapshots actually good for anything? Or are they just a regrettable historical accident we need to cope with somehow? In the latter case, we can handle them in qemu (loadvm accepts them), or in qemu-img (set snapshot tag). Regardless of where we support anonymous snapshots, we need to be able to create them for testing purposes. We can create them either in qemu (vm-snapshot-save), or in qemu-img (unset snapshot tag). >>> that, we STILL have to support use of existing qcow2 files that were >>> created by earlier qemu and/or other tools that have snapshots without a >>> tag. >> >> I understand the need to deal with existing qcow2 files lacking tags. >> >> I understand the desire to be able to create such files via QMP. I >> don't share it, though :) > > I'm on the fence - I could live with the requirement that QMP requires a > name, and that creation of anonymous snapshots requires other tools. > Libvirt will always pass a name, for what it is worth, regardless of > whether HMP generates a name, and regardless of whether QMP requires a > name. But testing nameless support is harder if it requires an extra > tool to create a nameless snapshot in the first place. Not really hard if we limit anonymous snapshot support to qemu-img: change snapshot tag. As far as I can tell, qemu can't create anonymous snapshots anymore since commit 7d631a1 "savevm: Generate a name when run without one" from August 2010. Hasn't impeded testing so far (possibly because there has been none). >> For block driver method bdrv_snapshot_create() both tag and ID are >> optional. If ID is missing, it picks one. qcow2 picks max. existing ID >> + 1. If tag is missing, the snapshot doesn't get one. >> >> When savevm overwrites an existing snapshot, it reuses both tag and ID. >> One of them matches @name. >> >> When savevm creates a new snapshot, @name becomes the tag. If @name >> isn't given, we make one up. ID is left to the block driver. >> >> vm-snapshot-save behaves the same. >> >> Say we have two snapshots, with IDs 1 and 5. Say I want to overwrite >> the first one, and chose to identify it by its ID. Since I got dirt on >> my monitor, I misread ID 7. Since neither ID nor tag 7 exist, I >> accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2 >> block driver). Next new snapshot will inevitably get ID 7 and the >> confusion is complete. All because the stupid command doesn't let me >> express myself clearly: I mean *ID* 7, *not* tag 7! > > Yep - another instance of the confusion that current HMP 'savevm' has, > and that I want to get rid of by adding -f as a force option at a > minimum to the HMP layer. Current patches' -f doesn't help in my example. >> I think I'd try the following for QMP: >> >> * Drop the capability to overwrite existing snapshot. No real loss, as >> it's not an atomic overwrite: it first deletes the old one, then >> creates the new one, and if create fails, the old one's still gone. > > We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you > are saying that the HMP command would then be a sequence of _two_ QMP > calls - delete then create, where QMP create _always_ fails if a > snapshot already exists with that name. Makes sense - it just means > that the 'force' semantics are now put as a burden on the caller, > instead of on QMP. I'm fine with HMP 'savevm -f'. In HMP, we go for user convenience in common use cases. For QMP, it's generally better to go for building blocks, i.e. commands that do just one thing. Let the client combine them however it sees fit. >> * Take parameter @tag and @id. >> >> Fail if @tag is given and matches any existing tag. If it's not >> given, we make one up that doesn't match any existing tag. > > Generating an id makes sense. And the command must return the id that > got used (whether passed in by the user or generated). Yes. >> Fail if @id is given and matches any existing ID. If it's not given, >> we make one up that doesn't match any existing ID. > > I don't know if I like the idea of QMP generating a tag, vs. leaving the > snapshot anonymous. But note that the answer to the next question > determines whether we can bypass this question, since if... > >> >> * Seriously consider making @tag mandatory. > > ...tag is made mandatory, then QMP never has to generate a tag. I'm > still on the fence on whether to always have a tag in place (whether by > QMP generation or by mandatory argument) vs. allowing anonymous > snapshots. But again, libvirt always names its snapshots, and I trust > your judgment as maintainer of this area of code in which way we should go. From a QMP point of view, I like a mandatory tag, because it reduces headaches. For the QCOW2 / block layer / qemu-img point of view, I'm cc'ing Kevin.
diff --git a/hmp-commands.hx b/hmp-commands.hx index 3d98604..382b87d 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -310,7 +310,7 @@ ETEXI .args_type = "name:s?", .params = "[tag|id]", .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", - .mhandler.cmd = do_savevm, + .mhandler.cmd = hmp_vm_snapshot_save, }, STEXI @@ -318,7 +318,7 @@ STEXI @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 -a snapshot with the same tag or ID, it is replaced. More info at +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at @ref{vm_snapshots}. ETEXI diff --git a/hmp.c b/hmp.c index dbe9b90..b38b6ce 100644 --- a/hmp.c +++ b/hmp.c @@ -1433,3 +1433,14 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict) qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err); hmp_handle_error(mon, &local_err); } + +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) +{ + const char *name = qdict_get_try_str(qdict, "name"); + Error *err = NULL; + SnapshotInfo *info = NULL; + + info = qmp_vm_snapshot_save(!!name, name, true, true, &err); + qapi_free_SnapshotInfo(info); + hmp_handle_error(mon, &err); +} diff --git a/hmp.h b/hmp.h index 80e8b41..1bb8590 100644 --- a/hmp.h +++ b/hmp.h @@ -86,5 +86,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict); void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict); void hmp_chardev_add(Monitor *mon, const QDict *qdict); void hmp_chardev_remove(Monitor *mon, const QDict *qdict); +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict); #endif diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 9e6a4a9..87b82d7 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify); void qemu_add_machine_init_done_notifier(Notifier *notify); -void do_savevm(Monitor *mon, const QDict *qdict); int load_vmstate(const char *name); void do_delvm(Monitor *mon, const QDict *qdict); void do_info_snapshots(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index f629a24..cbb73d9 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3453,3 +3453,25 @@ # Since: 1.5 ## { 'command': 'query-tpm', 'returns': ['TPMInfo'] } + +## +# @vm-snapshot-save: +# +# Create a snapshot of the whole virtual machine. If tag is provided as @name, +# it is used as human readable identifier. If there is already a snapshot +# with the same tag or id, the force argument needs to be true to replace it. +# +# The VM is automatically stopped and resumed and saving a snapshot can take +# a long time. +# +# @name: #optional tag of new snapshot or tag|id of existing snapshot +# +# @force: #optional specify whether existing snapshot is replaced or not, +# default is false +# +# Returns: SnapshotInfo on success +# +# Since: 1.5 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'}, + 'returns': 'SnapshotInfo' } diff --git a/qmp-commands.hx b/qmp-commands.hx index 1e0e11e..119e7a5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1445,6 +1445,49 @@ Example: EQMP { + .name = "vm-snapshot-save", + .args_type = "name:s?,force:b?", + .params = "[name] [force]", + .help = "save a VM snapshot, to replace existing snapshot use force parameter", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save + }, + +SQMP +vm-snapshot-save +------ + +Create a snapshot of the whole virtual machine. If tag is provided as name, +it is used as human readable identifier. If there is already a snapshot with +the same tag, the force argument needs to be true to replace it. + +The VM is automatically stopped and resumed and saving a snapshot can take +a long time. + +Arguments: + +- "name": tag of new snapshot or tag|id of existing snapshot + (json-string, optional) + +- "force": specify whether existing snapshot is replaced or not, + default is false (json-bool, optional) + +Example: + +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } +<- { + "return": { + "id": "1", + "name": "my_snapshot", + "date-sec": 1364480534, + "date-nsec": 978215000, + "vm-clock-sec": 5, + "vm-clock-nsec": 153620449, + "vm-state-size": 5709953 + } + } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "", diff --git a/savevm.c b/savevm.c index 3c1ac9e..7b127eb 100644 --- a/savevm.c +++ b/savevm.c @@ -2249,17 +2249,18 @@ static int del_existing_snapshots(const char *name, Error **errp) return 0; } -void do_savevm(Monitor *mon, const QDict *qdict) +SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name, + bool has_force, bool force, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; + SnapshotInfo *info = NULL; int ret; QEMUFile *f; int saved_vm_running; uint64_t vm_state_size; qemu_timeval tv; struct tm tm; - const char *name = qdict_get_try_str(qdict, "name"); Error *local_err = NULL; /* Verify if there is a device that doesn't support snapshots and is writable */ @@ -2271,16 +2272,16 @@ void do_savevm(Monitor *mon, const QDict *qdict) } if (!bdrv_can_snapshot(bs)) { - monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n", - bdrv_get_device_name(bs)); - return; + error_setg(errp, "device '%s' is writable but does not support " + "snapshots", bdrv_get_device_name(bs)); + return NULL; } } bs = bdrv_snapshots(); if (!bs) { - monitor_printf(mon, "No block device can accept snapshots\n"); - return; + error_setg(errp, "no block device can accept snapshots"); + return NULL; } saved_vm_running = runstate_is_running(); @@ -2294,11 +2295,23 @@ void do_savevm(Monitor *mon, const QDict *qdict) sn->date_nsec = tv.tv_usec * 1000; sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); - if (name) { + if (has_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); + if (has_force && force) { + pstrcpy(sn->name, sizeof(sn->name), old_sn->name); + pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); + + /* Delete old snapshots of the same name */ + if (del_existing_snapshots(name, &local_err) < 0) { + error_propagate(errp, local_err); + goto the_end; + } + } else { + error_setg(errp, "snapshot '%s' exists, for override use " + "'force' parameter", name); + goto the_end; + } } else { pstrcpy(sn->name, sizeof(sn->name), name); } @@ -2308,24 +2321,17 @@ void do_savevm(Monitor *mon, const QDict *qdict) strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm); } - /* Delete old snapshots of the same name */ - if (name && del_existing_snapshots(name, &local_err) < 0) { - monitor_printf(mon, "%s\n", error_get_pretty(local_err)); - error_free(local_err); - goto the_end; - } - /* save the VM state */ f = qemu_fopen_bdrv(bs, 1); if (!f) { - monitor_printf(mon, "Could not open VM state file\n"); + error_setg(errp, "failed to open '%s' file", bdrv_get_device_name(bs)); goto the_end; } - ret = qemu_savevm_state(f, NULL); + ret = qemu_savevm_state(f, &local_err); vm_state_size = qemu_ftell(f); qemu_fclose(f); if (ret < 0) { - monitor_printf(mon, "Error %d while writing VM\n", ret); + error_propagate(errp, local_err); goto the_end; } @@ -2336,17 +2342,27 @@ void do_savevm(Monitor *mon, const QDict *qdict) if (bdrv_can_snapshot(bs1)) { /* 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, NULL); + ret = bdrv_snapshot_create(bs1, sn, &local_err); if (ret < 0) { - monitor_printf(mon, "Error while creating snapshot on '%s'\n", - bdrv_get_device_name(bs1)); + error_propagate(errp, local_err); } } } + info = g_malloc0(sizeof(SnapshotInfo)); + info->id = g_strdup(sn->id_str); + info->name = g_strdup(sn->name); + info->date_nsec = sn->date_nsec; + info->date_sec = sn->date_sec; + info->vm_state_size = vm_state_size; + info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000; + info->vm_clock_sec = sn->vm_clock_nsec / 1000000000; + the_end: if (saved_vm_running) vm_start(); + + return info; } void qmp_xen_save_devices_state(const char *filename, Error **errp)
QMP command "vm-snapshot-save" has also extra optional force parameter to specify whether replace existing snapshot or not. It also returns information about created snapshot. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- hmp-commands.hx | 4 ++-- hmp.c | 11 +++++++++ hmp.h | 1 + include/sysemu/sysemu.h | 1 - qapi-schema.json | 22 ++++++++++++++++++ qmp-commands.hx | 43 ++++++++++++++++++++++++++++++++++ savevm.c | 62 +++++++++++++++++++++++++++++++------------------ 7 files changed, 118 insertions(+), 26 deletions(-)