Patchwork Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled

login
register
mail settings
Submitter Anthony Liguori
Date March 22, 2011, 1:26 p.m.
Message ID <4D88A372.9040808@us.ibm.com>
Download mbox | patch
Permalink /patch/87920/
State New
Headers show

Comments

Anthony Liguori - March 22, 2011, 1:26 p.m.
On 03/22/2011 07:00 AM, Jan Kiszka wrote:
> We had a few migration related regressions recently. Do we have
> sufficient test cases in autotest for them? Also for migrating from
> older to the latest version?

Autotest is too late and also not nearly rigorous enough for what you're 
trying to catch.

Here's how I propose we tackle this.  This patch adds a -dump-savevm 
option that takes a version.  It spits out all of the fields we save for 
a particular version (well, not really, but it should).  We also can add 
type information.  The idea is that we'd write a simple test case (using 
gtester) that ran through and dumped the schema for each version.  We'd 
store the schema's in the tree and the test can compare old schema's to 
the current schema to check for failure.

This was thrown together in just a few minutes.  I'll try to put 
together something more complete later today but I wanted to share this 
before the call at least.

Regards,

Anthony Liguori

> Jan
>
Juan Quintela - March 22, 2011, 1:55 p.m.
Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 03/22/2011 07:00 AM, Jan Kiszka wrote:
>> We had a few migration related regressions recently. Do we have
>> sufficient test cases in autotest for them? Also for migrating from
>> older to the latest version?
>
> Autotest is too late and also not nearly rigorous enough for what
> you're trying to catch.
>
> Here's how I propose we tackle this.  This patch adds a -dump-savevm
> option that takes a version.  It spits out all of the fields we save
> for a particular version (well, not really, but it should).  We also
> can add type information.  The idea is that we'd write a simple test
> case (using gtester) that ran through and dumped the schema for each
> version.  We'd store the schema's in the tree and the test can compare
> old schema's to the current schema to check for failure.
>
> This was thrown together in just a few minutes.  I'll try to put
> together something more complete later today but I wanted to share
> this before the call at least.

This would be an start, althought I still think that a way to dump a
single device, and a way to dump the state of a device in a specific
version is needed.  Information as:
- is this always saved
- size of arrays
- ....

that is there is not saved.

Later, Juan.
Anthony Liguori - March 22, 2011, 1:59 p.m.
On 03/22/2011 08:55 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> On 03/22/2011 07:00 AM, Jan Kiszka wrote:
>>> We had a few migration related regressions recently. Do we have
>>> sufficient test cases in autotest for them? Also for migrating from
>>> older to the latest version?
>> Autotest is too late and also not nearly rigorous enough for what
>> you're trying to catch.
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm
>> option that takes a version.  It spits out all of the fields we save
>> for a particular version (well, not really, but it should).  We also
>> can add type information.  The idea is that we'd write a simple test
>> case (using gtester) that ran through and dumped the schema for each
>> version.  We'd store the schema's in the tree and the test can compare
>> old schema's to the current schema to check for failure.
>>
>> This was thrown together in just a few minutes.  I'll try to put
>> together something more complete later today but I wanted to share
>> this before the call at least.
> This would be an start, althought I still think that a way to dump a
> single device, and a way to dump the state of a device in a specific
> version is needed.  Information as:
> - is this always saved
> - size of arrays
> - ....
>
> that is there is not saved.

Yeah, we can add that down the road though.  With just something as 
simple as this, we can catch quite a few regressions.

Regards,

Anthony Liguori

> Later, Juan.
Avi Kivity - March 23, 2011, 9:10 a.m.
On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>
> Here's how I propose we tackle this.  This patch adds a -dump-savevm 
> option that takes a version.  It spits out all of the fields we save 
> for a particular version (well, not really, but it should).  We also 
> can add type information.  The idea is that we'd write a simple test 
> case (using gtester) that ran through and dumped the schema for each 
> version.  We'd store the schema's in the tree and the test can compare 
> old schema's to the current schema to check for failure.
>

Instead of generating the schema and comparing, what about the other way 
round?  Write vmstate in a formal schema, and generate the code at runtime.
Yoshiaki Tamura - March 23, 2011, 11:22 a.m.
2011/3/23 Avi Kivity <avi@redhat.com>:
> On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm
>> option that takes a version.  It spits out all of the fields we save for a
>> particular version (well, not really, but it should).  We also can add type
>> information.  The idea is that we'd write a simple test case (using gtester)
>> that ran through and dumped the schema for each version.  We'd store the
>> schema's in the tree and the test can compare old schema's to the current
>> schema to check for failure.
>>
>
> Instead of generating the schema and comparing, what about the other way
> round?  Write vmstate in a formal schema, and generate the code at runtime.

I agree :)

Yoshi

>
> --
> error compiling committee.c: too many arguments to function
>
>
>
Anthony Liguori - March 23, 2011, 12:15 p.m.
On 03/23/2011 04:10 AM, Avi Kivity wrote:
> On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm 
>> option that takes a version.  It spits out all of the fields we save 
>> for a particular version (well, not really, but it should).  We also 
>> can add type information.  The idea is that we'd write a simple test 
>> case (using gtester) that ran through and dumped the schema for each 
>> version.  We'd store the schema's in the tree and the test can 
>> compare old schema's to the current schema to check for failure.
>>
>
> Instead of generating the schema and comparing, what about the other 
> way round?  Write vmstate in a formal schema, and generate the code at 
> runtime.

This is exactly where I want to go in the future.

Regards,

Anthony Liguori

Patch

From 43fb56c4ee68905d886ea21dad338f3e76350ba5 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Tue, 22 Mar 2011 08:21:00 -0500
Subject: [PATCH] vl: add -dump-savevm option to display VMState schema

This can be used to verify whether the savevm schema has changed for a given
version of the migration protocol.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/hw.h         |    2 ++
 qemu-options.hx |    9 +++++++++
 savevm.c        |   22 ++++++++++++++++++++++
 vl.c            |    9 +++++++++
 4 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 1b09039..55816da 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -914,4 +914,6 @@  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    int required_for_version);
 void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
                         void *opaque);
+void vmstate_dump(FILE *f, int version);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index badb730..801757b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2349,6 +2349,15 @@  Specify a trace file to log output traces to.
 ETEXI
 #endif
 
+DEF("dump-savevm", HAS_ARG, QEMU_OPTION_dump_savevm,
+    "-dump-savevm VERSION\n"
+    "                dump the savevm schema for a given version\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -dump-savevm
+@findex -dump-savevm
+ETEXI
+
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
 @end table
diff --git a/savevm.c b/savevm.c
index 03fce62..da1fea1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1436,6 +1436,28 @@  static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
     vmstate_save_state(f,se->vmsd, se->opaque);
 }
 
+void vmstate_dump(FILE *f, int version)
+{
+    SaveStateEntry *se;
+    bool first = true;
+
+    QTAILQ_FOREACH(se, &savevm_handlers, entry) {
+        VMStateField *field;
+        if (!se->vmsd) {
+            continue;
+        }
+        if (first) {
+            first = false;
+        } else {
+            fprintf(f, "\n");
+        }
+        fprintf(f, "%s.__version__ = %d\n", se->vmsd->name, se->version_id);
+        for (field = se->vmsd->fields; field->name; field++) {
+            fprintf(f, "%s.%s\n", se->vmsd->name, field->name);
+        }
+    }
+}
+
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
 #define QEMU_VM_FILE_VERSION         0x00000003
diff --git a/vl.c b/vl.c
index ac47211..361fcef 100644
--- a/vl.c
+++ b/vl.c
@@ -1941,6 +1941,7 @@  int main(int argc, char **argv, char **envp)
     int show_vnc_port = 0;
     int defconfig = 1;
     const char *trace_file = NULL;
+    int dump_savevm = -1;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -2760,6 +2761,9 @@  int main(int argc, char **argv, char **envp)
                     fclose(fp);
                     break;
                 }
+            case QEMU_OPTION_dump_savevm:
+                dump_savevm = atoi(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }
@@ -3013,6 +3017,11 @@  int main(int argc, char **argv, char **envp)
 
     cpu_synchronize_all_post_init();
 
+    if (dump_savevm != -1) {
+        vmstate_dump(stdout, dump_savevm);
+        exit(0);
+    }
+
     /* must be after terminal init, SDL library changes signal handlers */
     os_setup_signal_handling();
 
-- 
1.7.0.4