Re: [Qemu-devel] [PATCH 7/9] global_state: Make section optional

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> This section would be sent:
>> 
>> a- for all new machine types
>> b- for old achine types if section state is different form {running,paused}
>>    that were the only giving us troubles.
>> 
>> So, in new qemus: it is alwasy there.  In old qemus: they are only
>> there if it an error has happened, basically stoping on target.
>
> I worry about whether we should do that for old machine types;
> in the current code, if the destination was started with -S  it wouldn't
> start in the case you were worried about, couldn't a careful managment
> layer spot-the state on the source and ensure it didn't start the destination?
>
> (What happens with guests that have been shutdown during a migrate, or panic
> or something - not that I'm sure what happens now).

Old guest:
If we start with -S, destination end in pause.  If there has been an
error during migration, management have to notice it.

Guest hasn't been started with -S, there is one error during migration
on source, but migration happens to end correctly.  Destination will try
to continue and will not notice the error on source.  Crash if we are
lucky, Disk corruption if we aren't.

So, for old guests, if the end state of the source is different of
paused/running, we just break migration, it is the safer thing to do,
antyhing else is just guessing.

>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/i386/pc_piix.c             |  2 ++
>>  hw/i386/pc_q35.c              |  2 ++
>>  include/migration/migration.h |  2 +-
>>  migration/migration.c         | 29 +++++++++++++++++++++++++++++
>>  4 files changed, 34 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 212e263..5c04784 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -52,6 +52,7 @@
>>  #ifdef CONFIG_XEN
>>  #  include <xen/hvm/hvm_info_table.h>
>>  #endif
>> +#include "migration/migration.h"
>> 
>>  #define MAX_IDE_BUS 2
>> 
>> @@ -312,6 +313,7 @@ static void pc_init_pci(MachineState *machine)
>> 
>>  static void pc_compat_2_3(MachineState *machine)
>>  {
>> +    global_state_set_optional();
>>  }
>> 
>>  static void pc_compat_2_2(MachineState *machine)
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index e67f2de..cc5827a 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -45,6 +45,7 @@
>>  #include "hw/usb.h"
>>  #include "hw/cpu/icc_bus.h"
>>  #include "qemu/error-report.h"
>> +#include "migration/migration.h"
>> 
>>  /* ICH9 AHCI has 6 ports */
>>  #define MAX_SATA_PORTS     6
>> @@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
>> 
>>  static void pc_compat_2_3(MachineState *machine)
>>  {
>> +    global_state_set_optional();
>>  }
>> 
>>  static void pc_compat_2_2(MachineState *machine)
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 785c2dc..f939d88 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -183,5 +183,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>>  void register_global_state(void);
>>  void global_state_store(void);
>>  char *global_state_get_runstate(void);
>> -
>> +void global_state_set_optional(void);
>>  #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index ab69f81..1035689 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -868,6 +868,7 @@ void migrate_fd_connect(MigrationState *s)
>>  }
>> 
>>  typedef struct {
>> +    bool optional;
>>      int32_t size;
>>      uint8_t runstate[100];
>>  } GlobalState;
>> @@ -888,6 +889,33 @@ char *global_state_get_runstate(void)
>>      return (char *)global_state.runstate;
>>  }
>> 
>> +void global_state_set_optional(void)
>> +{
>> +    global_state.optional = true;
>> +}
>
>
> It's a shame that it's not really a device class, because then
> you could have used a DEFINE_PROP_BIT.

Yeap.

>
>> +static bool global_state_needed(void *opaque)
>> +{
>> +    GlobalState *s = opaque;
>> +    char *runstate = (char *)s->runstate;
>> +
>> +    /* If it is not optional, it is mandatory */
>> +
>> +    if (s->optional == false) {
>> +        return true;
>> +    }
>> +
>> +    /* If state is running or paused, it is not needed */
>> +
>> +    if (strcmp(runstate, "running") == 0 ||
>> +        strcmp(runstate, "paused") == 0) {
>> +        return false;
>> +    }
>> +
>> +    /* for any other state it is needed */
>> +    return true;
>> +}
>> +
>>  static int global_state_post_load(void *opaque, int version_id)
>>  {
>>      GlobalState *s = opaque;
>> @@ -921,6 +949,7 @@ static const VMStateDescription vmstate_globalstate = {
>>      .minimum_version_id = 1,
>>      .post_load = global_state_post_load,
>>      .pre_save = global_state_pre_save,
>> +    .needed = global_state_needed,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_INT32(size, GlobalState),
>>          VMSTATE_BUFFER(runstate, GlobalState),
>> -- 
>> 2.4.0
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK