Re: [Qemu-devel] [PATCH v3 07/11] qapi: Convert savevm

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]

On 03/28/2013 10:47 AM, Pavel Hrdina wrote:
> QMP command "vm-snapshot-save" has also extra optional force parameter
> and name parameter isn't optional anymore. It also returns information
> about created snapshot.

Commit message is a bit stale from an earlier version of the series,
since name is still optional in this patch.

> +++ b/qapi-schema.json
> @@ -3453,3 +3453,25 @@
>  # 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, the force argument needs to be true to replace it.
> +#
> +# 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
> +#
> +# @force: #optional specify whether existing snapshot is replaced or not,
> +#         default is false
> +#
> +# Returns: Nothing on success

Comment is stale, now that you return the SnapshotInfo of the
newly-created snapshot.

> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'},
> +  'returns': 'SnapshotInfo' }

> +++ b/savevm.c
> @@ -2249,17 +2249,18 @@ static int del_existing_snapshots(const char *name, Error **errp)
>      return 0;
>  }
>  
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name,
> +                                  bool has_force, bool force, Error **errp)

Indentation is off by one (the two "bool has_" should be aligned).

> @@ -2271,16 +2272,16 @@ 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));
> -            return;
> +            error_setg(errp, "Device '%s' is writable but does not support "
> +                       "snapshots.", bdrv_get_device_name(bs));

Missed a removal of a trailing '.' in the error message.

> +            return NULL;
>          }
>      }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> -        return;
> +        error_setg(errp, "No block device can accept snapshots.");

And completely added a trailing '.' here.

> +            if (has_force && force) {
> +                pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> +                pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> +
> +                /* Delete old snapshots of the same name */
> +                if (del_existing_snapshots(name, &local_err) < 0) {
> +                    error_propagate(errp, local_err);
> +                    goto the_end;
> +                }
> +            } else {
> +                error_setg(errp, "Snapshot '%s' exists. For override use "
> +                           "'force' parameter.", name);

New message with trailing '.'; maybe:

"cannot override existing snapshot '%s' without 'force' parameter"


>      /* 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));

don't add a trailing '.'

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]