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

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) 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 <address@hidden>
>> ---
>>  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.

Code is similar, but meaning is different.

1st one: it is sent only if needed
2nd one: it is completely skiped.

Calling the second: configuration_optional() looks weeird, when the
meaning is NO_configuration.

We could rename the 1st one to global_state_skip_if_not_needed, but not
sure that it would be better :-(

I am open to name changes if you provide better names O:-)

>
>>  }
>> 
>>  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;

No, changed.

>
>
>> +
>> +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

Thanks.