Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-delete no takes two parameters name and id. They are
> optional, but one of the name or id must be provided. If both are provided
> they
> will match only the snapshot with the same name and id. The command returns
> SnapshotInfo only if the snapshot exists, if snapshot doesn't exist it returns
> nothing. If something goes wrong, it returns an error message.
>
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
> different. The delvm takes one optional flag -i and one parameter name. If you
> provide only the name parameter, it will match only snapshots with that name.
> If you also provide the flag, it will match only snapshots with name as id.
> Information about successfully deleted snapshot will be printed, if there is
> no
> snapshot with that name or id, the appropriate message will be printed. If
> something goes wrong, an error message will be printed.
>
> These improves behavior of the command to be more strict on selecting
> snapshots
> because actual behavior is wrong. Now if you want to delete snapshot with
> name '2'
> but there is no snapshot with that name it could delete snapshot with id '2'
> and
> that isn't what you want.
>
> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
> ---
> hmp-commands.hx | 14 ++++++++------
> hmp.c | 35 ++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/sysemu.h | 1 -
> qapi-schema.json | 17 +++++++++++++++++
> qmp-commands.hx | 50
> ++++++++++++++++++++++++++++++++++++++++++++++++-
> savevm.c | 49 +++++++++++++++++++++++++++++++++++-------------
> 7 files changed, 146 insertions(+), 21 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index df44906..d1701ce 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -339,16 +339,18 @@ ETEXI
>
> {
> .name = "delvm",
> - .args_type = "name:s",
> - .params = "tag|id",
> - .help = "delete a VM snapshot from its tag or id",
> - .mhandler.cmd = do_delvm,
> + .args_type = "id:-i,name:s",
> + .params = "[-i] tag|[id]",
This is a weird syntax description. I think I'd write it like this:
"tag|-i id"
> + .help = "delete a VM snapshot from its tag or id if -i flag is
> provided",
> + .mhandler.cmd = hmp_vm_snapshot_delete,
> },
>
> STEXI
> -@item delvm @var{tag}|@var{id}
> +@item delvm [-i] @var{tag}|[@var{id}]
> @findex delvm
> -Delete the snapshot identified by @var{tag} or @var{id}.
> +Delete a snapshot identified by @var{tag}. If flag -i is provided, delete
> +a snapshot indentified by @var{id}.
> +
> ETEXI
>
> {
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..2c754b3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1425,3 +1425,38 @@ 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_delete(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;
> +
> + if (id) {
> + info = qmp_vm_snapshot_delete(false, NULL, true, name, &local_err);
> + } else {
> + info = qmp_vm_snapshot_delete(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, "Deleted snapshot 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);
> + }
Why isn't an error set when trying to delete a snapshot that doesn't
exist?
> +
> + qapi_free_SnapshotInfo(info);
> + hmp_handle_error(mon, &local_err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..b0667b3 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_delete(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6578782..f46f9d2 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -67,7 +67,6 @@ 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);
>
> void qemu_announce_self(void);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..641f3ac 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3505,3 +3505,20 @@
> '*asl_compiler_rev': 'uint32',
> '*file': 'str',
> '*data': 'str' }}
> +
> +##
> +# @vm-snapshot-delete:
> +#
> +# Delete a snapshot identified by name or id or both. One of the name or id
> +# is required. It will returns SnapshotInfo of successfully deleted snapshot.
> +#
> +# @name: tag of an existing snapshot
> +#
> +# @id: id of an existing snapshot
> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
> + 'returns': 'SnapshotInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..86f399d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1419,7 +1419,55 @@ Example:
>
> -> { "execute": "add_client", "arguments": { "protocol": "vnc",
> "fdname": "myclient" } }
> -<- { "return": {} }
> +<- {
> + "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 = "vm-snapshot-delete",
> + .args_type = "name:s?,id:s?",
> + .params = "[tag] [id]",
> + .help = "delete a VM snapshot from its tag or id",
> + .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_delete
> + },
> +
> +SQMP
> +vm-snapshot-delete
> +------
> +
> +Delete a snapshot identified by name or id or both. One of the name or id
> +is required. It will returns SnapshotInfo of successfully deleted snapshot.
> +
> +Arguments:
> +
> +- "name": tag of an existing snapshot (json-string, optional)
> +
> +- "id": id of an existing snapshot (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-delete", "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
> {
> diff --git a/savevm.c b/savevm.c
> index 96a2340..4af7d2d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2473,28 +2473,51 @@ int load_vmstate(const char *name)
> return 0;
> }
>
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> + const bool has_id, const char *id,
> + Error **errp)
> {
> - BlockDriverState *bs, *bs1;
> - Error *local_err = NULL;
> - const char *name = qdict_get_str(qdict, "name");
> + BlockDriverState *bs;
> + SnapshotInfo *info = NULL;
> + QEMUSnapshotInfo sn;
> +
> + if (!has_name && !has_id) {
> + error_setg(errp, "name or id must be provided");
> + return NULL;
> + }
>
> bs = bdrv_snapshots();
> if (!bs) {
> - monitor_printf(mon, "No block device supports snapshots\n");
> - return;
> + error_setg(errp, "no block device supports snapshots");
Please leave a capital N at the start of the message.
> + return NULL;
> }
>
> - bs1 = NULL;
> - while ((bs1 = bdrv_next(bs1))) {
> - if (bdrv_can_snapshot(bs1)) {
> - bdrv_snapshot_delete(bs1, name, &local_err);
> - if (error_is_set(&local_err)) {
> - monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> - error_free(local_err);
> + if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
> + /* no need to set an error if snapshot doesn't exist */
> + return NULL;
> + }
Is it even allowed in QMP that you return NULL without setting an error?
What would the JSON response look like?
> +
> + 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;
> +
> + bs = NULL;
> + while ((bs = bdrv_next(bs))) {
> + if (bdrv_can_snapshot(bs)
> + && bdrv_snapshot_find(bs, &sn, name, id, false)) {
> + bdrv_snapshot_delete(bs, sn.name, errp);
> + if (error_is_set(errp)) {
You need to use a local_err, the caller could have passed a NULL errp,
and then you wouldn't detect the error here.
> + return NULL;
> }
> }
> }
> +
> + return info;
> }
Kevin
