Re: [Qemu-devel] [PATCH 02/12] savevm: Live migration handlers register the struct directly

[Orit Wasserman <owasserm@redhat.com>]

On 06/28/2012 10:22 PM, Juan Quintela wrote:
> Notice that the live migration users never unregister, so no problem
> about freeing the ops structure.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c       |    9 +++++++--
>  block-migration.c |   10 ++++++++--
>  migration.h       |    4 ++--
>  savevm.c          |   18 +++++++-----------
>  vl.c              |    3 +--
>  vmstate.h         |    5 +----
>  6 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 7ce074e..0b7e31f 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -298,7 +298,7 @@ static void migration_end(void)
> 
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> 
> -int ram_save_live(QEMUFile *f, int stage, void *opaque)
> +static int ram_save_live(QEMUFile *f, int stage, void *opaque)
>  {
>      ram_addr_t addr;
>      uint64_t bytes_transferred_last;
> @@ -436,7 +436,7 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      return NULL;
>  }
> 
> -int ram_load(QEMUFile *f, void *opaque, int version_id)
> +static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      ram_addr_t addr;
>      int flags, ret = 0;
> @@ -533,6 +533,11 @@ done:
>      return ret;
>  }
> 
> +SaveVMHandlers savevm_ram_handlers = {
> +    .save_live_state = ram_save_live,
> +    .load_state = ram_load,
> +};
> +
>  #ifdef HAS_AUDIO
>  struct soundhw {
>      const char *name;
> diff --git a/block-migration.c b/block-migration.c
> index b95b4e1..00151a0 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -709,11 +709,17 @@ static void block_set_params(const MigrationParams *params, void *opaque)
>      block_mig_state.blk_enable |= params->shared;
>  }
> 
> +SaveVMHandlers savevm_block_handlers = {
> +    .set_params = block_set_params,
> +    .save_live_state = block_save_live,
> +    .load_state = block_load,
> +};
> +
>  void blk_mig_init(void)
>  {
>      QSIMPLEQ_INIT(&block_mig_state.bmds_list);
>      QSIMPLEQ_INIT(&block_mig_state.blk_list);
> 
> -    register_savevm_live(NULL, "block", 0, 1, block_set_params,
> -                         block_save_live, NULL, block_load, &block_mig_state);
> +    register_savevm_live(NULL, "block", 0, 1, &savevm_block_handlers,
> +                         &block_mig_state);
>  }
> diff --git a/migration.h b/migration.h
> index 3990771..98bceda 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -18,6 +18,7 @@
>  #include "qemu-common.h"
>  #include "notify.h"
>  #include "error.h"
> +#include "vmstate.h"
> 
>  struct MigrationParams {
>      int blk;
> @@ -81,8 +82,7 @@ uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_transferred(void);
>  uint64_t ram_bytes_total(void);
> 
> -int ram_save_live(QEMUFile *f, int stage, void *opaque);
> -int ram_load(QEMUFile *f, void *opaque, int version_id);
> +extern SaveVMHandlers savevm_ram_handlers;
> 
>  /**
>   * @migrate_add_blocker - prevent migration from proceeding
> diff --git a/savevm.c b/savevm.c
> index 73626d4..a451be2 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1223,10 +1223,7 @@ int register_savevm_live(DeviceState *dev,
>                           const char *idstr,
>                           int instance_id,
>                           int version_id,
> -                         SaveSetParamsHandler *set_params,
> -                         SaveLiveStateHandler *save_live_state,
> -                         SaveStateHandler *save_state,
> -                         LoadStateHandler *load_state,
> +                         SaveVMHandlers *ops,
>                           void *opaque)
>  {
>      SaveStateEntry *se;
> @@ -1234,16 +1231,12 @@ int register_savevm_live(DeviceState *dev,
>      se = g_malloc0(sizeof(SaveStateEntry));
>      se->version_id = version_id;
>      se->section_id = global_section_id++;
> -    se->ops = g_malloc0(sizeof(SaveVMHandlers));
> -    se->ops->set_params = set_params;
> -    se->ops->save_live_state = save_live_state;
> -    se->ops->save_state = save_state;
> -    se->ops->load_state = load_state;
> +    se->ops = ops;
>      se->opaque = opaque;
>      se->vmsd = NULL;
>      se->no_migrate = 0;
>      /* if this is a live_savem then set is_ram */
> -    if (save_live_state != NULL) {
> +    if (ops->save_live_state != NULL) {
>          se->is_ram = 1;
>      }
> 
> @@ -1282,8 +1275,11 @@ int register_savevm(DeviceState *dev,
>                      LoadStateHandler *load_state,
>                      void *opaque)
>  {
> +    SaveVMHandlers *ops = g_malloc0(sizeof(SaveVMHandlers));
> +    ops->save_state = save_state;
> +    ops->load_state = load_state;
>      return register_savevm_live(dev, idstr, instance_id, version_id,
> -                                NULL, NULL, save_state, load_state, opaque);
> +                                ops, opaque);
>  }
> 
>  void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
> diff --git a/vl.c b/vl.c
> index 1329c30..b1f31e8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3438,8 +3438,7 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_sdcard, snapshot, machine->use_scsi,
>                    IF_SD, 0, SD_OPTS);
> 
> -    register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live, NULL,
> -                         ram_load, NULL);
> +    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
> 

Juan,
Can't we move it migration.c (migrate_init and qemu_start_incoming migration)?
this will remove to use an extern for savevm_ram_handlers.

Orit
>      if (nb_numa_nodes > 0) {
>          int i;
> diff --git a/vmstate.h b/vmstate.h
> index 909af69..4bce53b 100644
> --- a/vmstate.h
> +++ b/vmstate.h
> @@ -50,10 +50,7 @@ int register_savevm_live(DeviceState *dev,
>                           const char *idstr,
>                           int instance_id,
>                           int version_id,
> -                         SaveSetParamsHandler *set_params,
> -                         SaveLiveStateHandler *save_live_state,
> -                         SaveStateHandler *save_state,
> -                         LoadStateHandler *load_state,
> +                         SaveVMHandlers *ops,
>                           void *opaque);
> 
>  void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
>