diff mbox

migration: We also want to store the global state for savevm

Message ID 1436946982-24643-1-git-send-email-quintela@redhat.com
State New
Headers show

Commit Message

Juan Quintela July 15, 2015, 7:56 a.m. UTC 
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(-)

Comments

Amit Shah July 15, 2015, 8:10 a.m. UTC | #1 
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
Dr. David Alan Gilbert July 15, 2015, 8:18 a.m. UTC | #2 
* 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
Juan Quintela July 15, 2015, 8:19 a.m. UTC | #3 
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>
Christian Borntraeger July 15, 2015, 8:33 a.m. UTC | #4 
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
>
Amit Shah July 15, 2015, 8:40 a.m. UTC | #5 
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
diff mbox

Patch

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