Re: [Qemu-devel] [PATCH 9/9] migration: Add configuration section

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> It needs to be the first one and it is not optional, that is the reason
> why it is opencoded.  For new machine types, it is required than machine
> type name is the same in both sides.
> 
> It is just done right now for pc's.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/i386/pc_piix.c             |  1 +
>  hw/i386/pc_q35.c              |  1 +
>  include/migration/migration.h |  2 ++
>  savevm.c                      | 72 ++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 71 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5c04784..95806b3 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -314,6 +314,7 @@ static void pc_init_pci(MachineState *machine)
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      global_state_set_optional();
> +    savevm_skip_configuration();
>  }
> 
>  static void pc_compat_2_2(MachineState *machine)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index cc5827a..e32c040 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -293,6 +293,7 @@ static void pc_q35_init(MachineState *machine)
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      global_state_set_optional();
> +    savevm_skip_configuration();

It's a shame that these two functions, that do basically the same thing
(to two different pieces of data) have such different names.

>  }
> 
>  static void pc_compat_2_2(MachineState *machine)
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index f939d88..da89827 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -34,6 +34,7 @@
>  #define QEMU_VM_SECTION_FULL         0x04
>  #define QEMU_VM_SUBSECTION           0x05
>  #define QEMU_VM_VMDESCRIPTION        0x06
> +#define QEMU_VM_CONFIGURATION        0x07
> 
>  struct MigrationParams {
>      bool blk;
> @@ -184,4 +185,5 @@ void register_global_state(void);
>  void global_state_store(void);
>  char *global_state_get_runstate(void);
>  void global_state_set_optional(void);
> +void savevm_skip_configuration(void);
>  #endif
> diff --git a/savevm.c b/savevm.c
> index 2b4e554..ea149e7 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -238,11 +238,55 @@ typedef struct SaveStateEntry {
>  typedef struct SaveState {
>      QTAILQ_HEAD(, SaveStateEntry) handlers;
>      int global_section_id;
> +    bool skip_configuration;
> +    uint32_t len;
> +    char *name;
>  } SaveState;
> 
>  static SaveState savevm_state = {
>      .handlers = QTAILQ_HEAD_INITIALIZER(savevm_state.handlers),
>      .global_section_id = 0,
> +    .skip_configuration = false,
> +};
> +
> +void savevm_skip_configuration(void)
> +{
> +    savevm_state.skip_configuration = true;
> +}
> +
> +
> +static void configuration_pre_save(void *opaque)
> +{
> +    SaveState *state = opaque;
> +    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> +
> +    state->len = strlen(current_name);
> +    state->name = strdup(current_name);

Will that ever get freed?  If it never gets freed is it safe to
just make it 

   state->len = current_name;


> +
> +static int configuration_post_load(void *opaque, int version_id)
> +{
> +    SaveState *state = opaque;
> +    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> +
> +    if (strncmp(state->name, current_name, state->len) != 0) {
> +        error_report("Machine type received is '%s' and local is '%s'",
> +                     state->name, current_name);
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_configuration = {
> +    .name = "configuartion",

Typo! configuration

Dave

> +    .version_id = 1,
> +    .post_load = configuration_post_load,
> +    .pre_save = configuration_pre_save,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(len, SaveState),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, 0, len),
> +        VMSTATE_END_OF_LIST()
> +    },
>  };
> 
>  static void dump_vmstate_vmsd(FILE *out_file,
> @@ -625,7 +669,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
>                               const MigrationParams *params)
>  {
>      SaveStateEntry *se;
> -    int ret;
> +    int ret, len;
> 
>      trace_savevm_state_begin();
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> @@ -638,9 +682,13 @@ void qemu_savevm_state_begin(QEMUFile *f,
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>      qemu_put_be32(f, QEMU_VM_FILE_VERSION);
> 
> -    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> -        int len;
> +    /* We want this to be the 1st section on the migration stream */
> +    if (!savevm_state.skip_configuration) {
> +        qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> +        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
> +    }
> 
> +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_live_setup) {
>              continue;
>          }
> @@ -946,7 +994,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      Error *local_err = NULL;
>      uint8_t section_type;
>      unsigned int v;
> -    int ret;
> +    int ret, len;
>      int file_error_after_eof = -1;
> 
>      if (qemu_savevm_state_blocked(&local_err)) {
> @@ -970,11 +1018,25 @@ int qemu_loadvm_state(QEMUFile *f)
>          return -ENOTSUP;
>      }
> 
> +    /* if it exist, we want the configuration to be the 1st section
> +       and we require it to be there.  That is the reason why we don't
> +       read it on the while loop. */
> +    if (!savevm_state.skip_configuration) {
> +        if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
> +            error_report("Configuration section missing");
> +            return -EINVAL;
> +        }
> +        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> +
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
>      while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>          uint32_t instance_id, version_id, section_id;
>          SaveStateEntry *se;
>          char idstr[257];
> -        int len;
> 
>          trace_qemu_loadvm_state_section(section_type);
>          switch (section_type) {
> -- 
> 2.4.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK