Re: [Qemu-devel] [PATCH 1/8] migration: split hmp_savevm to migrate_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. The patch also moves snapshot name generation to the
> hmp specific code as QMP version of this code will require the name
> on the protocol level.
>
> Addition of migration_savevm to migration/migration.h is temporary. It
> will be removed in the next patch with QMP level change.
>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Juan Quintela <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: Amit Shah <address@hidden>
> CC: Markus Armbruster <address@hidden>
> ---
>  hmp.c                         | 26 ++++++++++++++++++++++++
>  include/migration/migration.h |  3 +++
>  migration/savevm.c            | 46 
> +++++++++++++++++--------------------------
>  3 files changed, 47 insertions(+), 28 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index c2b2c16..e7bab75 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -18,6 +18,7 @@
>  #include "net/eth.h"
>  #include "sysemu/char.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/sysemu.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
>  #include "qmp-commands.h"
> @@ -32,6 +33,7 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "migration/migration.h"
>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2386,3 +2388,27 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const 
> QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    char name_buf[64];
> +
> +    if (name == NULL) {

I'd prefer !name.

> +        qemu_timeval tv;
> +        struct tm tm;
> +
> +        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> +        localtime_r((const time_t *)&tv.tv_sec, &tm);

I'm afraid this uses tv.tv_sec uninitialized.  Not sure how this
survived testing :)

Aside: the ugly cast comes from commit d7d9b52.  It works when the
system conforms to POSIX (tv_sec is time_t), or when tv_sec's type is
essentially the same as time_t (supposedly the case in old OpenBSD).  As
far as I can tell, OpenBSD was fixed in 2013.

> +        strftime(name_buf, sizeof(name_buf), "vm-%Y%m%d%H%M%S", &tm);
> +
> +        name = name_buf;
> +    }
> +
> +    migrate_savevm(name, &local_err);
> +
> +    if (local_err != NULL) {
> +        error_report_err(local_err);
> +    }
> +}
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index d9494b8..73c8bb1 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -277,6 +277,8 @@ int migrate_compress_threads(void);
>  int migrate_decompress_threads(void);
>  bool migrate_use_events(void);
>  
> +void migrate_savevm(const char *name, Error **errp);
> +
>  /* Sending on the return path - generic and then for each message type */
>  void migrate_send_rp_message(MigrationIncomingState *mis,
>                               enum mig_rp_message_type message_type,
> @@ -321,4 +323,5 @@ int ram_save_queue_pages(MigrationState *ms, const char 
> *rbname,
>  PostcopyState postcopy_state_get(void);
>  /* Set the state and return the old state */
>  PostcopyState postcopy_state_set(PostcopyState new_state);
> +
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0ad1b93..308302a 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)
> +void migrate_savevm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1914,29 +1914,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      int saved_vm_running;
>      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));
>          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));
> +    if (bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));

Before the patch, we delete snapshots only when name comes from the
user, not when we made it up.

After the patch, we delete always.

What happens before the patch when we make up a name, a snapshot with
this name exists, but we don't delete it?

Should deleting old snapshots move to the HMP command as well?

>          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);
> @@ -1960,39 +1958,31 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      sn->date_nsec = tv.tv_usec * 1000;
>      sn->vm_clock_nsec = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  
> -    if (name) {
> -        ret = bdrv_snapshot_find(bs, old_sn, name);
> -        if (ret >= 0) {
> -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> -        } else {
> -            pstrcpy(sn->name, sizeof(sn->name), name);
> -        }
> +    ret = bdrv_snapshot_find(bs, old_sn, name);
> +    if (ret >= 0) {
> +        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> +        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>      } else {
> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> -        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> +        pstrcpy(sn->name, sizeof(sn->name), name);
>      }

Another place where we now use the code for "name from user" for made-up
name as well.  Not sure about the impact.

>  
>      /* 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: