Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm command

[Eric Blake <eblake@redhat.com>]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> The patch also moves hmp_delvm implementation into hmp.c. This function
> is just a simple wrapper now and does not have knowledge about
> migration internals.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hmp.c              | 11 +++++++++++
>  migration/savevm.c | 16 +++++++---------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>  4 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index c9c7100..a342c8c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2390,3 +2390,14 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>          error_report_err(local_err);
>      }
>  }
> +
> +void hmp_delvm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;

Same comment as earlier, about 'err' vs. 'local_err' naming,

> +
> +    qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

and redundant comparison to NULL.

> +++ b/migration/savevm.c
> @@ -2099,17 +2099,15 @@ int load_vmstate(const char *name)
>      return 0;
>  }
>  
> -void hmp_delvm(Monitor *mon, const QDict *qdict)
> +void qmp_delvm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs;
> -    Error *err;
> -    const char *name = qdict_get_str(qdict, "name");
> -
> -    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        monitor_printf(mon,
> -                       "Error while deleting snapshot on device '%s': %s\n",
> -                       bdrv_get_device_name(bs), error_get_pretty(err));
> -        error_free(err);
> +    Error *local_err = NULL;

Instead of renaming 'err' to 'local_err',

> +
> +    if (bdrv_all_delete_snapshot(name, &bs, &local_err) < 0) {
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs), error_get_pretty(local_err));

you'll want to use Markus' proposed error prefixing code.


> +SQMP
> +delvm
> +------------------

Divider too long.

-- 
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: 604 bytes --]