| Message ID | ac7209d0c25123bc6e583d2e941bf2d09dea648f.1363957855.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/22/2013 07:16 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(-) > > +++ b/qapi-schema.json > @@ -3442,3 +3442,21 @@ > # 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, it is replaced. Any reason we have to fix this up later in the series, instead of getting the interface right to begin with (in regards to having an optional force argument)? > +# > +# The VM is automatically stopped and resumed and saving a snapshot can take > +# a long time. Transactionally, are we exposing the right building blocks? While the existing HMP command pauses the domain up front, I think the QMP interface should be job-oriented. That is, it should be possible to start a long-running job and have QMP return immediately, so that QMP remains responsive, and lets us do a live migrate, live bandwidth tuning, the ability to gracefully abort, and the ability to pause the domain if live migrate isn't converging fast enough. HMP would then preserve its existing interface by calling multiple QMP commands, instead of trying to make QMP an exact analogue to the sub-par HMP interface. Libvirt _really_ wants to be able to cancel an in-progress snapshot creation action, but can't if all we expose is the same interface as HMP has always had. > +# @name: #optional tag of new snapshot or tag|id of existing snapshot > +# > +# Returns: Nothing on success Bad. If @name is optional, and one is generated on your behalf, then you need to return the 'name' that was generated. Also, even if 'name' is provided, knowing which 'id' was allocated is useful, since later APIs can operate on 'name' or 'id'. > +# > +# Since: 1.5 > +## > +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } In other words, this needs a return value. > 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."); Odd that we weren't consistent on whether errors ended with '.' when using monitor_printf; your patch at least tried to be self-consistent, even if opposite the normal usage of error_setg() :)
On 26.3.2013 20:56, Eric Blake wrote: > On 03/22/2013 07:16 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(-) >> > >> +++ b/qapi-schema.json >> @@ -3442,3 +3442,21 @@ >> # 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, it is replaced. > > Any reason we have to fix this up later in the series, instead of > getting the interface right to begin with (in regards to having an > optional force argument)? It was supposed to introduce new feature for both, but I can make it as you suggesting. > >> +# >> +# The VM is automatically stopped and resumed and saving a snapshot can take >> +# a long time. > > Transactionally, are we exposing the right building blocks? While the > existing HMP command pauses the domain up front, I think the QMP > interface should be job-oriented. That is, it should be possible to > start a long-running job and have QMP return immediately, so that QMP > remains responsive, and lets us do a live migrate, live bandwidth > tuning, the ability to gracefully abort, and the ability to pause the > domain if live migrate isn't converging fast enough. HMP would then > preserve its existing interface by calling multiple QMP commands, > instead of trying to make QMP an exact analogue to the sub-par HMP > interface. > > Libvirt _really_ wants to be able to cancel an in-progress snapshot > creation action, but can't if all we expose is the same interface as HMP > has always had. > I'm working on live snapshots which will introduce this functionality, but I couldn't give you any deadline when I'll have it done. So if you (libvirt) really want to have savevm and vm-snapshot-save non-blocking before I'll finish live snapshots, then I could rewrite this patch. I know that current approach isn't good enough because of that blocking behavior. >> +# @name: #optional tag of new snapshot or tag|id of existing snapshot >> +# >> +# Returns: Nothing on success > > Bad. If @name is optional, and one is generated on your behalf, then > you need to return the 'name' that was generated. Also, even if 'name' > is provided, knowing which 'id' was allocated is useful, since later > APIs can operate on 'name' or 'id'. That's a good point. I'll fix this to return all information about the created snapshot. > >> +# >> +# Since: 1.5 >> +## >> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } > > In other words, this needs a return value. > > >> 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."); > > Odd that we weren't consistent on whether errors ended with '.' when > using monitor_printf; your patch at least tried to be self-consistent, > even if opposite the normal usage of error_setg() :) > Thanks for review. I've fixed error messages and other issues you mentioned in other patches. Pavel
diff --git a/hmp-commands.hx b/hmp-commands.hx index df44906..3c1cb05 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 b0a861c..914d88c 100644 --- a/hmp.c +++ b/hmp.c @@ -1423,3 +1423,12 @@ 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; + + qmp_vm_snapshot_save(!!name, name, &err); + hmp_handle_error(mon, &err); +} diff --git a/hmp.h b/hmp.h index 95fe76e..d5c61fa 100644 --- a/hmp.h +++ b/hmp.h @@ -85,5 +85,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 fdaa9da..87842d3 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3442,3 +3442,21 @@ # 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, 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.5 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index b370060..0f5e544 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1423,6 +1423,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 71981d1..45d1b09 100644 --- a/savevm.c +++ b/savevm.c @@ -2135,7 +2135,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; @@ -2145,7 +2145,6 @@ void do_savevm(Monitor *mon, const QDict *qdict) 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 */ @@ -2157,15 +2156,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; } @@ -2180,7 +2179,7 @@ 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); @@ -2195,23 +2194,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; } @@ -2222,10 +2220,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(-)