| Message ID | f369a988af44f54f7cfd7868af29d9b27ec72d75.1345016001.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/15/2012 01:41 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- I'm focusing my review more on the public interface (since that's what affects libvirt), and therefore glanced through 1 through 12 but did not pay close attention to them. > hmp-commands.hx | 2 +- > hmp.c | 10 ++++++++++ > hmp.h | 1 + > qapi-schema.json | 19 +++++++++++++++++++ > qmp-commands.hx | 29 +++++++++++++++++++++++++++++ > savevm.c | 25 +++++++++---------------- > sysemu.h | 1 - > 7 files changed, 69 insertions(+), 18 deletions(-) > > + > +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) > +{ > + const char *name = qdict_get_try_str(qdict, "name"); In the cover letter, you said "and for QMP you have to always provide name parameter" - but this says 'name' is optional and can still be empty (id only). > +++ b/qapi-schema.json > @@ -2356,3 +2356,22 @@ > # Since: 1.2.0 > ## > { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] } > + > +## > +# @vm-snapshot-save: > +# > +# Create a snapshot of the whole virtual machine. If 'tag' is provided, 'tag' is a misnomer, since you list @name as the parameter 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: tag or id of new or existing snapshot Here, you document @name as required, > +# > +# Returns: Nothing on success > +# If an error occurs, GenericError with error message > +# > +# Since: 1.2 We missed 1.2 hard freeze. This better be 1.3. > +## > +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } but here, you listed it as '*name' meaning optional. I'm okay with leaving name optional, provided that it means that you end up allocating the next unique id. But if that's the case, then returning nothing is wrong; this command needs to return { 'id':'int', '*name':'str' }, so that the user knows what snapshot just got allocated. > +SQMP > +vm-snapshot-save > +------ > + > +Create a snapshot of the whole virtual machine. If '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. > + > +The VM is automatically stopped and resumed and saving a snapshot can take > +a long time. I don't like that this command can take a long time. Just today on #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled. If we're going to create a new interface, it would be nicer to create a command that starts the save process and returns immediately, as well as commands to track progress, allow an early abort, and send an event on completion. The HMP 'savevm' interface can issue multiple QMP commands under the hood to continue with it's blocking behavior, but as long as we are fixing things, I think the QMP interface should be more powerful. > + > +Arguments: > + > +- "name": tag or id of new or existing snapshot > + > +Example: > + > +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } > +<- { "return": {} } Again, you need to return the id of the just-created snapshot. > @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) > } > > /* Delete old snapshots of the same name */ > - if (name && del_existing_snapshots(name, NULL) < 0) { > + if (has_name && del_existing_snapshots(name, errp) < 0) { Here's hoping later in the series updates this to make saner decisions. (Maybe I should peruse the entire series before commenting on individual patches?)
On 08/15/2012 09:49 PM, Eric Blake wrote: > On 08/15/2012 01:41 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >> --- > I'm focusing my review more on the public interface (since that's what > affects libvirt), and therefore glanced through 1 through 12 but did not > pay close attention to them. > >> hmp-commands.hx | 2 +- >> hmp.c | 10 ++++++++++ >> hmp.h | 1 + >> qapi-schema.json | 19 +++++++++++++++++++ >> qmp-commands.hx | 29 +++++++++++++++++++++++++++++ >> savevm.c | 25 +++++++++---------------- >> sysemu.h | 1 - >> 7 files changed, 69 insertions(+), 18 deletions(-) >> >> + >> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict) >> +{ >> + const char *name = qdict_get_try_str(qdict, "name"); > In the cover letter, you said "and for QMP you have to always provide > name parameter" - but this says 'name' is optional and can still be > empty (id only). I said in cover letter that the last two patches introduce this functionality. >> +++ b/qapi-schema.json >> @@ -2356,3 +2356,22 @@ >> # Since: 1.2.0 >> ## >> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] } >> + >> +## >> +# @vm-snapshot-save: >> +# >> +# Create a snapshot of the whole virtual machine. If 'tag' is provided, > 'tag' is a misnomer, since you list @name as the parameter 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: tag or id of new or existing snapshot > Here, you document @name as required, > >> +# >> +# Returns: Nothing on success >> +# If an error occurs, GenericError with error message >> +# >> +# Since: 1.2 > We missed 1.2 hard freeze. This better be 1.3. > >> +## >> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } > but here, you listed it as '*name' meaning optional. I'm okay with > leaving name optional, provided that it means that you end up allocating > the next unique id. But if that's the case, then returning nothing is > wrong; this command needs to return { 'id':'int', '*name':'str' }, so > that the user knows what snapshot just got allocated. If you apply whole patch-series, then you have to for QMP always provide @name. As I said above, this behaviour is introduced by last two patches. >> +SQMP >> +vm-snapshot-save >> +------ >> + >> +Create a snapshot of the whole virtual machine. If '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. >> + >> +The VM is automatically stopped and resumed and saving a snapshot can take >> +a long time. > I don't like that this command can take a long time. Just today on > #virt IRC, someone was complaining that 'savevm' HMP cannot be canceled. > If we're going to create a new interface, it would be nicer to create a > command that starts the save process and returns immediately, as well as > commands to track progress, allow an early abort, and send an event on > completion. The HMP 'savevm' interface can issue multiple QMP commands > under the hood to continue with it's blocking behavior, but as long as > we are fixing things, I think the QMP interface should be more powerful. Well, I could try to do it...with this I'll probably introduce new command vm-snapshot-save-cancel or vm-snaphsot-cancel as we have migrate and migrate-cancel. >> + >> +Arguments: >> + >> +- "name": tag or id of new or existing snapshot >> + >> +Example: >> + >> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } } >> +<- { "return": {} } > Again, you need to return the id of the just-created snapshot. > >> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> } >> >> /* Delete old snapshots of the same name */ >> - if (name && del_existing_snapshots(name, NULL) < 0) { >> + if (has_name && del_existing_snapshots(name, errp) < 0) { > Here's hoping later in the series updates this to make saner decisions. > (Maybe I should peruse the entire series before commenting on > individual patches?) >
diff --git a/hmp-commands.hx b/hmp-commands.hx index f6104b0..32476d1 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -267,7 +267,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 diff --git a/hmp.c b/hmp.c index a9d5675..9f791e8 100644 --- a/hmp.c +++ b/hmp.c @@ -1102,3 +1102,13 @@ void hmp_closefd(Monitor *mon, const QDict *qdict) qmp_closefd(fdname, &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 7dd93bf..0ce1021 100644 --- a/hmp.h +++ b/hmp.h @@ -71,5 +71,6 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); void hmp_getfd(Monitor *mon, const QDict *qdict); void hmp_closefd(Monitor *mon, const QDict *qdict); +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict); #endif diff --git a/qapi-schema.json b/qapi-schema.json index 53bbe46..5c086f9 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2356,3 +2356,22 @@ # Since: 1.2.0 ## { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] } + +## +# @vm-snapshot-save: +# +# Create a snapshot of the whole virtual machine. If '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. +# +# The VM is automatically stopped and resumed and saving a snapshot can take +# a long time. +# +# @name: tag or id of new or existing snapshot +# +# Returns: Nothing on success +# If an error occurs, GenericError with error message +# +# Since: 1.2 +## +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 527b9f7..ac7c4c9 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1111,6 +1111,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, +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": tag or id of new or existing snapshot + +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 500eb72..42e496f 100644 --- a/savevm.c +++ b/savevm.c @@ -2100,7 +2100,7 @@ static int del_existing_snapshots(const char *name, 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; @@ -2115,7 +2115,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"); /* Verify if there is a device that doesn't support snapshots and is writable */ bs = NULL; @@ -2126,15 +2125,14 @@ 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_set(errp, QERR_NOT_SUPPORTED); return; } } bs = bdrv_snapshots(); if (!bs) { - monitor_printf(mon, "No block device can accept snapshots\n"); + error_set(errp, QERR_NOT_SUPPORTED); return; } @@ -2155,7 +2153,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, NULL); if (ret >= 0) { pstrcpy(sn->name, sizeof(sn->name), old_sn->name); @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict) } /* Delete old snapshots of the same name */ - if (name && del_existing_snapshots(name, NULL) < 0) { + if (has_name && del_existing_snapshots(name, errp) < 0) { 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_set(errp, QERR_OPEN_FILE_FAILED, bdrv_get_device_name(bs)); goto the_end; } - ret = qemu_savevm_state(f, NULL); + ret = qemu_savevm_state(f, errp); vm_state_size = qemu_ftell(f); qemu_fclose(f); - if (ret < 0) { - monitor_printf(mon, "Error %d while writing VM\n", ret); + if (error_is_set(errp)) { goto the_end; } @@ -2201,11 +2198,7 @@ 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); - if (ret < 0) { - monitor_printf(mon, "Error while creating snapshot on '%s'\n", - bdrv_get_device_name(bs1)); - } + bdrv_snapshot_create(bs1, sn, errp); } } diff --git a/sysemu.h b/sysemu.h index 651d96e..d1ceade 100644 --- a/sysemu.h +++ b/sysemu.h @@ -69,7 +69,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);
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- hmp-commands.hx | 2 +- hmp.c | 10 ++++++++++ hmp.h | 1 + qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 29 +++++++++++++++++++++++++++++ savevm.c | 25 +++++++++---------------- sysemu.h | 1 - 7 files changed, 69 insertions(+), 18 deletions(-)