| Message ID | 92c5c71b9467824fe0423debd5d62ad6c7321106.1357741229.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/09/2013 08:18 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > hmp-commands.hx | 4 ++-- > hmp.c | 9 +++++++++ > hmp.h | 1 + > include/sysemu/sysemu.h | 1 - > qapi-schema.json | 18 ++++++++++++++++++ > qmp-commands.hx | 29 +++++++++++++++++++++++++++++ > savevm.c | 27 ++++++++++++--------------- > 7 files changed, 71 insertions(+), 18 deletions(-) > For patches 1-6, you may add Reviewed-by: Eric Blake <eblake@redhat.com> However, for this patch, I think we may need to think more about the design. > +++ b/qapi-schema.json > @@ -3017,3 +3017,21 @@ > # Since: 1.3.0 > ## > { 'command': 'nbd-server-stop' } > + > +## > +# @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, it is replaced. > +# > +# The VM is automatically stopped and resumed and saving a snapshot can take > +# a long time. Since my initial review on your earlier RFC, I have changed my mind a bit and started thinking that merely exposing the existing HMP commands as raw QMP commands is gross. The HMP design is blocking - you can't abort the snapshot, and since it is long-running, you can't do anything else with the guest during that time, either. Of course, the guest is paused, but why does it have to be? Couldn't we enhance the snapshot code to use the same principles as live migration to take an internal snapshot without pausing the guest up front? This our chance to make life better by breaking the QMP interface into a sequence of commands, and merely rewriting the existing HMP command call the entire QMP sequence under the hood, rather than just exposing the same clunky one-command design in QMP. In other words, I think it would be nicer to have a job-based snapshot, where one command starts the snapshot, and then another command can query status to see if the snapshot is still underway, an event gets fired when the snapshot completes, and where yet another command can be used to cleanly abort a long-running snapshot if it turns out the snapshot is no longer needed because something else came up that must be dealt with now. In other words, I think this series and Wenchao Xia's series[1] need to coordinate on a common design. [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00645.html > +# > +# @name: #optional tag of new snapshot or tag|id of existing snapshot > +# > +# Returns: Nothing on success > +# > +# Since: 1.4.0 > +## > +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } But if we insist making the same clunky QMP interface, then this (plus the followup improvements in 13/13 for adding a force option) seems like a valid translation from HMP.
δΊ 2013-1-10 6:32, Eric Blake ει: > On 01/09/2013 08:18 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> hmp-commands.hx | 4 ++-- >> hmp.c | 9 +++++++++ >> hmp.h | 1 + >> include/sysemu/sysemu.h | 1 - >> qapi-schema.json | 18 ++++++++++++++++++ >> qmp-commands.hx | 29 +++++++++++++++++++++++++++++ >> savevm.c | 27 ++++++++++++--------------- >> 7 files changed, 71 insertions(+), 18 deletions(-) >> > > For patches 1-6, you may add Reviewed-by: Eric Blake <eblake@redhat.com> > However, for this patch, I think we may need to think more about the design. > >> +++ b/qapi-schema.json >> @@ -3017,3 +3017,21 @@ >> # Since: 1.3.0 >> ## >> { 'command': 'nbd-server-stop' } >> + >> +## >> +# @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, it is replaced. >> +# >> +# The VM is automatically stopped and resumed and saving a snapshot can take >> +# a long time. > > Since my initial review on your earlier RFC, I have changed my mind a > bit and started thinking that merely exposing the existing HMP commands > as raw QMP commands is gross. The HMP design is blocking - you can't > abort the snapshot, and since it is long-running, you can't do anything > else with the guest during that time, either. Of course, the guest is > paused, but why does it have to be? Couldn't we enhance the snapshot > code to use the same principles as live migration to take an internal > snapshot without pausing the guest up front? > > This our chance to make life better by breaking the QMP interface into a > sequence of commands, and merely rewriting the existing HMP command call > the entire QMP sequence under the hood, rather than just exposing the > same clunky one-command design in QMP. In other words, I think it would > be nicer to have a job-based snapshot, where one command starts the > snapshot, and then another command can query status to see if the > snapshot is still underway, an event gets fired when the snapshot > completes, and where yet another command can be used to cleanly abort a > long-running snapshot if it turns out the snapshot is no longer needed > because something else came up that must be dealt with now. > > In other words, I think this series and Wenchao Xia's series[1] need to > coordinate on a common design. > [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00645.html > I think the job based snapshot may comes later after vmstate live save is implemented, and now this interface is acceptable but mark it in name or comment that it is sync. >> +# >> +# @name: #optional tag of new snapshot or tag|id of existing snapshot >> +# >> +# Returns: Nothing on success >> +# >> +# Since: 1.4.0 >> +## >> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } > > But if we insist making the same clunky QMP interface, then this (plus > the followup improvements in 13/13 for adding a force option) seems like > a valid translation from HMP. >
diff --git a/hmp-commands.hx b/hmp-commands.hx index 010b8c9..3b781f7 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 9e9e624..2d8600a 100644 --- a/hmp.c +++ b/hmp.c @@ -1336,3 +1336,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict) qmp_nbd_server_stop(&errp); hmp_handle_error(mon, &errp); } + +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) +{ + const char *name = qdict_get_try_str(qdict, "name"); + Error *err = NULL; + + qmp_vm_snapshot_save(!!name, name, &err); + hmp_handle_error(mon, &err); +} diff --git a/hmp.h b/hmp.h index 21f3e05..e447f99 100644 --- a/hmp.h +++ b/hmp.h @@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict); void hmp_nbd_server_start(Monitor *mon, const QDict *qdict); void hmp_nbd_server_add(Monitor *mon, const QDict *qdict); void hmp_nbd_server_stop(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 d02d13f..d4db699 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); diff --git a/qapi-schema.json b/qapi-schema.json index 5dfa052..a6f4cb0 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3017,3 +3017,21 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-stop' } + +## +# @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, it is replaced. +# +# 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 +# +# Returns: Nothing on success +# +# Since: 1.4.0 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 5c692d0..9a0b566 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1348,6 +1348,35 @@ Example: EQMP { + .name = "vm-snapshot-save", + .args_type = "name:s?", + .params = "name", + .help = "save a VM snapshot. If no tag or id are provided, a new snapshot is created", + .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 or id, it is replaced. + +The VM is automatically stopped and resumed and saving a snapshot can take +a long time. + +Arguments: + +- "name": #optional tag of new snapshot or tag|id of existing snapshot (json-string, optional) + +Example: + +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } +<- { "return": {} } + +EQMP + { .name = "qmp_capabilities", .args_type = "", .params = "", diff --git a/savevm.c b/savevm.c index b705693..930ef65 100644 --- a/savevm.c +++ b/savevm.c @@ -2101,7 +2101,7 @@ static int del_existing_snapshots(const char *name, Error **errp) return 0; } -void do_savevm(Monitor *mon, const QDict *qdict) +void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; @@ -2116,7 +2116,6 @@ void do_savevm(Monitor *mon, const QDict *qdict) struct timeval tv; struct tm tm; #endif - 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 */ @@ -2128,15 +2127,15 @@ 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)); + error_setg(errp, "Device '%s' is writable but does not support " + "snapshots.", bdrv_get_device_name(bs)); return; } } bs = bdrv_snapshots(); if (!bs) { - monitor_printf(mon, "No block device can accept snapshots\n"); + error_setg(errp, "No block device can accept snapshots."); return; } @@ -2157,7 +2156,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) #endif 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); @@ -2178,23 +2177,22 @@ void do_savevm(Monitor *mon, const QDict *qdict) } /* 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); + if (has_name && del_existing_snapshots(name, &local_err) < 0) { + error_propagate(errp, 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; } @@ -2205,10 +2203,9 @@ 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); } } }
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- hmp-commands.hx | 4 ++-- hmp.c | 9 +++++++++ hmp.h | 1 + include/sysemu/sysemu.h | 1 - qapi-schema.json | 18 ++++++++++++++++++ qmp-commands.hx | 29 +++++++++++++++++++++++++++++ savevm.c | 27 ++++++++++++--------------- 7 files changed, 71 insertions(+), 18 deletions(-)