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

[Markus Armbruster]

"Denis V. Lunev" <address@hidden> writes:

> This would be useful in the next step when QMP version of this call will
> be introduced.
>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Juan Quintela <address@hidden>
> CC: Amit Shah <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Eric Blake <address@hidden>
> ---
>  migration/savevm.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 2c65ab2..f83ffd0 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> +static void do_savevm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1915,28 +1915,26 @@ 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.", bdrv_get_device_name(bs));

Since you're touching this already, please drop the period and clean up
the line wrapping:

       error_setg(errp,
                  "Device '%s' is writable but does not support snapshots",
                  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));
>          error_free(local_err);
>          return;
>      }
>  
>      bs = bdrv_all_find_vmstate_bs();
>      if (bs == NULL) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> +        error_setg(errp, "No block device can accept snapshots");
>          return;
>      }
>      aio_context = bdrv_get_aio_context(bs);
> @@ -1945,7 +1943,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>  
>      ret = global_state_store();
>      if (ret) {
> -        monitor_printf(mon, "Error saving global state\n");
> +        error_setg(errp, "Error saving global state");
>          return;
>      }
>      vm_stop(RUN_STATE_SAVE_VM);
> @@ -1977,22 +1975,20 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      /* 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, "Could not open VM state file");
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(f, &local_err);
> +    ret = qemu_savevm_state(f, errp);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> -        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> -        error_free(local_err);
>          goto the_end;
>      }
>  
>      ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
>      if (ret < 0) {
> -        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> -                       bdrv_get_device_name(bs));
> +        error_setg(errp, "Error while creating snapshot on '%s'",
> +                   bdrv_get_device_name(bs));
>      }
>  
>   the_end:
> @@ -2002,6 +1998,18 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +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) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> +        error_free(local_err);

Easier:

        error_report_err(local_err);

Already used elsewhere in this file.

> +    }
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>  {
>      QEMUFile *f;