[Qemu-devel] [PATCH 00/11] Migraiton events + optional sections

[Qemu-devel] [PATCH 00/11] Migraiton events + optional sections

[Juan Quintela]

Hi

As the beggining of both series have been accepted, I merged both here.  Changes:
- answered all comments on list and applied suggested changes
- Use migrate_set_state() consistently
- we were misusing atomic_cmpxchg(), it didn't matter until now because
  we were missing only traces, but it showed with events missing.
- Reorganized how the migration events are generated, now only on migrate_set_state.

Please review.


Juan Quintela (11):
  runstate: Add runstate store
  runstate: migration allows more transitions now
  migration: create new section to store global state
  global_state: Make section optional
  vmstate: Create optional sections
  migration: Add configuration section
  migration: Use cmpxchg correctly
  migration: Use always helper to set state
  migration: No need to call trace_migrate_set_state()
  migration: create migration event
  migration: Add migration events on target side

 docs/qmp/qmp-events.txt       |  14 +++++
 hw/i386/pc_piix.c             |   2 +
 hw/i386/pc_q35.c              |   2 +
 include/migration/migration.h |   4 ++
 include/migration/vmstate.h   |   2 +
 include/sysemu/sysemu.h       |   1 +
 migration/migration.c         | 138 +++++++++++++++++++++++++++++++++++++++---
 migration/savevm.c            |  69 +++++++++++++++++++++
 migration/vmstate.c           |  11 ++++
 qapi/event.json               |  14 +++++
 trace-events                  |   1 +
 vl.c                          |  15 +++++
 12 files changed, 263 insertions(+), 10 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 01/11] runstate: Add runstate store

[Juan Quintela]

This allows us to store the current state to send it through migration.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/sysemu/sysemu.h |  1 +
 vl.c                    | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 0304aa7..d7bec42 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -28,6 +28,7 @@ bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
 bool runstate_needs_reset(void);
+bool runstate_store(char *str, size_t size);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, RunState state);

diff --git a/vl.c b/vl.c
index 2201e27..c659a00 100644
--- a/vl.c
+++ b/vl.c
@@ -630,6 +630,18 @@ bool runstate_check(RunState state)
     return current_run_state == state;
 }

+bool runstate_store(char *str, size_t size)
+{
+    const char *state = RunState_lookup[current_run_state];
+    size_t len = strlen(state) + 1;
+
+    if (len > size) {
+        return false;
+    }
+    memcpy(str, state, len);
+    return true;
+}
+
 static void runstate_init(void)
 {
     const RunStateTransition *p;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 01/11] runstate: Add runstate store

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> This allows us to store the current state to send it through migration.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/sysemu/sysemu.h |  1 +
>  vl.c                    | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 0304aa7..d7bec42 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -28,6 +28,7 @@ bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
>  int runstate_is_running(void);
>  bool runstate_needs_reset(void);
> +bool runstate_store(char *str, size_t size);
>  typedef struct vm_change_state_entry VMChangeStateEntry;
>  typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
> 
> diff --git a/vl.c b/vl.c
> index 2201e27..c659a00 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -630,6 +630,18 @@ bool runstate_check(RunState state)
>      return current_run_state == state;
>  }
> 
> +bool runstate_store(char *str, size_t size)
> +{
> +    const char *state = RunState_lookup[current_run_state];
> +    size_t len = strlen(state) + 1;
> +
> +    if (len > size) {
> +        return false;
> +    }
> +    memcpy(str, state, len);
> +    return true;
> +}
> +

It seems a lot of work for a status string; but OK.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  static void runstate_init(void)
>  {
>      const RunStateTransition *p;
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 02/11] runstate: migration allows more transitions now

[Juan Quintela]

Next commit would allow to move from incoming migration to error happening on source.

Should we add more states to this transition?  Luiz?

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vl.c b/vl.c
index c659a00..555fd88 100644
--- a/vl.c
+++ b/vl.c
@@ -571,6 +571,8 @@ static const RunStateTransition runstate_transitions_def[] = {

     { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
+    { RUN_STATE_INMIGRATE, RUN_STATE_INTERNAL_ERROR },
+    { RUN_STATE_INMIGRATE, RUN_STATE_IO_ERROR },

     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
     { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
-- 
2.4.3

Re: [Qemu-devel] [PATCH 02/11] runstate: migration allows more transitions now

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Next commit would allow to move from incoming migration to error happening on source.
> 
> Should we add more states to this transition?  Luiz?
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  vl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index c659a00..555fd88 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -571,6 +571,8 @@ static const RunStateTransition runstate_transitions_def[] = {
> 
>      { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
> +    { RUN_STATE_INMIGRATE, RUN_STATE_INTERNAL_ERROR },
> +    { RUN_STATE_INMIGRATE, RUN_STATE_IO_ERROR },
> 
>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
>      { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 03/11] migration: create new section to store global state

[Juan Quintela]

This includes a new section that for now just stores the current qemu state.

Right now, there are only one way to control what is the state of the
target after migration.

- If you run the target qemu with -S, it would start stopped.
- If you run the target qemu without -S, it would run just after migration finishes.

The problem here is what happens if we start the target without -S and
there happens one error during migration that puts current state as
-EIO.  Migration would ends (notice that the error happend doing block
IO, network IO, i.e. nothing related with migration), and when
migration finish, we would just "continue" running on destination,
probably hanging the guest/corruption data, whatever.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h |  1 +
 migration/migration.c         | 93 +++++++++++++++++++++++++++++++++++++++++--
 vl.c                          |  1 +
 3 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 9387c8c..1280193 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,

 void ram_mig_init(void);
 void savevm_skip_section_footers(void);
+void register_global_state(void);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index b04b457..01bb90d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -25,6 +25,7 @@
 #include "qemu/thread.h"
 #include "qmp-commands.h"
 #include "trace.h"
+#include "qapi/util.h"

 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */

@@ -96,6 +97,81 @@ void migration_incoming_state_destroy(void)
     mis_current = NULL;
 }

+
+typedef struct {
+    int32_t size;
+    uint8_t runstate[100];
+} GlobalState;
+
+static GlobalState global_state;
+
+static void global_state_store(void)
+{
+    if (!runstate_store((char *)global_state.runstate,
+                        sizeof(global_state.runstate))) {
+        printf("Runstate is too big\n");
+        exit(-1);
+    }
+}
+
+static char *global_state_get_runstate(void)
+{
+    return (char *)global_state.runstate;
+}
+
+static int global_state_post_load(void *opaque, int version_id)
+{
+    GlobalState *s = opaque;
+    int ret = 0;
+    char *runstate = (char *)s->runstate;
+
+    printf("loaded state: %s\n", runstate);
+
+    if (strcmp(runstate, "running") != 0) {
+        Error *local_err = NULL;
+        int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
+                                -1, &local_err);
+
+        if (r == -1) {
+            if (local_err) {
+                error_report_err(local_err);
+            }
+            return -1;
+        }
+        ret = vm_stop_force_state(r);
+    }
+
+   return ret;
+}
+
+static void global_state_pre_save(void *opaque)
+{
+    GlobalState *s = opaque;
+
+    s->size = strlen((char *)s->runstate) + 1;
+    printf("saved state: %s\n", s->runstate);
+}
+
+static const VMStateDescription vmstate_globalstate = {
+    .name = "globalstate",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = global_state_post_load,
+    .pre_save = global_state_pre_save,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(size, GlobalState),
+        VMSTATE_BUFFER(runstate, GlobalState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+void register_global_state(void)
+{
+    /* We would use it independently that we receive it */
+    strcpy((char *)&global_state.runstate, "");
+    vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
+}
+
 /*
  * Called on -incoming with a defer: uri.
  * The migration can be started later after any parameters have been
@@ -163,10 +239,20 @@ static void process_incoming_migration_co(void *opaque)
         exit(EXIT_FAILURE);
     }

-    if (autostart) {
+    /* runstate == "" means that we haven't received it through the
+     * wire, so we obey autostart.  runstate == runing means that we
+     * need to run it, we need to make sure that we do it after
+     * everything else has finished.  Every other state change is done
+     * at the post_load function */
+
+    if (strcmp(global_state_get_runstate(), "running") == 0) {
         vm_start();
-    } else {
-        runstate_set(RUN_STATE_PAUSED);
+    } else if (strcmp(global_state_get_runstate(), "") == 0) {
+        if (autostart) {
+            vm_start();
+        } else {
+            runstate_set(RUN_STATE_PAUSED);
+        }
     }
     migrate_decompress_threads_join();
 }
@@ -791,6 +877,7 @@ static void *migration_thread(void *opaque)
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 old_vm_running = runstate_is_running();

+                global_state_store();
                 ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
                 if (ret >= 0) {
                     qemu_file_set_rate_limit(s->file, INT64_MAX);
diff --git a/vl.c b/vl.c
index 555fd88..95acdb1 100644
--- a/vl.c
+++ b/vl.c
@@ -4473,6 +4473,7 @@ int main(int argc, char **argv, char **envp)
         return 0;
     }

+    register_global_state();
     if (incoming) {
         Error *local_err = NULL;
         qemu_start_incoming_migration(incoming, &local_err);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 03/11] migration: create new section to store global state

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> This includes a new section that for now just stores the current qemu state.
> 
> Right now, there are only one way to control what is the state of the
> target after migration.
> 
> - If you run the target qemu with -S, it would start stopped.
> - If you run the target qemu without -S, it would run just after migration finishes.
> 
> The problem here is what happens if we start the target without -S and
> there happens one error during migration that puts current state as
> -EIO.  Migration would ends (notice that the error happend doing block
> IO, network IO, i.e. nothing related with migration), and when
> migration finish, we would just "continue" running on destination,
> probably hanging the guest/corruption data, whatever.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/migration.h |  1 +
>  migration/migration.c         | 93 +++++++++++++++++++++++++++++++++++++++++--
>  vl.c                          |  1 +
>  3 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 9387c8c..1280193 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> 
>  void ram_mig_init(void);
>  void savevm_skip_section_footers(void);
> +void register_global_state(void);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b04b457..01bb90d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -25,6 +25,7 @@
>  #include "qemu/thread.h"
>  #include "qmp-commands.h"
>  #include "trace.h"
> +#include "qapi/util.h"
> 
>  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
> 
> @@ -96,6 +97,81 @@ void migration_incoming_state_destroy(void)
>      mis_current = NULL;
>  }
> 
> +
> +typedef struct {
> +    int32_t size;

Still think that size should be unsigned.

> +    uint8_t runstate[100];
> +} GlobalState;
> +
> +static GlobalState global_state;
> +
> +static void global_state_store(void)
> +{
> +    if (!runstate_store((char *)global_state.runstate,
> +                        sizeof(global_state.runstate))) {
> +        printf("Runstate is too big\n");
> +        exit(-1);

Hmmmm:
  1) Shouldn't that be an error report? or an assert?
  2) Exit should use EXIT_FAILURE (which is +ve as well)
  3) But you shouldn't kill the guest on an outwards migration
     - just fail the migration.
  4) (And anyway this all seems overkill for sending a 
      status string).

> +    }
> +}
> +
> +static char *global_state_get_runstate(void)
> +{
> +    return (char *)global_state.runstate;
> +}
> +
> +static int global_state_post_load(void *opaque, int version_id)
> +{
> +    GlobalState *s = opaque;
> +    int ret = 0;
> +    char *runstate = (char *)s->runstate;
> +
> +    printf("loaded state: %s\n", runstate);

trace_.....

> +
> +    if (strcmp(runstate, "running") != 0) {
> +        Error *local_err = NULL;
> +        int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
> +                                -1, &local_err);

Is there a reason not to do the qapi_enum_parse first, and then
compare it's output to RUN_STATE_RUNNING?

> +
> +        if (r == -1) {
> +            if (local_err) {
> +                error_report_err(local_err);
> +            }
> +            return -1;

I'm not sure, but shouldn't that be -EINVAL ?
(Not that vmstate is consistent about it)

> +        }
> +        ret = vm_stop_force_state(r);

Kind of going back to adding the state transitions;
don't you need to allow INMIGRATE to * - because it could
be pretty much anything? (Shutdown? Suspended?)

> +    }
> +
> +   return ret;
> +}
> +
> +static void global_state_pre_save(void *opaque)
> +{
> +    GlobalState *s = opaque;
> +
> +    s->size = strlen((char *)s->runstate) + 1;
> +    printf("saved state: %s\n", s->runstate);

trace_

> +}
> +
> +static const VMStateDescription vmstate_globalstate = {
> +    .name = "globalstate",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = global_state_post_load,
> +    .pre_save = global_state_pre_save,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(size, GlobalState),
> +        VMSTATE_BUFFER(runstate, GlobalState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +void register_global_state(void)
> +{
> +    /* We would use it independently that we receive it */
> +    strcpy((char *)&global_state.runstate, "");
> +    vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
> +}
> +
>  /*
>   * Called on -incoming with a defer: uri.
>   * The migration can be started later after any parameters have been
> @@ -163,10 +239,20 @@ static void process_incoming_migration_co(void *opaque)
>          exit(EXIT_FAILURE);
>      }
> 
> -    if (autostart) {
> +    /* runstate == "" means that we haven't received it through the
> +     * wire, so we obey autostart.  runstate == runing means that we
> +     * need to run it, we need to make sure that we do it after
> +     * everything else has finished.  Every other state change is done
> +     * at the post_load function */
> +
> +    if (strcmp(global_state_get_runstate(), "running") == 0) {
>          vm_start();
> -    } else {
> -        runstate_set(RUN_STATE_PAUSED);
> +    } else if (strcmp(global_state_get_runstate(), "") == 0) {
> +        if (autostart) {
> +            vm_start();
> +        } else {
> +            runstate_set(RUN_STATE_PAUSED);
> +        }
>      }
>      migrate_decompress_threads_join();
>  }
> @@ -791,6 +877,7 @@ static void *migration_thread(void *opaque)
>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>                  old_vm_running = runstate_is_running();
> 
> +                global_state_store();
>                  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>                  if (ret >= 0) {
>                      qemu_file_set_rate_limit(s->file, INT64_MAX);
> diff --git a/vl.c b/vl.c
> index 555fd88..95acdb1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4473,6 +4473,7 @@ int main(int argc, char **argv, char **envp)
>          return 0;
>      }
> 
> +    register_global_state();
>      if (incoming) {
>          Error *local_err = NULL;
>          qemu_start_incoming_migration(incoming, &local_err);
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 03/11] migration: create new section to store global state

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> This includes a new section that for now just stores the current qemu state.
>> 
>> Right now, there are only one way to control what is the state of the
>> target after migration.
>> 
>> - If you run the target qemu with -S, it would start stopped.
>> - If you run the target qemu without -S, it would run just after migration finishes.
>> 
>> The problem here is what happens if we start the target without -S and
>> there happens one error during migration that puts current state as
>> -EIO.  Migration would ends (notice that the error happend doing block
>> IO, network IO, i.e. nothing related with migration), and when
>> migration finish, we would just "continue" running on destination,
>> probably hanging the guest/corruption data, whatever.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  include/migration/migration.h |  1 +
>>  migration/migration.c         | 93 +++++++++++++++++++++++++++++++++++++++++--
>>  vl.c                          |  1 +
>>  3 files changed, 92 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 9387c8c..1280193 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>> 
>>  void ram_mig_init(void);
>>  void savevm_skip_section_footers(void);
>> +void register_global_state(void);
>>  #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b04b457..01bb90d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -25,6 +25,7 @@
>>  #include "qemu/thread.h"
>>  #include "qmp-commands.h"
>>  #include "trace.h"
>> +#include "qapi/util.h"
>> 
>>  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>> 
>> @@ -96,6 +97,81 @@ void migration_incoming_state_destroy(void)
>>      mis_current = NULL;
>>  }
>> 
>> +
>> +typedef struct {
>> +    int32_t size;
>
> Still think that size should be unsigned.

changed


>
>> +    uint8_t runstate[100];
>> +} GlobalState;
>> +
>> +static GlobalState global_state;
>> +
>> +static void global_state_store(void)
>> +{
>> +    if (!runstate_store((char *)global_state.runstate,
>> +                        sizeof(global_state.runstate))) {
>> +        printf("Runstate is too big\n");
>> +        exit(-1);
>
> Hmmmm:
>   1) Shouldn't that be an error report? or an assert?
>   2) Exit should use EXIT_FAILURE (which is +ve as well)
>   3) But you shouldn't kill the guest on an outwards migration
>      - just fail the migration.
>   4) (And anyway this all seems overkill for sending a 
>       status string).

moved to a trace & and return -EINVAL.  Adjusted only caller to handle
the error.

>
>> +    }
>> +}
>> +
>> +static char *global_state_get_runstate(void)
>> +{
>> +    return (char *)global_state.runstate;
>> +}
>> +
>> +static int global_state_post_load(void *opaque, int version_id)
>> +{
>> +    GlobalState *s = opaque;
>> +    int ret = 0;
>> +    char *runstate = (char *)s->runstate;
>> +
>> +    printf("loaded state: %s\n", runstate);
>
> trace_.....

Done

>
>> +
>> +    if (strcmp(runstate, "running") != 0) {
>> +        Error *local_err = NULL;
>> +        int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
>> +                                -1, &local_err);
>
> Is there a reason not to do the qapi_enum_parse first, and then
> compare it's output to RUN_STATE_RUNNING?

Avoid running the enum_parse at all if we are not at running state, I
don't really care one way or another.

>
>> +
>> +        if (r == -1) {
>> +            if (local_err) {
>> +                error_report_err(local_err);
>> +            }
>> +            return -1;
>
> I'm not sure, but shouldn't that be -EINVAL ?
> (Not that vmstate is consistent about it)

Changed.


>> +        }
>> +        ret = vm_stop_force_state(r);
>
> Kind of going back to adding the state transitions;
> don't you need to allow INMIGRATE to * - because it could
> be pretty much anything? (Shutdown? Suspended?)

good question. Doing the change in the other patch.


>> +    }
>> +
>> +   return ret;
>> +}
>> +
>> +static void global_state_pre_save(void *opaque)
>> +{
>> +    GlobalState *s = opaque;
>> +
>> +    s->size = strlen((char *)s->runstate) + 1;
>> +    printf("saved state: %s\n", s->runstate);
>
> trace_

Done.

>> +}
>> +
>> +static const VMStateDescription vmstate_globalstate = {
>> +    .name = "globalstate",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .post_load = global_state_post_load,
>> +    .pre_save = global_state_pre_save,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_INT32(size, GlobalState),
>> +        VMSTATE_BUFFER(runstate, GlobalState),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +void register_global_state(void)
>> +{
>> +    /* We would use it independently that we receive it */
>> +    strcpy((char *)&global_state.runstate, "");
>> +    vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
>> +}
>> +
>>  /*
>>   * Called on -incoming with a defer: uri.
>>   * The migration can be started later after any parameters have been
>> @@ -163,10 +239,20 @@ static void process_incoming_migration_co(void *opaque)
>>          exit(EXIT_FAILURE);
>>      }
>> 
>> -    if (autostart) {
>> +    /* runstate == "" means that we haven't received it through the
>> +     * wire, so we obey autostart.  runstate == runing means that we
>> +     * need to run it, we need to make sure that we do it after
>> +     * everything else has finished.  Every other state change is done
>> +     * at the post_load function */
>> +
>> +    if (strcmp(global_state_get_runstate(), "running") == 0) {
>>          vm_start();
>> -    } else {
>> -        runstate_set(RUN_STATE_PAUSED);
>> +    } else if (strcmp(global_state_get_runstate(), "") == 0) {
>> +        if (autostart) {
>> +            vm_start();
>> +        } else {
>> +            runstate_set(RUN_STATE_PAUSED);
>> +        }
>>      }
>>      migrate_decompress_threads_join();
>>  }
>> @@ -791,6 +877,7 @@ static void *migration_thread(void *opaque)
>>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>                  old_vm_running = runstate_is_running();
>> 
>> +                global_state_store();
>>                  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>                  if (ret >= 0) {
>>                      qemu_file_set_rate_limit(s->file, INT64_MAX);
>> diff --git a/vl.c b/vl.c
>> index 555fd88..95acdb1 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4473,6 +4473,7 @@ int main(int argc, char **argv, char **envp)
>>          return 0;
>>      }
>> 
>> +    register_global_state();
>>      if (incoming) {
>>          Error *local_err = NULL;
>>          qemu_start_incoming_migration(incoming, &local_err);
>> -- 
>> 2.4.3
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 04/11] global_state: Make section optional

[Juan Quintela]

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.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/i386/pc_piix.c             |  1 +
 hw/i386/pc_q35.c              |  1 +
 include/migration/migration.h |  1 +
 migration/migration.c         | 30 +++++++++++++++++++++++++++++-
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e142f75..735fb22 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -307,6 +307,7 @@ static void pc_init1(MachineState *machine)
 static void pc_compat_2_3(MachineState *machine)
 {
     savevm_skip_section_footers();
+    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 b68263d..0523ecc 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -291,6 +291,7 @@ static void pc_q35_init(MachineState *machine)
 static void pc_compat_2_3(MachineState *machine)
 {
     savevm_skip_section_footers();
+    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 1280193..bb53d93 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -198,4 +198,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
 void ram_mig_init(void);
 void savevm_skip_section_footers(void);
 void register_global_state(void);
+void global_state_set_optional(void);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 01bb90d..f1ecf76 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -99,6 +99,7 @@ void migration_incoming_state_destroy(void)


 typedef struct {
+    bool optional;
     int32_t size;
     uint8_t runstate[100];
 } GlobalState;
@@ -119,6 +120,33 @@ static char *global_state_get_runstate(void)
     return (char *)global_state.runstate;
 }

+void global_state_set_optional(void)
+{
+    global_state.optional = true;
+}
+
+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;
@@ -149,7 +177,6 @@ static void global_state_pre_save(void *opaque)
     GlobalState *s = opaque;

     s->size = strlen((char *)s->runstate) + 1;
-    printf("saved state: %s\n", s->runstate);
 }

 static const VMStateDescription vmstate_globalstate = {
@@ -158,6 +185,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.3

Re: [Qemu-devel] [PATCH 04/11] global_state: Make section optional

[Dr. David Alan Gilbert]

* 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.

Possibly for a different patch in the series; but I'm worried about
at least two of the other states:
   SAVE_VM - I think this means that savevm/loadvm is broken?
   SUSPENDED - what happens to a migrate of a suspended VM?

Dave

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/i386/pc_piix.c             |  1 +
>  hw/i386/pc_q35.c              |  1 +
>  include/migration/migration.h |  1 +
>  migration/migration.c         | 30 +++++++++++++++++++++++++++++-
>  4 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e142f75..735fb22 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -307,6 +307,7 @@ static void pc_init1(MachineState *machine)
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      savevm_skip_section_footers();
> +    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 b68263d..0523ecc 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -291,6 +291,7 @@ static void pc_q35_init(MachineState *machine)
>  static void pc_compat_2_3(MachineState *machine)
>  {
>      savevm_skip_section_footers();
> +    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 1280193..bb53d93 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -198,4 +198,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>  void ram_mig_init(void);
>  void savevm_skip_section_footers(void);
>  void register_global_state(void);
> +void global_state_set_optional(void);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 01bb90d..f1ecf76 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -99,6 +99,7 @@ void migration_incoming_state_destroy(void)
> 
> 
>  typedef struct {
> +    bool optional;
>      int32_t size;
>      uint8_t runstate[100];
>  } GlobalState;
> @@ -119,6 +120,33 @@ static char *global_state_get_runstate(void)
>      return (char *)global_state.runstate;
>  }
> 
> +void global_state_set_optional(void)
> +{
> +    global_state.optional = true;
> +}
> +
> +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;
> @@ -149,7 +177,6 @@ static void global_state_pre_save(void *opaque)
>      GlobalState *s = opaque;
> 
>      s->size = strlen((char *)s->runstate) + 1;
> -    printf("saved state: %s\n", s->runstate);
>  }
> 
>  static const VMStateDescription vmstate_globalstate = {
> @@ -158,6 +185,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.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 05/11] vmstate: Create optional sections

[Juan Quintela]

To make sections optional, we need to do it at the beggining of the code.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/vmstate.h |  2 ++
 migration/savevm.c          |  8 ++++++++
 migration/vmstate.c         | 11 +++++++++++
 trace-events                |  1 +
 4 files changed, 22 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 7153b1e..38de790 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -815,6 +815,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
 void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                         void *opaque, QJSON *vmdesc);

+bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
+
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
diff --git a/migration/savevm.c b/migration/savevm.c
index 2091882..b018f30 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -834,6 +834,11 @@ void qemu_savevm_state_complete(QEMUFile *f)
         if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
             continue;
         }
+        if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) {
+            trace_savevm_section_skip(se->idstr, se->section_id);
+            continue;
+        }
+
         trace_savevm_section_start(se->idstr, se->section_id);

         json_start_object(vmdesc, NULL);
@@ -947,6 +952,9 @@ static int qemu_save_device_state(QEMUFile *f)
         if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
             continue;
         }
+        if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) {
+            continue;
+        }

         save_section_header(f, se, QEMU_VM_SECTION_FULL);

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 6138d1a..e8ccf22 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -276,6 +276,17 @@ static void vmsd_desc_field_end(const VMStateDescription *vmsd, QJSON *vmdesc,
     json_end_object(vmdesc);
 }

+
+bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
+{
+    if (vmsd->needed && !vmsd->needed(opaque)) {
+        /* optional section not needed */
+        return false;
+    }
+    return true;
+}
+
+
 void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                         void *opaque, QJSON *vmdesc)
 {
diff --git a/trace-events b/trace-events
index 52b7efa..bcdce29 100644
--- a/trace-events
+++ b/trace-events
@@ -1191,6 +1191,7 @@ qemu_loadvm_state_section_partend(uint32_t section_id) "%u"
 qemu_loadvm_state_section_startfull(uint32_t section_id, const char *idstr, uint32_t instance_id, uint32_t version_id) "%u(%s) %u %u"
 savevm_section_start(const char *id, unsigned int section_id) "%s, section_id %u"
 savevm_section_end(const char *id, unsigned int section_id, int ret) "%s, section_id %u -> %d"
+savevm_section_skip(const char *id, unsigned int section_id) "%s, section_id %u"
 savevm_state_begin(void) ""
 savevm_state_header(void) ""
 savevm_state_iterate(void) ""
-- 
2.4.3

[Qemu-devel] [PATCH 06/11] migration: Add configuration section

[Juan Quintela]

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 ++
 migration/savevm.c            | 61 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 735fb22..5009836 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -308,6 +308,7 @@ static void pc_compat_2_3(MachineState *machine)
 {
     savevm_skip_section_footers();
     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 0523ecc..3dd060d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,6 +292,7 @@ static void pc_compat_2_3(MachineState *machine)
 {
     savevm_skip_section_footers();
     global_state_set_optional();
+    savevm_skip_configuration();
 }

 static void pc_compat_2_2(MachineState *machine)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bb53d93..cfc0608 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
 #define QEMU_VM_SECTION_FOOTER       0x7e

 struct MigrationParams {
@@ -199,4 +200,5 @@ void ram_mig_init(void);
 void savevm_skip_section_footers(void);
 void register_global_state(void);
 void global_state_set_optional(void);
+void savevm_skip_configuration(void);
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index b018f30..00ea10d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -244,11 +244,55 @@ typedef struct SaveStateEntry {
 typedef struct SaveState {
     QTAILQ_HEAD(, SaveStateEntry) handlers;
     int global_section_id;
+    bool skip_configuration;
+    uint32_t len;
+    const 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 = 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 = "configuration",
+    .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,
@@ -721,6 +765,11 @@ void qemu_savevm_state_begin(QEMUFile *f,
         se->ops->set_params(params, se->opaque);
     }

+    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;
@@ -1035,6 +1084,18 @@ int qemu_loadvm_state(QEMUFile *f)
         return -ENOTSUP;
     }

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

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

[Dr. David Alan Gilbert]

* 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.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> 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 ++
>  migration/savevm.c            | 61 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 65 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 735fb22..5009836 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -308,6 +308,7 @@ static void pc_compat_2_3(MachineState *machine)
>  {
>      savevm_skip_section_footers();
>      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 0523ecc..3dd060d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -292,6 +292,7 @@ static void pc_compat_2_3(MachineState *machine)
>  {
>      savevm_skip_section_footers();
>      global_state_set_optional();
> +    savevm_skip_configuration();
>  }
> 
>  static void pc_compat_2_2(MachineState *machine)
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index bb53d93..cfc0608 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
>  #define QEMU_VM_SECTION_FOOTER       0x7e
> 
>  struct MigrationParams {
> @@ -199,4 +200,5 @@ void ram_mig_init(void);
>  void savevm_skip_section_footers(void);
>  void register_global_state(void);
>  void global_state_set_optional(void);
> +void savevm_skip_configuration(void);
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b018f30..00ea10d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -244,11 +244,55 @@ typedef struct SaveStateEntry {
>  typedef struct SaveState {
>      QTAILQ_HEAD(, SaveStateEntry) handlers;
>      int global_section_id;
> +    bool skip_configuration;
> +    uint32_t len;
> +    const 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 = 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 = "configuration",
> +    .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,
> @@ -721,6 +765,11 @@ void qemu_savevm_state_begin(QEMUFile *f,
>          se->ops->set_params(params, se->opaque);
>      }
> 
> +    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;
> @@ -1035,6 +1084,18 @@ int qemu_loadvm_state(QEMUFile *f)
>          return -ENOTSUP;
>      }
> 
> +    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;
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 07/11] migration: Use cmpxchg correctly

[Juan Quintela]

cmpxchg returns the old value

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index f1ecf76..1791185 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -505,7 +505,7 @@ void qmp_migrate_set_parameters(bool has_compress_level,

 static void migrate_set_state(MigrationState *s, int old_state, int new_state)
 {
-    if (atomic_cmpxchg(&s->state, old_state, new_state) == new_state) {
+    if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
         trace_migrate_set_state(new_state);
     }
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH 07/11] migration: Use cmpxchg correctly

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> cmpxchg returns the old value
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Nice spot.

> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index f1ecf76..1791185 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -505,7 +505,7 @@ void qmp_migrate_set_parameters(bool has_compress_level,
> 
>  static void migrate_set_state(MigrationState *s, int old_state, int new_state)
>  {
> -    if (atomic_cmpxchg(&s->state, old_state, new_state) == new_state) {
> +    if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
>          trace_migrate_set_state(new_state);
>      }
>  }
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 08/11] migration: Use always helper to set state

[Juan Quintela]

There were three places that were not using the migrate_set_state()
helper, just fix that.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1791185..1c84249 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -545,7 +545,7 @@ void migrate_fd_error(MigrationState *s)
 {
     trace_migrate_fd_error();
     assert(s->file == NULL);
-    s->state = MIGRATION_STATUS_FAILED;
+    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
     trace_migrate_set_state(MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
@@ -630,7 +630,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
     s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
                decompress_thread_count;
     s->bandwidth_limit = bandwidth_limit;
-    s->state = MIGRATION_STATUS_SETUP;
+    migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
     trace_migrate_set_state(MIGRATION_STATUS_SETUP);

     s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
@@ -723,7 +723,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 #endif
     } else {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
-        s->state = MIGRATION_STATUS_FAILED;
+        migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
         return;
     }

-- 
2.4.3

Re: [Qemu-devel] [PATCH 08/11] migration: Use always helper to set state

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> There were three places that were not using the migrate_set_state()
> helper, just fix that.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1791185..1c84249 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -545,7 +545,7 @@ void migrate_fd_error(MigrationState *s)
>  {
>      trace_migrate_fd_error();
>      assert(s->file == NULL);
> -    s->state = MIGRATION_STATUS_FAILED;
> +    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
>      trace_migrate_set_state(MIGRATION_STATUS_FAILED);
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
> @@ -630,7 +630,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
>      s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
>                 decompress_thread_count;
>      s->bandwidth_limit = bandwidth_limit;
> -    s->state = MIGRATION_STATUS_SETUP;
> +    migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
>      trace_migrate_set_state(MIGRATION_STATUS_SETUP);

Does that work if you do a migrate, cancel it and then migrate again?
I think the status would be MIGRATION_STATUS_CANCELLED at that point.

Dave

> 
>      s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> @@ -723,7 +723,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>  #endif
>      } else {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
> -        s->state = MIGRATION_STATUS_FAILED;
> +        migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
>          return;
>      }
> 
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 08/11] migration: Use always helper to set state

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> There were three places that were not using the migrate_set_state()
>> helper, just fix that.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1791185..1c84249 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -545,7 +545,7 @@ void migrate_fd_error(MigrationState *s)
>>  {
>>      trace_migrate_fd_error();
>>      assert(s->file == NULL);
>> -    s->state = MIGRATION_STATUS_FAILED;
>> +    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
>>      trace_migrate_set_state(MIGRATION_STATUS_FAILED);
>>      notifier_list_notify(&migration_state_notifiers, s);
>>  }
>> @@ -630,7 +630,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
>>      s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
>>                 decompress_thread_count;
>>      s->bandwidth_limit = bandwidth_limit;
>> -    s->state = MIGRATION_STATUS_SETUP;
>> +    migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
>>      trace_migrate_set_state(MIGRATION_STATUS_SETUP);
>
> Does that work if you do a migrate, cancel it and then migrate again?
> I think the status would be MIGRATION_STATUS_CANCELLED at that point.

Force state to be NONE in a different patch, see new series.


>
> Dave
>
>> 
>>      s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>> @@ -723,7 +723,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>>  #endif
>>      } else {
>>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
>> -        s->state = MIGRATION_STATUS_FAILED;
>> +        migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
>>          return;
>>      }
>> 
>> -- 
>> 2.4.3
>> 
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 09/11] migration: No need to call trace_migrate_set_state()

[Juan Quintela]

We now use the helper everywhere, so no need to call this on this two
places.  See on previous commit that there were a place where we missed
to mark the trace.  Now all tracing is done in migrate_set_state().

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 1c84249..b31c7f4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -546,7 +546,6 @@ void migrate_fd_error(MigrationState *s)
     trace_migrate_fd_error();
     assert(s->file == NULL);
     migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
-    trace_migrate_set_state(MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }

@@ -631,7 +630,6 @@ static MigrationState *migrate_init(const MigrationParams *params)
                decompress_thread_count;
     s->bandwidth_limit = bandwidth_limit;
     migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
-    trace_migrate_set_state(MIGRATION_STATUS_SETUP);

     s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     return s;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 09/11] migration: No need to call trace_migrate_set_state()

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> We now use the helper everywhere, so no need to call this on this two
> places.  See on previous commit that there were a place where we missed
> to mark the trace.  Now all tracing is done in migrate_set_state().

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1c84249..b31c7f4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -546,7 +546,6 @@ void migrate_fd_error(MigrationState *s)
>      trace_migrate_fd_error();
>      assert(s->file == NULL);
>      migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
> -    trace_migrate_set_state(MIGRATION_STATUS_FAILED);
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
> 
> @@ -631,7 +630,6 @@ static MigrationState *migrate_init(const MigrationParams *params)
>                 decompress_thread_count;
>      s->bandwidth_limit = bandwidth_limit;
>      migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
> -    trace_migrate_set_state(MIGRATION_STATUS_SETUP);
> 
>      s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>      return s;
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 10/11] migration: create migration event

[Juan Quintela]

We have one argument that tells us what event has happened.

Signed-off-by: Juan Quintela <quintela@redhat.com>

X3

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 docs/qmp/qmp-events.txt | 14 ++++++++++++++
 migration/migration.c   |  2 ++
 qapi/event.json         | 14 ++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 4c13d48..3fe8cb7 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -473,6 +473,20 @@ Example:
 { "timestamp": {"seconds": 1290688046, "microseconds": 417172},
   "event": "SPICE_MIGRATE_COMPLETED" }

+MIGRATION
+---------
+
+Emitted when a migration event happens
+
+Data: None.
+
+ - "status": migration status
+     See MigrationStatus in ~/qapi-scheme.json for possible values
+
+Example:
+
+{"timestamp": {"seconds": 1432121972, "microseconds": 744001},
+ "event": "MIGRATION", "data": {"status": "completed"}}

 STOP
 ----
diff --git a/migration/migration.c b/migration/migration.c
index b31c7f4..3637d36 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -26,6 +26,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "qapi/util.h"
+#include "qapi-event.h"

 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */

@@ -506,6 +507,7 @@ void qmp_migrate_set_parameters(bool has_compress_level,
 static void migrate_set_state(MigrationState *s, int old_state, int new_state)
 {
     if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
+        qapi_event_send_migration(new_state, &error_abort);
         trace_migrate_set_state(new_state);
     }
 }
diff --git a/qapi/event.json b/qapi/event.json
index 378dda5..fe5e182 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -243,6 +243,20 @@
 { 'event': 'SPICE_MIGRATE_COMPLETED' }

 ##
+# @MIGRATION
+#
+# Emitted when a migration event happens
+#
+# @status: @MigrationStatus describing the current migration status.
+#          If this field is not returned, no migration process
+#          has been initiated
+#
+# Since: 2.4
+##
+{ 'event': 'MIGRATION',
+  'data': {'status': 'MigrationStatus'}}
+
+##
 # @ACPI_DEVICE_OST
 #
 # Emitted when guest executes ACPI _OST method.
-- 
2.4.3

Re: [Qemu-devel] [PATCH 10/11] migration: create migration event

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

On 06/16/2015 07:50 PM, Juan Quintela wrote:
> We have one argument that tells us what event has happened.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> X3
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Intentional double-S-o-b?

> ---
>  docs/qmp/qmp-events.txt | 14 ++++++++++++++
>  migration/migration.c   |  2 ++
>  qapi/event.json         | 14 ++++++++++++++
>  3 files changed, 30 insertions(+)
> 

> +Data: None.
> +
> + - "status": migration status
> +     See MigrationStatus in ~/qapi-scheme.json for possible values

s/scheme/schema/


> +++ b/qapi/event.json
> @@ -243,6 +243,20 @@
>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
> 
>  ##
> +# @MIGRATION
> +#
> +# Emitted when a migration event happens
> +#
> +# @status: @MigrationStatus describing the current migration status.
> +#          If this field is not returned, no migration process
> +#          has been initiated

Drop this second sentence. 'status' is emitted unconditionally (it is
not marked optional).

With those two changes,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/11] migration: create migration event

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> On 06/16/2015 07:50 PM, Juan Quintela wrote:
>> We have one argument that tells us what event has happened.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> X3
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Intentional double-S-o-b?

No, merge artifact


>
>> ---
>>  docs/qmp/qmp-events.txt | 14 ++++++++++++++
>>  migration/migration.c   |  2 ++
>>  qapi/event.json         | 14 ++++++++++++++
>>  3 files changed, 30 insertions(+)
>> 
>
>> +Data: None.
>> +
>> + - "status": migration status
>> +     See MigrationStatus in ~/qapi-scheme.json for possible values
>
> s/scheme/schema/

fixed

>
>
>> +++ b/qapi/event.json
>> @@ -243,6 +243,20 @@
>>  { 'event': 'SPICE_MIGRATE_COMPLETED' }
>> 
>>  ##
>> +# @MIGRATION
>> +#
>> +# Emitted when a migration event happens
>> +#
>> +# @status: @MigrationStatus describing the current migration status.
>> +#          If this field is not returned, no migration process
>> +#          has been initiated
>
> Drop this second sentence. 'status' is emitted unconditionally (it is
> not marked optional).
>
> With those two changes,
> Reviewed-by: Eric Blake <eblake@redhat.com>

fixed

[Qemu-devel] [PATCH 11/11] migration: Add migration events on target side

[Juan Quintela]

We reuse the migration events from the source side, sending them on the
appropiate place.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 migration/migration.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3637d36..2b4fd55 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -218,6 +218,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;

+    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
     if (!strcmp(uri, "defer")) {
         deferred_incoming_migration(errp);
     } else if (strstart(uri, "tcp:", &p)) {
@@ -246,7 +247,7 @@ static void process_incoming_migration_co(void *opaque)
     int ret;

     migration_incoming_state_new(f);
-
+    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
     ret = qemu_loadvm_state(f);

     qemu_fclose(f);
@@ -254,10 +255,12 @@ static void process_incoming_migration_co(void *opaque)
     migration_incoming_state_destroy();

     if (ret < 0) {
+        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
         error_report("load of migration failed: %s", strerror(-ret));
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
     }
+    qapi_event_send_migration(MIGRATION_STATUS_COMPLETED, &error_abort);
     qemu_announce_self();

     /* Make sure all file formats flush their mutable metadata */
-- 
2.4.3

Re: [Qemu-devel] [PATCH 11/11] migration: Add migration events on target side

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> We reuse the migration events from the source side, sending them on the
> appropiate place.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  migration/migration.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 3637d36..2b4fd55 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -218,6 +218,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> 
> +    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);

Try and avoid error_abort - you don't want to trigger an assert (and associated
core etc) if it's just something like the monitor disconnecting.
(And anyway in this case you have an errp).

Dave

>      if (!strcmp(uri, "defer")) {
>          deferred_incoming_migration(errp);
>      } else if (strstart(uri, "tcp:", &p)) {
> @@ -246,7 +247,7 @@ static void process_incoming_migration_co(void *opaque)
>      int ret;
> 
>      migration_incoming_state_new(f);
> -
> +    qapi_event_send_migration(MIGRATION_STATUS_ACTIVE, &error_abort);
>      ret = qemu_loadvm_state(f);
> 
>      qemu_fclose(f);
> @@ -254,10 +255,12 @@ static void process_incoming_migration_co(void *opaque)
>      migration_incoming_state_destroy();
> 
>      if (ret < 0) {
> +        qapi_event_send_migration(MIGRATION_STATUS_FAILED, &error_abort);
>          error_report("load of migration failed: %s", strerror(-ret));
>          migrate_decompress_threads_join();
>          exit(EXIT_FAILURE);
>      }
> +    qapi_event_send_migration(MIGRATION_STATUS_COMPLETED, &error_abort);
>      qemu_announce_self();
> 
>      /* Make sure all file formats flush their mutable metadata */
> -- 
> 2.4.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 11/11] migration: Add migration events on target side

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

On 06/18/2015 04:53 AM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> We reuse the migration events from the source side, sending them on the
>> appropiate place.

s/appropiate/appropriate/

>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  migration/migration.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3637d36..2b4fd55 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -218,6 +218,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>>  {
>>      const char *p;
>>
>> +    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
> 
> Try and avoid error_abort - you don't want to trigger an assert (and associated
> core etc) if it's just something like the monitor disconnecting.
> (And anyway in this case you have an errp).

But this use is fine, matching the idiom of ALL OTHER qapi_event_send_*
calls.  (Arguably, if sending an event can never fail, then maybe we
shouldn't have made it a parameter; OOM failures already abort, and if
the only other possible failure is malformed json but the whole point of
a generated code guarantees that we cannot hit that bug, or if the only
failure is a disconnected monitor but you can't report the error because
you have no monitor left, then being able to catch an error doesn't help).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 11/11] migration: Add migration events on target side

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 06/18/2015 04:53 AM, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> We reuse the migration events from the source side, sending them on the
> >> appropiate place.
> 
> s/appropiate/appropriate/
> 
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> ---
> >>  migration/migration.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index 3637d36..2b4fd55 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -218,6 +218,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
> >>  {
> >>      const char *p;
> >>
> >> +    qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
> > 
> > Try and avoid error_abort - you don't want to trigger an assert (and associated
> > core etc) if it's just something like the monitor disconnecting.
> > (And anyway in this case you have an errp).
> 
> But this use is fine, matching the idiom of ALL OTHER qapi_event_send_*
> calls.  (Arguably, if sending an event can never fail, then maybe we
> shouldn't have made it a parameter; OOM failures already abort, and if
> the only other possible failure is malformed json but the whole point of
> a generated code guarantees that we cannot hit that bug, or if the only
> failure is a disconnected monitor but you can't report the error because
> you have no monitor left, then being able to catch an error doesn't help).

I think you're right the current stuff hung off qapi_event_send_migration never
uses the error pointer.  Still, it would be nice to try and avoid error_abort
where possible; you can see some implementation of an event sender deciding
to throw an error if it can't write to whatever event log it's got, and then
you don't want to cause an assert() - you might want an error printed and an 
exit, but you dont want an assert.

Anyway, since as you say, since all callers are equally broken, and this
was my only issue with this patch:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK