Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Eric Blake <eblake@redhat.com>]

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

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> This would be useful in the next step when QMP version of this call will
> be introduced.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  migration/savevm.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 

> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      uint64_t vm_state_size;
>      qemu_timeval tv;
>      struct tm tm;
> -    const char *name = qdict_get_try_str(qdict, "name");
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
>      if (!bdrv_all_can_snapshot(&bs)) {
> -        monitor_printf(mon, "Device '%s' is writable but does not "
> -                       "support snapshots.\n", bdrv_get_device_name(bs));
> +        error_setg(errp,
> +                   "Device '%s' is writable but does not support snapshots.",

No trailing '.' in error_setg() calls.

> +                   bdrv_get_device_name(bs));
>          return;
>      }
>  
>      /* Delete old snapshots of the same name */
>      if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> -        monitor_printf(mon,
> -                       "Error while deleting snapshot on device '%s': %s\n",
> -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));

Markus' series to add a prefixing notation would be better to use here
(although I didn't check if he caught this one in that series already):
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

>  
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +
> +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

I would have just written 'if (local_err) {'; but that's minor style.

Looks like a clean refactoring, other than the nit about the trailing
'.', so with that fixed:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]