Re: [Qemu-devel] [PATCH 4/5] migration: improve error reporting for hmp_loadvm

[Markus Armbruster]

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

> The patch adds Error ** parameter to load_vmstate call and fills error
> inside. The caller after that properly reports error either through
> monitor or via local stderr facility during VM start.
>
> This helper will be usefull too for qmp_load_vmstate implementation.
>
> 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>
> ---
>  include/sysemu/sysemu.h |  2 +-
>  migration/savevm.c      | 25 +++++++++++++------------
>  monitor.c               |  7 ++++++-
>  vl.c                    |  5 ++++-
>  4 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 3bb8897..d9ccf45 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -78,7 +78,7 @@ void qemu_remove_exit_notifier(Notifier *notify);
>  void qemu_add_machine_init_done_notifier(Notifier *notify);
>  
>  void hmp_savevm(Monitor *mon, const QDict *qdict);
> -int load_vmstate(const char *name);
> +int load_vmstate(const char *name, Error **errp);
>  void hmp_delvm(Monitor *mon, const QDict *qdict);
>  void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 90b6850..08c6c65 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2042,7 +2042,7 @@ void qmp_xen_save_devices_state(const char *filename, 
> Error **errp)
>      }
>  }
>  
> -int load_vmstate(const char *name)
> +int load_vmstate(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs_vm_state;
>      QEMUSnapshotInfo sn;
> @@ -2051,20 +2051,20 @@ int load_vmstate(const char *name)
>      AioContext *aio_context;
>  
>      if (!bdrv_all_can_snapshot(&bs)) {
> -        error_report("Device '%s' is writable but does not support 
> snapshots.",
> -                     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.

I'd prefer breaking lines between arguments instead of in the middle of
an argument:

        error_setg(errp,
                   "Device '%s' is writable but does not support snapshots",
                   bdrv_get_device_name(bs));

I'll continue to review when I have something that applies.

[...]