| Message ID | 889e6a3afd112f0037dd25308d821b54ffa77ad0.1366127809.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > QMP command vm-snapshot-load and HMP command loadvm behave similar to > vm-snapshot-delete and delvm. The only different is that they will load > the snapshot instead of deleting it. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > hmp-commands.hx | 16 +++++----- > hmp.c | 35 ++++++++++++++++++++++ > hmp.h | 1 + > include/sysemu/sysemu.h | 1 - > monitor.c | 12 -------- > qapi-schema.json | 18 +++++++++++ > qmp-commands.hx | 38 ++++++++++++++++++++++++ > savevm.c | 79 ++++++++++++++++++++++++++++++------------------- > vl.c | 7 ++++- > 9 files changed, 156 insertions(+), 51 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index d1701ce..e80410b 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -324,17 +324,19 @@ ETEXI > > { > .name = "loadvm", > - .args_type = "name:s", > - .params = "tag|id", > - .help = "restore a VM snapshot from its tag or id", > - .mhandler.cmd = do_loadvm, > + .args_type = "id:-i,name:s", > + .params = "[-i] tag|[id]", > + .help = "restore a VM snapshot from its tag or id if -i flag is provided", Same comments as 4/11 about possibilities of making this look nicer ('name' seems nicer than 'tag|[id]'). > } > + > +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict) > +{ > + const char *name = qdict_get_try_str(qdict, "name"); > + const bool id = qdict_get_try_bool(qdict, "id", false); > + Error *local_err = NULL; > + SnapshotInfo *info = NULL; > + > + if (id) { > + info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err); > + } else { > + info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err); > + } Could be written without if/else: qmp_vm_snapshot_load(id, name, !id, name, &local_err); but doesn't really bother me as written. > + > + if (info) { > + char buf[256]; Fixed-size buffer... > + QEMUSnapshotInfo sn = { > + .vm_state_size = info->vm_state_size, > + .date_sec = info->date_sec, > + .date_nsec = info->date_nsec, > + .vm_clock_nsec = info->vm_clock_sec * 1000000000 + > + info->vm_clock_nsec, > + }; > + pstrcpy(sn.id_str, sizeof(sn.id_str), info->id); > + pstrcpy(sn.name, sizeof(sn.name), info->name); > + monitor_printf(mon, "Loaded snapshot's info:\n"); > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn)); ...but potentially large amount of information to be placed in it. I would MUCH rather see this code using gstring, or even direct printing. I think you need to base this on top of the ideas here: https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02540.html These two series need to be coordinated to minimize churn and rebase conflicts on cleanups such as removing buffer truncation from bdrv_snapshot_dump. Hmm, I probably should have mentioned this on 4/11 as well. > + } else if (!error_is_set(&local_err)) { Same comments as 4/11, in that info == NULL should imply that local_err is set. > +++ b/qapi-schema.json > @@ -3522,3 +3522,21 @@ > ## > { 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'}, > 'returns': 'SnapshotInfo' } > + > +## > +# @vm-snapshot-load: > +# > +# Set the whole virtual machine to the snapshot identified by the tag > +# or the unique snapshot id or both. It must be a snapshot of a whole VM and > +# at least one of the name or id parameter must be specified. > +# > +# @name: tag of existing snapshot > +# > +# @id: id of existing snapshot As in 4/11, mark these with #optional. > +# > +# Returns: SnapshotInfo on success > +# > +# Since: 1.5 > +## > +{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'}, > + 'returns': 'SnapshotInfo' } Same comments as 4/11 on return value design decisions. > @@ -2393,26 +2393,35 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp) > vm_start(); > } > > -int load_vmstate(const char *name) > +SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, > + bool has_id, const char *id, Error **errp) > { > BlockDriverState *bs, *bs_vm_state; > QEMUSnapshotInfo sn; > QEMUFile *f; > - Error *local_err; > + SnapshotInfo *info = NULL; > + int saved_vm_running; This should be bool. runstate_is_running() is currently typed as int, but it defers to runstate_check() which is bool; fixing runstate_is* to consistently use bool would be worthy of a separate cleanup patch. > + > + if (!has_name && !has_id) { > + error_setg(errp, "name or id must be specified"); > + return NULL; > + } > > bs_vm_state = bdrv_snapshots(); > if (!bs_vm_state) { > - error_report("No block device supports snapshots"); > - return -ENOTSUP; > + error_setg(errp, "no block device supports snapshots"); > + return NULL; > } > > /* Don't even try to load empty VM states */ > - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) { > - return -ENOENT; > - } else if (sn.vm_state_size == 0) { > - error_report("This is a disk-only snapshot. Revert to it offline " > - "using qemu-img."); > - return -EINVAL; > + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { Oh, I didn't notice this before, but 4/11 has the same issue. You are not guaranteed that 'name' and 'id' are initialized correctly unless you first check 'has_name' and 'has_id'. And given my suggestion above of possibly compressing things to: qmp_vm_snapshot_load(id, name, !id, name, &local_err); then it REALLY matters that you pass NULL for the parameters that are not provided by the caller. To be safe, you must use one of: if (!has_name) { name = NULL; } if (!has_id) { id = NULL; } ... bdrv_snapshot_find(bs_vm_state, &sn, name, id, false) ... or: bdrv_snapshot_find(bs_vm_state, &sn, has_name ? name : NULL, has_id ? id : NULL, false) unless you take on the bigger task of ensuring that ALL callers pass in NULL when has_name is false (and the code generator for QMP currently doesn't guarantee that). > + return NULL; > + } > + > + if (sn.vm_state_size == 0) { > + error_setg(errp, "this is a disk-only snapshot, revert to it offline " > + "using qemu-img"); Indentation is off; a continuation should line up to the character after the '(' of the line before. > + return NULL; > } > > /* Verify if there is any device that doesn't support snapshots and is > @@ -2425,29 +2434,28 @@ int load_vmstate(const char *name) > } > > if (!bdrv_can_snapshot(bs)) { > - error_report("Device '%s' is writable but does not support snapshots.", > - bdrv_get_device_name(bs)); > - return -ENOTSUP; > + error_setg(errp, "device '%s' is writable but does not support " > + "snapshots", bdrv_get_device_name(bs)); > + return NULL; > } Pre-existing, but why do we care? Loading a snapshot doesn't require writing, just reading. > > - if (!bdrv_snapshot_find(bs, &sn, name, name, true)) { > - error_report("Device '%s' does not have the requested snapshot '%s'", > - bdrv_get_device_name(bs), name); > - return -ENOENT; > + if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { > + return NULL; Missing error report. > } > } > > + saved_vm_running = runstate_is_running(); > + vm_stop(RUN_STATE_RESTORE_VM); > + > /* Flush all IO requests so they don't interfere with the new state. */ > bdrv_drain_all(); > > bs = NULL; > while ((bs = bdrv_next(bs))) { > if (bdrv_can_snapshot(bs)) { > - bdrv_snapshot_goto(bs, name, &local_err); > - if (error_is_set(&local_err)) { > - error_report("%s", error_get_pretty(local_err)); > - error_free(local_err); > - return -1; > + bdrv_snapshot_goto(bs, sn.name, errp); > + if (error_is_set(errp)) { > + return NULL; Pre-existing, but yet another case of partial failure, where it might be nicer to let the caller know if we failed halfway through loading a located snapshot, differently than complete failure where we didn't change any state.
On 17.4.2013 01:43, Eric Blake wrote: > On 04/16/2013 10:05 AM, Pavel Hrdina wrote: >> QMP command vm-snapshot-load and HMP command loadvm behave similar to >> vm-snapshot-delete and delvm. The only different is that they will load >> the snapshot instead of deleting it. >> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> hmp-commands.hx | 16 +++++----- >> hmp.c | 35 ++++++++++++++++++++++ >> hmp.h | 1 + >> include/sysemu/sysemu.h | 1 - >> monitor.c | 12 -------- >> qapi-schema.json | 18 +++++++++++ >> qmp-commands.hx | 38 ++++++++++++++++++++++++ >> savevm.c | 79 ++++++++++++++++++++++++++++++------------------- >> vl.c | 7 ++++- >> 9 files changed, 156 insertions(+), 51 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index d1701ce..e80410b 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -324,17 +324,19 @@ ETEXI >> >> { >> .name = "loadvm", >> - .args_type = "name:s", >> - .params = "tag|id", >> - .help = "restore a VM snapshot from its tag or id", >> - .mhandler.cmd = do_loadvm, >> + .args_type = "id:-i,name:s", >> + .params = "[-i] tag|[id]", >> + .help = "restore a VM snapshot from its tag or id if -i flag is provided", > > Same comments as 4/11 about possibilities of making this look nicer > ('name' seems nicer than 'tag|[id]'). > >> } >> + >> +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict) >> +{ >> + const char *name = qdict_get_try_str(qdict, "name"); >> + const bool id = qdict_get_try_bool(qdict, "id", false); >> + Error *local_err = NULL; >> + SnapshotInfo *info = NULL; >> + >> + if (id) { >> + info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err); >> + } else { >> + info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err); >> + } > > Could be written without if/else: > qmp_vm_snapshot_load(id, name, !id, name, &local_err); > > but doesn't really bother me as written. > >> + >> + if (info) { >> + char buf[256]; > > Fixed-size buffer... I'll leave it there for now and Wenchao could update this in his patch series. > >> + QEMUSnapshotInfo sn = { >> + .vm_state_size = info->vm_state_size, >> + .date_sec = info->date_sec, >> + .date_nsec = info->date_nsec, >> + .vm_clock_nsec = info->vm_clock_sec * 1000000000 + >> + info->vm_clock_nsec, >> + }; >> + pstrcpy(sn.id_str, sizeof(sn.id_str), info->id); >> + pstrcpy(sn.name, sizeof(sn.name), info->name); >> + monitor_printf(mon, "Loaded snapshot's info:\n"); >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn)); > > ...but potentially large amount of information to be placed in it. I > would MUCH rather see this code using gstring, or even direct printing. > I think you need to base this on top of the ideas here: > > https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02540.html > > These two series need to be coordinated to minimize churn and rebase > conflicts on cleanups such as removing buffer truncation from > bdrv_snapshot_dump. > > Hmm, I probably should have mentioned this on 4/11 as well. > >> + } else if (!error_is_set(&local_err)) { > > Same comments as 4/11, in that info == NULL should imply that local_err > is set. > >> +++ b/qapi-schema.json >> @@ -3522,3 +3522,21 @@ >> ## >> { 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'}, >> 'returns': 'SnapshotInfo' } >> + >> +## >> +# @vm-snapshot-load: >> +# >> +# Set the whole virtual machine to the snapshot identified by the tag >> +# or the unique snapshot id or both. It must be a snapshot of a whole VM and >> +# at least one of the name or id parameter must be specified. >> +# >> +# @name: tag of existing snapshot >> +# >> +# @id: id of existing snapshot > > As in 4/11, mark these with #optional. > >> +# >> +# Returns: SnapshotInfo on success >> +# >> +# Since: 1.5 >> +## >> +{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'}, >> + 'returns': 'SnapshotInfo' } > > Same comments as 4/11 on return value design decisions. > >> @@ -2393,26 +2393,35 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp) >> vm_start(); >> } >> >> -int load_vmstate(const char *name) >> +SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >> + bool has_id, const char *id, Error **errp) >> { >> BlockDriverState *bs, *bs_vm_state; >> QEMUSnapshotInfo sn; >> QEMUFile *f; >> - Error *local_err; >> + SnapshotInfo *info = NULL; >> + int saved_vm_running; > > This should be bool. runstate_is_running() is currently typed as int, > but it defers to runstate_check() which is bool; fixing runstate_is* to > consistently use bool would be worthy of a separate cleanup patch. > >> + >> + if (!has_name && !has_id) { >> + error_setg(errp, "name or id must be specified"); >> + return NULL; >> + } >> >> bs_vm_state = bdrv_snapshots(); >> if (!bs_vm_state) { >> - error_report("No block device supports snapshots"); >> - return -ENOTSUP; >> + error_setg(errp, "no block device supports snapshots"); >> + return NULL; >> } >> >> /* Don't even try to load empty VM states */ >> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) { >> - return -ENOENT; >> - } else if (sn.vm_state_size == 0) { >> - error_report("This is a disk-only snapshot. Revert to it offline " >> - "using qemu-img."); >> - return -EINVAL; >> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { > > Oh, I didn't notice this before, but 4/11 has the same issue. You are > not guaranteed that 'name' and 'id' are initialized correctly unless you > first check 'has_name' and 'has_id'. And given my suggestion above of > possibly compressing things to: > > qmp_vm_snapshot_load(id, name, !id, name, &local_err); > > then it REALLY matters that you pass NULL for the parameters that are > not provided by the caller. To be safe, you must use one of: > > if (!has_name) { > name = NULL; > } > if (!has_id) { > id = NULL; > } > ... bdrv_snapshot_find(bs_vm_state, &sn, name, id, false) ... > Thanks for pointing this out, I didn't realize that. > or: > > bdrv_snapshot_find(bs_vm_state, &sn, > has_name ? name : NULL, > has_id ? id : NULL, > false) > > unless you take on the bigger task of ensuring that ALL callers pass in > NULL when has_name is false (and the code generator for QMP currently > doesn't guarantee that). > >> + return NULL; >> + } >> + >> + if (sn.vm_state_size == 0) { >> + error_setg(errp, "this is a disk-only snapshot, revert to it offline " >> + "using qemu-img"); > > Indentation is off; a continuation should line up to the character after > the '(' of the line before. > >> + return NULL; >> } >> >> /* Verify if there is any device that doesn't support snapshots and is >> @@ -2425,29 +2434,28 @@ int load_vmstate(const char *name) >> } >> >> if (!bdrv_can_snapshot(bs)) { >> - error_report("Device '%s' is writable but does not support snapshots.", >> - bdrv_get_device_name(bs)); >> - return -ENOTSUP; >> + error_setg(errp, "device '%s' is writable but does not support " >> + "snapshots", bdrv_get_device_name(bs)); >> + return NULL; >> } > > Pre-existing, but why do we care? Loading a snapshot doesn't require > writing, just reading. For now I'll leave it there. This basically check that there is for example some raw disk and therefore the snapshot state wouldn't be loaded for all block devices. We anyway need to improve savevm, loadvm and delvm to do a better checking and as you said behave transactionally. This also applies for most of your comments about functionality and it should be fixed in another patch series. > >> >> - if (!bdrv_snapshot_find(bs, &sn, name, name, true)) { >> - error_report("Device '%s' does not have the requested snapshot '%s'", >> - bdrv_get_device_name(bs), name); >> - return -ENOENT; >> + if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >> + return NULL; > > Missing error report. > >> } >> } >> >> + saved_vm_running = runstate_is_running(); >> + vm_stop(RUN_STATE_RESTORE_VM); >> + >> /* Flush all IO requests so they don't interfere with the new state. */ >> bdrv_drain_all(); >> >> bs = NULL; >> while ((bs = bdrv_next(bs))) { >> if (bdrv_can_snapshot(bs)) { >> - bdrv_snapshot_goto(bs, name, &local_err); >> - if (error_is_set(&local_err)) { >> - error_report("%s", error_get_pretty(local_err)); >> - error_free(local_err); >> - return -1; >> + bdrv_snapshot_goto(bs, sn.name, errp); >> + if (error_is_set(errp)) { >> + return NULL; > > Pre-existing, but yet another case of partial failure, where it might be > nicer to let the caller know if we failed halfway through loading a > located snapshot, differently than complete failure where we didn't > change any state. >
diff --git a/hmp-commands.hx b/hmp-commands.hx index d1701ce..e80410b 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -324,17 +324,19 @@ ETEXI { .name = "loadvm", - .args_type = "name:s", - .params = "tag|id", - .help = "restore a VM snapshot from its tag or id", - .mhandler.cmd = do_loadvm, + .args_type = "id:-i,name:s", + .params = "[-i] tag|[id]", + .help = "restore a VM snapshot from its tag or id if -i flag is provided", + .mhandler.cmd = hmp_vm_snapshot_load, }, STEXI -@item loadvm @var{tag}|@var{id} +@item loadvm [-i] @var{tag}|[@var{id}] @findex loadvm -Set the whole virtual machine to the snapshot identified by the tag -@var{tag} or the unique snapshot ID @var{id}. +Set the whole virtual machine to the snapshot identified by the tag @var{tag}. +If flag -i is provided, snapshot identified by @var{id} will be loaded. It must +be a snapshot of a whole VM. + ETEXI { diff --git a/hmp.c b/hmp.c index 2c754b3..516bb09 100644 --- a/hmp.c +++ b/hmp.c @@ -1460,3 +1460,38 @@ void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict) qapi_free_SnapshotInfo(info); hmp_handle_error(mon, &local_err); } + +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict) +{ + const char *name = qdict_get_try_str(qdict, "name"); + const bool id = qdict_get_try_bool(qdict, "id", false); + Error *local_err = NULL; + SnapshotInfo *info = NULL; + + if (id) { + info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err); + } else { + info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err); + } + + if (info) { + char buf[256]; + QEMUSnapshotInfo sn = { + .vm_state_size = info->vm_state_size, + .date_sec = info->date_sec, + .date_nsec = info->date_nsec, + .vm_clock_nsec = info->vm_clock_sec * 1000000000 + + info->vm_clock_nsec, + }; + pstrcpy(sn.id_str, sizeof(sn.id_str), info->id); + pstrcpy(sn.name, sizeof(sn.name), info->name); + monitor_printf(mon, "Loaded snapshot's info:\n"); + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn)); + } else if (!error_is_set(&local_err)) { + monitor_printf(mon, "Snapshot '%s' not found.\n", name); + } + + qapi_free_SnapshotInfo(info); + hmp_handle_error(mon, &local_err); +} diff --git a/hmp.h b/hmp.h index b0667b3..a65cbf2 100644 --- a/hmp.h +++ b/hmp.h @@ -86,5 +86,6 @@ 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_delete(Monitor *mon, const QDict *qdict); +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict); #endif diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 70c6927..913f49f 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -66,7 +66,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_info_snapshots(Monitor *mon, const QDict *qdict); void qemu_announce_self(void); diff --git a/monitor.c b/monitor.c index 332abe7..02c20ef 100644 --- a/monitor.c +++ b/monitor.c @@ -2093,18 +2093,6 @@ void qmp_closefd(const char *fdname, Error **errp) error_set(errp, QERR_FD_NOT_FOUND, fdname); } -static void do_loadvm(Monitor *mon, const QDict *qdict) -{ - int saved_vm_running = runstate_is_running(); - const char *name = qdict_get_str(qdict, "name"); - - vm_stop(RUN_STATE_RESTORE_VM); - - if (load_vmstate(name) == 0 && saved_vm_running) { - vm_start(); - } -} - int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 641f3ac..d19b410 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3522,3 +3522,21 @@ ## { 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'}, 'returns': 'SnapshotInfo' } + +## +# @vm-snapshot-load: +# +# Set the whole virtual machine to the snapshot identified by the tag +# or the unique snapshot id or both. It must be a snapshot of a whole VM and +# at least one of the name or id parameter must be specified. +# +# @name: tag of existing snapshot +# +# @id: id of existing snapshot +# +# Returns: SnapshotInfo on success +# +# Since: 1.5 +## +{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'}, + 'returns': 'SnapshotInfo' } diff --git a/qmp-commands.hx b/qmp-commands.hx index 86f399d..f2d5da8 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1471,6 +1471,44 @@ Example: EQMP { + .name = "vm-snapshot-load", + .args_type = "name:s?,id:s?", + .params = "[name] [id]", + .help = "restore a VM snapshot from its tag or id", + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_load + }, + +SQMP +vm-snapshot-load +------ + +Set the whole virtual machine to the snapshot identified by the tag +or the unique snapshot id or both. It must be a snapshot of a whole VM and +at least one of the name or id parameter must be specified. + +Arguments: + +- "name": tag of existing snapshot (json-string, optional) + +- "id": id of existing snapshot (json-string, optional) + +Example: + +-> { "execute": "vm-snapshot-load", "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 0419e0d..1258f5f 100644 --- a/savevm.c +++ b/savevm.c @@ -2393,26 +2393,35 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp) vm_start(); } -int load_vmstate(const char *name) +SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, + bool has_id, const char *id, Error **errp) { BlockDriverState *bs, *bs_vm_state; QEMUSnapshotInfo sn; QEMUFile *f; - Error *local_err; + SnapshotInfo *info = NULL; + int saved_vm_running; + + if (!has_name && !has_id) { + error_setg(errp, "name or id must be specified"); + return NULL; + } bs_vm_state = bdrv_snapshots(); if (!bs_vm_state) { - error_report("No block device supports snapshots"); - return -ENOTSUP; + error_setg(errp, "no block device supports snapshots"); + return NULL; } /* Don't even try to load empty VM states */ - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) { - return -ENOENT; - } else if (sn.vm_state_size == 0) { - error_report("This is a disk-only snapshot. Revert to it offline " - "using qemu-img."); - return -EINVAL; + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { + return NULL; + } + + if (sn.vm_state_size == 0) { + error_setg(errp, "this is a disk-only snapshot, revert to it offline " + "using qemu-img"); + return NULL; } /* Verify if there is any device that doesn't support snapshots and is @@ -2425,29 +2434,28 @@ int load_vmstate(const char *name) } if (!bdrv_can_snapshot(bs)) { - error_report("Device '%s' is writable but does not support snapshots.", - bdrv_get_device_name(bs)); - return -ENOTSUP; + error_setg(errp, "device '%s' is writable but does not support " + "snapshots", bdrv_get_device_name(bs)); + return NULL; } - if (!bdrv_snapshot_find(bs, &sn, name, name, true)) { - error_report("Device '%s' does not have the requested snapshot '%s'", - bdrv_get_device_name(bs), name); - return -ENOENT; + if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { + return NULL; } } + saved_vm_running = runstate_is_running(); + vm_stop(RUN_STATE_RESTORE_VM); + /* Flush all IO requests so they don't interfere with the new state. */ bdrv_drain_all(); bs = NULL; while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs)) { - bdrv_snapshot_goto(bs, name, &local_err); - if (error_is_set(&local_err)) { - error_report("%s", error_get_pretty(local_err)); - error_free(local_err); - return -1; + bdrv_snapshot_goto(bs, sn.name, errp); + if (error_is_set(errp)) { + return NULL; } } } @@ -2455,21 +2463,32 @@ int load_vmstate(const char *name) /* restore the VM state */ f = qemu_fopen_bdrv(bs_vm_state, 0); if (!f) { - error_report("Could not open VM state file"); - return -EINVAL; + error_setg(errp, "could not open VM state file"); + return NULL; } qemu_system_reset(VMRESET_SILENT); - qemu_loadvm_state(f, &local_err); + qemu_loadvm_state(f, errp); qemu_fclose(f); - if (error_is_set(&local_err)) { - error_report("%s", error_get_pretty(local_err)); - error_free(local_err); - return -1; + if (error_is_set(errp)) { + return NULL; } - return 0; + if (saved_vm_running) { + vm_start(); + } + + 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 = sn.vm_state_size; + info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000; + info->vm_clock_sec = sn.vm_clock_nsec / 1000000000; + + return info; } SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, diff --git a/vl.c b/vl.c index 0598998..b0a5f01 100644 --- a/vl.c +++ b/vl.c @@ -4409,8 +4409,13 @@ int main(int argc, char **argv, char **envp) qemu_system_reset(VMRESET_SILENT); if (loadvm) { - if (load_vmstate(loadvm) < 0) { + Error *errp = NULL; + qmp_vm_snapshot_load(true, loadvm, false, NULL, &errp); + + if (error_is_set(&errp)) { autostart = 0; + error_report("%s", error_get_pretty(errp)); + error_free(errp); } }
QMP command vm-snapshot-load and HMP command loadvm behave similar to vm-snapshot-delete and delvm. The only different is that they will load the snapshot instead of deleting it. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- hmp-commands.hx | 16 +++++----- hmp.c | 35 ++++++++++++++++++++++ hmp.h | 1 + include/sysemu/sysemu.h | 1 - monitor.c | 12 -------- qapi-schema.json | 18 +++++++++++ qmp-commands.hx | 38 ++++++++++++++++++++++++ savevm.c | 79 ++++++++++++++++++++++++++++++------------------- vl.c | 7 ++++- 9 files changed, 156 insertions(+), 51 deletions(-)