Re: [PATCH V7 04/29] migration: simplify savevm

[Dr. David Alan Gilbert]

* Steve Sistare (steven.sistare@oracle.com) wrote:
> Use qemu_file_open to simplify a few functions in savevm.c.
> No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

So I think this is mostly OK, but a couple of minor tidyups below;
so with the tidies and the renames from the previous patch:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0bef031..c71d525 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2910,8 +2910,9 @@ bool save_snapshot(const char *name, bool overwrite, 
> const char *vmstate,
>  void qmp_xen_save_devices_state(const char *filename, bool has_live, bool 
> live,
>                                  Error **errp)
>  {
> +    const char *ioc_name = "migration-xen-save-state";
> +    int flags = O_WRONLY | O_CREAT | O_TRUNC;

I don't see why to take these (or the matching ones in load) as separate
variables; just keep them as is, and be parameters.

>      QEMUFile *f;
> -    QIOChannelFile *ioc;
>      int saved_vm_running;
>      int ret;
>  
> @@ -2925,14 +2926,10 @@ void qmp_xen_save_devices_state(const char *filename, 
> bool has_live, bool live,
>      vm_stop(RUN_STATE_SAVE_VM);
>      global_state_store_running();
>  
> -    ioc = qio_channel_file_new_path(filename, O_WRONLY | O_CREAT | O_TRUNC,
> -                                    0660, errp);
> -    if (!ioc) {
> +    f = qemu_file_open(filename, flags, 0660, ioc_name, errp);
> +    if (!f) {
>          goto the_end;
>      }
> -    qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-save-state");
> -    f = qemu_fopen_channel_output(QIO_CHANNEL(ioc));
> -    object_unref(OBJECT(ioc));
>      ret = qemu_save_device_state(f);
>      if (ret < 0 || qemu_fclose(f) < 0) {
>          error_setg(errp, QERR_IO_ERROR);
> @@ -2960,8 +2957,8 @@ void qmp_xen_save_devices_state(const char *filename, 
> bool has_live, bool live,
>  
>  void qmp_xen_load_devices_state(const char *filename, Error **errp)
>  {
> +    const char *ioc_name = "migration-xen-load-state";
>      QEMUFile *f;
> -    QIOChannelFile *ioc;
>      int ret;
>  
>      /* Guest must be paused before loading the device state; the RAM state
> @@ -2973,14 +2970,10 @@ void qmp_xen_load_devices_state(const char *filename, 
> Error **errp)
>      }
>      vm_stop(RUN_STATE_RESTORE_VM);
>  
> -    ioc = qio_channel_file_new_path(filename, O_RDONLY | O_BINARY, 0, errp);
> -    if (!ioc) {
> +    f = qemu_file_open(filename, O_RDONLY | O_BINARY, 0, ioc_name, errp);
> +    if (!f) {
>          return;
>      }
> -    qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state");
> -    f = qemu_fopen_channel_input(QIO_CHANNEL(ioc));
> -    object_unref(OBJECT(ioc));
> -
>      ret = qemu_loadvm_state(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> -- 
> 1.8.3.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK