[Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Juan Quintela]

Previous commit only stored a valid state for migration.  It stored the
empty string for savevm.  Now, we are also storing the current state for
savevm.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/migration.h | 1 +
 migration/migration.c         | 2 +-
 migration/savevm.c            | 6 ++++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index b2711ef..a2f8ed0 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -202,4 +202,5 @@ void savevm_skip_section_footers(void);
 void register_global_state(void);
 void global_state_set_optional(void);
 void savevm_skip_configuration(void);
+int global_state_store(void);
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index ba82ff6..86ca099 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,7 +110,7 @@ typedef struct {

 static GlobalState global_state;

-static int global_state_store(void)
+int global_state_store(void)
 {
     if (!runstate_store((char *)global_state.runstate,
                         sizeof(global_state.runstate))) {
diff --git a/migration/savevm.c b/migration/savevm.c
index 86735fc..81dbe58 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1315,6 +1315,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }

     saved_vm_running = runstate_is_running();
+
+    ret = global_state_store();
+    if (ret) {
+        monitor_printf(mon, "Error saving global state\n");
+        return;
+    }
     vm_stop(RUN_STATE_SAVE_VM);

     memset(sn, 0, sizeof(*sn));
-- 
2.4.3

Re: [Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Amit Shah]

On (Wed) 15 Jul 2015 [09:56:22], Juan Quintela wrote:
> Previous commit only stored a valid state for migration.  It stored the
> empty string for savevm.  Now, we are also storing the current state for
> savevm.

Can you include some more information in the commit message?  Like the
commit hash instead of previous commit, and what bug this fixes?

Thanks,

		Amit

Re: [Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Previous commit only stored a valid state for migration.  It stored the
> empty string for savevm.  Now, we are also storing the current state for
> savevm.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

Looks OK to me; Christian - does it fix it for you?

Dave

> ---
>  include/migration/migration.h | 1 +
>  migration/migration.c         | 2 +-
>  migration/savevm.c            | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index b2711ef..a2f8ed0 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -202,4 +202,5 @@ void savevm_skip_section_footers(void);
>  void register_global_state(void);
>  void global_state_set_optional(void);
>  void savevm_skip_configuration(void);
> +int global_state_store(void);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index ba82ff6..86ca099 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -110,7 +110,7 @@ typedef struct {
> 
>  static GlobalState global_state;
> 
> -static int global_state_store(void)
> +int global_state_store(void)
>  {
>      if (!runstate_store((char *)global_state.runstate,
>                          sizeof(global_state.runstate))) {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 86735fc..81dbe58 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1315,6 +1315,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      }
> 
>      saved_vm_running = runstate_is_running();
> +
> +    ret = global_state_store();
> +    if (ret) {
> +        monitor_printf(mon, "Error saving global state\n");
> +        return;
> +    }
>      vm_stop(RUN_STATE_SAVE_VM);
> 
>      memset(sn, 0, sizeof(*sn));
> -- 
> 2.4.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 15 Jul 2015 [09:56:22], Juan Quintela wrote:
>> Previous commit only stored a valid state for migration.  It stored the
>> empty string for savevm.  Now, we are also storing the current state for
>> savevm.
>
> Can you include some more information in the commit message?  Like the
> commit hash instead of previous commit, and what bug this fixes?
>
> Thanks,
>
> 		Amit

What about:


Commit df4b1024526cae3479da3492d6371fd4a7324a03 introduced global_state
section.  But it only filled the state while doing migration.  While
doing a savevm, we stored an empty string as state.  So when we did a
loadvm, it complained that state was invalid.

Fedora 21, 4.1.1, qemu 2.4.0-rc0
> ../../configure --target-list="x86_64-softmmu"

068 2s ... - output mismatch (see 068.out.bad)
--- /home/bos/jhuston/src/qemu/tests/qemu-iotests/068.out	2015-07-08
17:56:18.588164979 -0400
+++ 068.out.bad	2015-07-09 17:39:58.636651317 -0400
@@ -6,6 +6,8 @@
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
+qemu-system-x86_64: Unknown savevm section or instance 'globalstate' 0
+qemu-system-x86_64: Error -22 while loading VM state
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) quit
 *** done
Failures: 068
Failed 1 of 1 tests

Actually, there were two problems here:
- we registered global_state too late for load_vm (fixed on another
  patch on the list)
- we didn't store a valid state for savevm (fixed by this patch).


Reported-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Christian Borntraeger]

Am 15.07.2015 um 10:18 schrieb Dr. David Alan Gilbert:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Previous commit only stored a valid state for migration.  It stored the
>> empty string for savevm.  Now, we are also storing the current state for
>> savevm.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Looks OK to me; Christian - does it fix it for you?

This patch seems to work ok.


> 
> Dave
> 
>> ---
>>  include/migration/migration.h | 1 +
>>  migration/migration.c         | 2 +-
>>  migration/savevm.c            | 6 ++++++
>>  3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index b2711ef..a2f8ed0 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -202,4 +202,5 @@ void savevm_skip_section_footers(void);
>>  void register_global_state(void);
>>  void global_state_set_optional(void);
>>  void savevm_skip_configuration(void);
>> +int global_state_store(void);
>>  #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index ba82ff6..86ca099 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -110,7 +110,7 @@ typedef struct {
>>
>>  static GlobalState global_state;
>>
>> -static int global_state_store(void)
>> +int global_state_store(void)
>>  {
>>      if (!runstate_store((char *)global_state.runstate,
>>                          sizeof(global_state.runstate))) {
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 86735fc..81dbe58 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1315,6 +1315,12 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>      }
>>
>>      saved_vm_running = runstate_is_running();
>> +
>> +    ret = global_state_store();
>> +    if (ret) {
>> +        monitor_printf(mon, "Error saving global state\n");
>> +        return;
>> +    }
>>      vm_stop(RUN_STATE_SAVE_VM);
>>
>>      memset(sn, 0, sizeof(*sn));
>> -- 
>> 2.4.3
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH] migration: We also want to store the global state for savevm

[Amit Shah]

On (Wed) 15 Jul 2015 [10:19:04], Juan Quintela wrote:
> Amit Shah <amit.shah@redhat.com> wrote:
> > On (Wed) 15 Jul 2015 [09:56:22], Juan Quintela wrote:
> >> Previous commit only stored a valid state for migration.  It stored the
> >> empty string for savevm.  Now, we are also storing the current state for
> >> savevm.
> >
> > Can you include some more information in the commit message?  Like the
> > commit hash instead of previous commit, and what bug this fixes?
> >
> > Thanks,
> >
> > 		Amit
> 
> What about:

Nice :-)

> Commit df4b1024526cae3479da3492d6371fd4a7324a03 introduced global_state
> section.  But it only filled the state while doing migration.  While
> doing a savevm, we stored an empty string as state.  So when we did a
> loadvm, it complained that state was invalid.
> 
> Fedora 21, 4.1.1, qemu 2.4.0-rc0
> > ../../configure --target-list="x86_64-softmmu"
> 
> 068 2s ... - output mismatch (see 068.out.bad)
> --- /home/bos/jhuston/src/qemu/tests/qemu-iotests/068.out	2015-07-08
> 17:56:18.588164979 -0400
> +++ 068.out.bad	2015-07-09 17:39:58.636651317 -0400
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +qemu-system-x86_64: Unknown savevm section or instance 'globalstate' 0
> +qemu-system-x86_64: Error -22 while loading VM state
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) quit
>  *** done
> Failures: 068
> Failed 1 of 1 tests
> 
> Actually, there were two problems here:
> - we registered global_state too late for load_vm (fixed on another
>   patch on the list)
> - we didn't store a valid state for savevm (fixed by this patch).
> 
> 
> Reported-by: John Snow <jsnow@redhat.com>

Thanks,

		Amit