[PATCH v2 0/6] scripts/migration: Fix analyze-migration.py and add a test

[PATCH v2 0/6] scripts/migration: Fix analyze-migration.py and add a test

[Fabiano Rosas]

was: [PATCH] qtest/migration: Add a test for the analyze-migration script
https://lore.kernel.org/r/20230927214756.14117-1-farosas@suse.de

The analyze-migration.py script should be kept in sync with the code
that generates the migration stream. The addition/removal of sections
and flags from the stream can cause the script to break. Issues when
parsing the stream mostly manifest in the form of cryptic python
errors such as:

Looking at git log, it seems that this is a fairly useful script for
people debugging issues with the migration stream. Let's add a test
for it to catch bugs early avoid keeping a broken script in the tree.

People suggested putting this in avocado or a dedicated shell
script. I hope looking at the patches it becomes clear that it's best
to have this along with migration-test. Mostly to avoid duplicating
capabilities code and arch-specific flags.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1030789937

Fabiano Rosas (5):
  migration: Fix analyze-migration.py 'configuration' parsing
  migration: Add capability parsing to analyze-migration.py
  migration: Fix analyze-migration.py when ignore-shared is used
  migration: Fix analyze-migration read operation signedness
  tests/qtest/migration: Add a test for the analyze-migration script

Nikolay Borisov (1):
  migration: Add the configuration vmstate to the json writer

 migration/migration.c        |  1 +
 migration/savevm.c           | 20 ++++++++---
 scripts/analyze-migration.py | 67 ++++++++++++++++++++++++++++++++----
 tests/qtest/meson.build      |  2 ++
 tests/qtest/migration-test.c | 60 ++++++++++++++++++++++++++++++++
 5 files changed, 139 insertions(+), 11 deletions(-)

-- 
2.35.3

[PATCH v2 1/6] migration: Add the configuration vmstate to the json writer

[Fabiano Rosas]

From: Nikolay Borisov <nborisov@suse.com>

Make the migration json writer part of MigrationState struct, allowing
the 'configuration' object be serialized to json.

This will facilitate the parsing of the 'configuration' object in the
next patch that fixes analyze-migration.py for arm.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
farosas: rewrote the commit message. The previous one was tied to
fixed-ram.
---
 migration/migration.c |  1 +
 migration/savevm.c    | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 585d3c8f55..dde8471f83 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1430,6 +1430,7 @@ int migrate_init(MigrationState *s, Error **errp)
     error_free(s->error);
     s->error = NULL;
     s->hostname = NULL;
+    s->vmdesc = NULL;
 
     migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 60eec7c31f..5343cbc234 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1217,13 +1217,27 @@ void qemu_savevm_non_migratable_list(strList **reasons)
 
 void qemu_savevm_state_header(QEMUFile *f)
 {
+    MigrationState *s = migrate_get_current();
+
+    s->vmdesc = json_writer_new(false);
+
     trace_savevm_state_header();
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
-    if (migrate_get_current()->send_configuration) {
+    if (s->send_configuration) {
         qemu_put_byte(f, QEMU_VM_CONFIGURATION);
-        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
+
+        /*
+         * This starts the main json object and is paired with the
+         * json_writer_end_object in
+         * qemu_savevm_state_complete_precopy_non_iterable
+         */
+        json_writer_start_object(s->vmdesc, NULL);
+
+        json_writer_start_object(s->vmdesc, "configuration");
+        vmstate_save_state(f, &vmstate_configuration, &savevm_state, s->vmdesc);
+        json_writer_end_object(s->vmdesc);
     }
 }
 
@@ -1272,8 +1286,6 @@ void qemu_savevm_state_setup(QEMUFile *f)
     Error *local_err = NULL;
     int ret;
 
-    ms->vmdesc = json_writer_new(false);
-    json_writer_start_object(ms->vmdesc, NULL);
     json_writer_int64(ms->vmdesc, "page_size", qemu_target_page_size());
     json_writer_start_array(ms->vmdesc, "devices");
 
-- 
2.35.3

[PATCH v2 2/6] migration: Fix analyze-migration.py 'configuration' parsing

[Fabiano Rosas]

The 'configuration' state subsections are currently not being parsed
and the script fails when analyzing an aarch64 stream:

Traceback (most recent call last):
  File "./scripts/analyze-migration.py", line 625, in <module>
    dump.read(dump_memory = args.memory)
  File "./scripts/analyze-migration.py", line 571, in read
    raise Exception("Unknown section type: %d" % section_type)
Exception: Unknown section type: 5

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/analyze-migration.py | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 082424558b..24687db497 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -261,12 +261,21 @@ def getDict(self):
 
 
 class ConfigurationSection(object):
-    def __init__(self, file):
+    def __init__(self, file, desc):
         self.file = file
+        self.desc = desc
 
     def read(self):
-        name_len = self.file.read32()
-        name = self.file.readstr(len = name_len)
+        if self.desc:
+            version_id = self.desc['version']
+            section = VMSDSection(self.file, version_id, self.desc,
+                                  'configuration')
+            section.read()
+        else:
+            # backward compatibility for older streams that don't have
+            # the configuration section in the json
+            name_len = self.file.read32()
+            name = self.file.readstr(len = name_len)
 
 class VMSDFieldGeneric(object):
     def __init__(self, desc, file):
@@ -532,7 +541,8 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
             if section_type == self.QEMU_VM_EOF:
                 break
             elif section_type == self.QEMU_VM_CONFIGURATION:
-                section = ConfigurationSection(file)
+                config_desc = self.vmsd_desc.get('configuration')
+                section = ConfigurationSection(file, config_desc)
                 section.read()
             elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL:
                 section_id = file.read32()
-- 
2.35.3

[PATCH v2 3/6] migration: Add capability parsing to analyze-migration.py

[Fabiano Rosas]

The script is broken when the configuration/capabilities section is
present. Add support for parsing the capabilities so we can fix it in
the next patch.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/analyze-migration.py | 38 ++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 24687db497..c700fed64d 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -264,6 +264,24 @@ class ConfigurationSection(object):
     def __init__(self, file, desc):
         self.file = file
         self.desc = desc
+        self.caps = []
+
+    def parse_capabilities(self, vmsd_caps):
+        if not vmsd_caps:
+            return
+
+        ncaps = vmsd_caps.data['caps_count'].data
+        self.caps = vmsd_caps.data['capabilities']
+
+        if type(self.caps) != list:
+            self.caps = [self.caps]
+
+        if len(self.caps) != ncaps:
+            raise Exception("Number of capabilities doesn't match "
+                            "caps_count field")
+
+    def has_capability(self, cap):
+        return any([str(c) == cap for c in self.caps])
 
     def read(self):
         if self.desc:
@@ -271,6 +289,8 @@ def read(self):
             section = VMSDSection(self.file, version_id, self.desc,
                                   'configuration')
             section.read()
+            self.parse_capabilities(
+                section.data.get("configuration/capabilities"))
         else:
             # backward compatibility for older streams that don't have
             # the configuration section in the json
@@ -297,6 +317,23 @@ def read(self):
         self.data = self.file.readvar(size)
         return self.data
 
+class VMSDFieldCap(object):
+    def __init__(self, desc, file):
+        self.file = file
+        self.desc = desc
+        self.data = ""
+
+    def __repr__(self):
+        return self.data
+
+    def __str__(self):
+        return self.data
+
+    def read(self):
+        len = self.file.read8()
+        self.data = self.file.readstr(len)
+
+
 class VMSDFieldInt(VMSDFieldGeneric):
     def __init__(self, desc, file):
         super(VMSDFieldInt, self).__init__(desc, file)
@@ -471,6 +508,7 @@ def getDict(self):
     "unused_buffer" : VMSDFieldGeneric,
     "bitmap" : VMSDFieldGeneric,
     "struct" : VMSDFieldStruct,
+    "capability": VMSDFieldCap,
     "unknown" : VMSDFieldGeneric,
 }
 
-- 
2.35.3

[PATCH v2 4/6] migration: Fix analyze-migration.py when ignore-shared is used

[Fabiano Rosas]

The script is currently broken when the x-ignore-shared capability is
used:

Traceback (most recent call last):
  File "./scripts/analyze-migration.py", line 656, in <module>
    dump.read(dump_memory = args.memory)
  File "./scripts/analyze-migration.py", line 593, in read
    section.read()
  File "./scripts/analyze-migration.py", line 163, in read
    self.name = self.file.readstr(len = namelen)
  File "./scripts/analyze-migration.py", line 53, in readstr
    return self.readvar(len).decode('utf-8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 55: invalid start byte

We're currently adding data to the middle of the ram section depending
on the presence of the capability. As a consequence, any code loading
the ram section needs to know about capabilities so it can interpret
the stream.

Skip the byte that's added when x-ignore-shared is used to fix the
script.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/analyze-migration.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index c700fed64d..56ab04dd2d 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -123,6 +123,7 @@ def __init__(self, file, version_id, ramargs, section_key):
         self.TARGET_PAGE_SIZE = ramargs['page_size']
         self.dump_memory = ramargs['dump_memory']
         self.write_memory = ramargs['write_memory']
+        self.ignore_shared = ramargs['ignore_shared']
         self.sizeinfo = collections.OrderedDict()
         self.data = collections.OrderedDict()
         self.data['section sizes'] = self.sizeinfo
@@ -169,6 +170,8 @@ def read(self):
                         f.truncate(0)
                         f.truncate(len)
                         self.files[self.name] = f
+                    if self.ignore_shared:
+                        mr_addr = self.file.read64()
                 flags &= ~self.RAM_SAVE_FLAG_MEM_SIZE
 
             if flags & self.RAM_SAVE_FLAG_COMPRESS:
@@ -572,6 +575,7 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
         ramargs['page_size'] = self.vmsd_desc['page_size']
         ramargs['dump_memory'] = dump_memory
         ramargs['write_memory'] = write_memory
+        ramargs['ignore_shared'] = False
         self.section_classes[('ram',0)][1] = ramargs
 
         while True:
@@ -582,6 +586,7 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
                 config_desc = self.vmsd_desc.get('configuration')
                 section = ConfigurationSection(file, config_desc)
                 section.read()
+                ramargs['ignore_shared'] = section.has_capability('x-ignore-shared')
             elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL:
                 section_id = file.read32()
                 name = file.readstr()
-- 
2.35.3

[PATCH v2 5/6] migration: Fix analyze-migration read operation signedness

[Fabiano Rosas]

The migration code uses unsigned values for 16, 32 and 64-bit
operations. Fix the script to do the same.

This was causing an issue when parsing the migration stream generated
on the ppc64 target because one of instance_ids was larger than the
32bit signed maximum:

Traceback (most recent call last):
  File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 658, in <module>
    dump.read(dump_memory = args.memory)
  File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 592, in read
    classdesc = self.section_classes[section_key]
KeyError: ('spapr_iommu', -2147483648)

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 scripts/analyze-migration.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 56ab04dd2d..de506cb8bf 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -38,13 +38,13 @@ def __init__(self, filename):
         self.file = open(self.filename, "rb")
 
     def read64(self):
-        return int.from_bytes(self.file.read(8), byteorder='big', signed=True)
+        return int.from_bytes(self.file.read(8), byteorder='big', signed=False)
 
     def read32(self):
-        return int.from_bytes(self.file.read(4), byteorder='big', signed=True)
+        return int.from_bytes(self.file.read(4), byteorder='big', signed=False)
 
     def read16(self):
-        return int.from_bytes(self.file.read(2), byteorder='big', signed=True)
+        return int.from_bytes(self.file.read(2), byteorder='big', signed=False)
 
     def read8(self):
         return int.from_bytes(self.file.read(1), byteorder='big', signed=True)
-- 
2.35.3

[PATCH v2 6/6] tests/qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Add a smoke test that migrates to a file and gives it to the
script. It should catch the most annoying errors such as changes in
the ram flags.

After code has been merged it becomes way harder to figure out what is
causing the script to fail, the person making the change is the most
likely to know right away what the problem is.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/meson.build      |  2 ++
 tests/qtest/migration-test.c | 60 ++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 1fba07f4ed..5e82eccb62 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -356,6 +356,8 @@ foreach dir : target_dirs
     test_deps += [qsd]
   endif
 
+  qtest_env.set('PYTHON', python.full_path())
+
   foreach test : target_qtests
     # Executables are shared across targets, declare them only the first time we
     # encounter them
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 46f1c275a2..cb1b6e890c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -66,6 +66,8 @@ static bool got_dst_resume;
  */
 #define DIRTYLIMIT_TOLERANCE_RANGE  25  /* MB/s */
 
+#define ANALYZE_SCRIPT "scripts/analyze-migration.py"
+
 #if defined(__linux__)
 #include <sys/syscall.h>
 #include <sys/vfs.h>
@@ -1510,6 +1512,61 @@ static void test_baddest(void)
     test_migrate_end(from, to, false);
 }
 
+#ifndef _WIN32
+static void test_analyze_script(void)
+{
+    MigrateStart args = {
+        .opts_source = "-uuid 11111111-1111-1111-1111-111111111111",
+    };
+    QTestState *from, *to;
+    g_autofree char *uri = NULL;
+    g_autofree char *file = NULL;
+    int pid, wstatus;
+    const char *python = g_getenv("PYTHON");
+
+    if (!python) {
+        g_test_skip("PYTHON variable not set");
+        return;
+    }
+
+    /* dummy url */
+    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
+        return;
+    }
+
+    /*
+     * Setting these two capabilities causes the "configuration"
+     * vmstate to include subsections for them. The script needs to
+     * parse those subsections properly.
+     */
+    migrate_set_capability(from, "validate-uuid", true);
+    migrate_set_capability(from, "x-ignore-shared", true);
+
+    file = g_strdup_printf("%s/migfile", tmpfs);
+    uri = g_strdup_printf("exec:cat > %s", file);
+
+    migrate_ensure_converge(from);
+    migrate_qmp(from, uri, "{}");
+    wait_for_migration_complete(from);
+
+    pid = fork();
+    if (!pid) {
+        close(1);
+        open("/dev/null", O_WRONLY);
+        execl(python, python, ANALYZE_SCRIPT, "-f", file, NULL);
+        g_assert_not_reached();
+    }
+
+    g_assert(waitpid(pid, &wstatus, 0) == pid);
+    if (WIFEXITED(wstatus) && WEXITSTATUS(wstatus) != 0) {
+        g_test_message("Failed to analyze the migration stream");
+        g_test_fail();
+    }
+    test_migrate_end(from, to, false);
+    cleanup("migfile");
+}
+#endif
+
 static void test_precopy_common(MigrateCommon *args)
 {
     QTestState *from, *to;
@@ -2844,6 +2901,9 @@ int main(int argc, char **argv)
     }
 
     qtest_add_func("/migration/bad_dest", test_baddest);
+#ifndef _WIN32
+    qtest_add_func("/migration/analyze-script", test_analyze_script);
+#endif
     qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
     qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
     /*
-- 
2.35.3

Re: [PATCH v2 6/6] tests/qtest/migration: Add a test for the analyze-migration script

[Thomas Huth]

On 09/10/2023 20.43, Fabiano Rosas wrote:
> Add a smoke test that migrates to a file and gives it to the
> script. It should catch the most annoying errors such as changes in
> the ram flags.
> 
> After code has been merged it becomes way harder to figure out what is
> causing the script to fail, the person making the change is the most
> likely to know right away what the problem is.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   tests/qtest/meson.build      |  2 ++
>   tests/qtest/migration-test.c | 60 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 62 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 0/6] scripts/migration: Fix analyze-migration.py and add a test

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> was: [PATCH] qtest/migration: Add a test for the analyze-migration script
> https://lore.kernel.org/r/20230927214756.14117-1-farosas@suse.de
>
> The analyze-migration.py script should be kept in sync with the code
> that generates the migration stream. The addition/removal of sections
> and flags from the stream can cause the script to break. Issues when
> parsing the stream mostly manifest in the form of cryptic python
> errors such as:

Well, not _that_ cryptic. =)

I meant to add this:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 55:
invalid start byte

Re: [PATCH v2 1/6] migration: Add the configuration vmstate to the json writer

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> From: Nikolay Borisov <nborisov@suse.com>
>
> Make the migration json writer part of MigrationState struct, allowing
> the 'configuration' object be serialized to json.
>
> This will facilitate the parsing of the 'configuration' object in the
> next patch that fixes analyze-migration.py for arm.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

queued.

>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> -        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
> +
> +        /*
> +         * This starts the main json object and is paired with the
> +         * json_writer_end_object in
> +         * qemu_savevm_state_complete_precopy_non_iterable
> +         */
> +        json_writer_start_object(s->vmdesc, NULL);

This don't depend of this patch, but it is ugly as hell.

Can we create:

json_write_start_main_object(s->vmdesc);

(equivalent for end)

And forbid json_writer_start_object() for taking a NULL parameter?

Later, Juan.

Re: [PATCH v2 2/6] migration: Fix analyze-migration.py 'configuration' parsing

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The 'configuration' state subsections are currently not being parsed
> and the script fails when analyzing an aarch64 stream:
>
> Traceback (most recent call last):
>   File "./scripts/analyze-migration.py", line 625, in <module>
>     dump.read(dump_memory = args.memory)
>   File "./scripts/analyze-migration.py", line 571, in read
>     raise Exception("Unknown section type: %d" % section_type)
> Exception: Unknown section type: 5
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Re: [PATCH v2 3/6] migration: Add capability parsing to analyze-migration.py

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The script is broken when the configuration/capabilities section is
> present. Add support for parsing the capabilities so we can fix it in
> the next patch.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Re: [PATCH v2 4/6] migration: Fix analyze-migration.py when ignore-shared is used

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The script is currently broken when the x-ignore-shared capability is
> used:
>
> Traceback (most recent call last):
>   File "./scripts/analyze-migration.py", line 656, in <module>
>     dump.read(dump_memory = args.memory)
>   File "./scripts/analyze-migration.py", line 593, in read
>     section.read()
>   File "./scripts/analyze-migration.py", line 163, in read
>     self.name = self.file.readstr(len = namelen)
>   File "./scripts/analyze-migration.py", line 53, in readstr
>     return self.readvar(len).decode('utf-8')
> UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 55: invalid start byte
>
> We're currently adding data to the middle of the ram section depending
> on the presence of the capability. As a consequence, any code loading
> the ram section needs to know about capabilities so it can interpret
> the stream.
>
> Skip the byte that's added when x-ignore-shared is used to fix the
> script.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

> @@ -582,6 +586,7 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>                  config_desc = self.vmsd_desc.get('configuration')
>                  section = ConfigurationSection(file, config_desc)
>                  section.read()
> +                ramargs['ignore_shared'] = section.has_capability('x-ignore-shared')

should we consider s/x-ignore-shared/ignore-shared/?

>              elif section_type == self.QEMU_VM_SECTION_START or section_type == self.QEMU_VM_SECTION_FULL:
>                  section_id = file.read32()
>                  name = file.readstr()

Re: [PATCH v2 5/6] migration: Fix analyze-migration read operation signedness

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> The migration code uses unsigned values for 16, 32 and 64-bit
> operations. Fix the script to do the same.
>
> This was causing an issue when parsing the migration stream generated
> on the ppc64 target because one of instance_ids was larger than the
> 32bit signed maximum:
>
> Traceback (most recent call last):
>   File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 658, in <module>
>     dump.read(dump_memory = args.memory)
>   File "/home/fabiano/kvm/qemu/build/scripts/analyze-migration.py", line 592, in read
>     classdesc = self.section_classes[section_key]
> KeyError: ('spapr_iommu', -2147483648)
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Re: [PATCH v2 6/6] tests/qtest/migration: Add a test for the analyze-migration script

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Add a smoke test that migrates to a file and gives it to the
> script. It should catch the most annoying errors such as changes in
> the ram flags.
>
> After code has been merged it becomes way harder to figure out what is
> causing the script to fail, the person making the change is the most
> likely to know right away what the problem is.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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


> ---
>  tests/qtest/meson.build      |  2 ++
>  tests/qtest/migration-test.c | 60 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 1fba07f4ed..5e82eccb62 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -356,6 +356,8 @@ foreach dir : target_dirs
>      test_deps += [qsd]
>    endif
>  
> +  qtest_env.set('PYTHON', python.full_path())
> +

I accept it, but I think that this part of the test should be in a
different patch so meson people could comment O:-)

> +
> +    pid = fork();

live and see, g_test and qemu don't have a function to execute a script?
Wow.

Re: [PATCH v2 4/6] migration: Fix analyze-migration.py when ignore-shared is used

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> The script is currently broken when the x-ignore-shared capability is
>> used:
>>
>> Traceback (most recent call last):
>>   File "./scripts/analyze-migration.py", line 656, in <module>
>>     dump.read(dump_memory = args.memory)
>>   File "./scripts/analyze-migration.py", line 593, in read
>>     section.read()
>>   File "./scripts/analyze-migration.py", line 163, in read
>>     self.name = self.file.readstr(len = namelen)
>>   File "./scripts/analyze-migration.py", line 53, in readstr
>>     return self.readvar(len).decode('utf-8')
>> UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 55: invalid start byte
>>
>> We're currently adding data to the middle of the ram section depending
>> on the presence of the capability. As a consequence, any code loading
>> the ram section needs to know about capabilities so it can interpret
>> the stream.
>>
>> Skip the byte that's added when x-ignore-shared is used to fix the
>> script.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>> @@ -582,6 +586,7 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>>                  config_desc = self.vmsd_desc.get('configuration')
>>                  section = ConfigurationSection(file, config_desc)
>>                  section.read()
>> +                ramargs['ignore_shared'] = section.has_capability('x-ignore-shared')
>
> should we consider s/x-ignore-shared/ignore-shared/?
>

We can consider s/ignore-shared/x-ignore-shared/ if that's what you
mean. The way you suggested doesn't work because the cap name comes from
QEMU with the "x-" part in it.

If you meant filtering the x out when parsing the capabilities in this
script, I think that would cause a sort of a UX issue because we need to
use x-ignore-shared to set the cap in QMP/HMP.

Re: [PATCH v2 1/6] migration: Add the configuration vmstate to the json writer

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> From: Nikolay Borisov <nborisov@suse.com>
>>
>> Make the migration json writer part of MigrationState struct, allowing
>> the 'configuration' object be serialized to json.
>>
>> This will facilitate the parsing of the 'configuration' object in the
>> next patch that fixes analyze-migration.py for arm.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> queued.
>
>>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
>> -        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>> +
>> +        /*
>> +         * This starts the main json object and is paired with the
>> +         * json_writer_end_object in
>> +         * qemu_savevm_state_complete_precopy_non_iterable
>> +         */
>> +        json_writer_start_object(s->vmdesc, NULL);
>
> This don't depend of this patch, but it is ugly as hell.
>
> Can we create:
>
> json_write_start_main_object(s->vmdesc);
>
> (equivalent for end)
>
> And forbid json_writer_start_object() for taking a NULL parameter?
>
> Later, Juan.

Yep, I'll look into it.

Re: [PATCH v2 6/6] tests/qtest/migration: Add a test for the analyze-migration script

[Fabiano Rosas]

Juan Quintela <quintela@redhat.com> writes:

> Fabiano Rosas <farosas@suse.de> wrote:
>> Add a smoke test that migrates to a file and gives it to the
>> script. It should catch the most annoying errors such as changes in
>> the ram flags.
>>
>> After code has been merged it becomes way harder to figure out what is
>> causing the script to fail, the person making the change is the most
>> likely to know right away what the problem is.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
>
>> ---
>>  tests/qtest/meson.build      |  2 ++
>>  tests/qtest/migration-test.c | 60 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 62 insertions(+)
>>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 1fba07f4ed..5e82eccb62 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -356,6 +356,8 @@ foreach dir : target_dirs
>>      test_deps += [qsd]
>>    endif
>>  
>> +  qtest_env.set('PYTHON', python.full_path())
>> +
>
> I accept it, but I think that this part of the test should be in a
> different patch so meson people could comment O:-)
>
>> +
>> +    pid = fork();
>
> live and see, g_test and qemu don't have a function to execute a script?

gtest has one to execute a _function_ in a fork, but fork+exec no.

Re: [PATCH v2 4/6] migration: Fix analyze-migration.py when ignore-shared is used

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> wrote:
>>> The script is currently broken when the x-ignore-shared capability is
>>> used:
>>>
>>> Traceback (most recent call last):
>>>   File "./scripts/analyze-migration.py", line 656, in <module>
>>>     dump.read(dump_memory = args.memory)
>>>   File "./scripts/analyze-migration.py", line 593, in read
>>>     section.read()
>>>   File "./scripts/analyze-migration.py", line 163, in read
>>>     self.name = self.file.readstr(len = namelen)
>>>   File "./scripts/analyze-migration.py", line 53, in readstr
>>>     return self.readvar(len).decode('utf-8')
>>> UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 55: invalid start byte
>>>
>>> We're currently adding data to the middle of the ram section depending
>>> on the presence of the capability. As a consequence, any code loading
>>> the ram section needs to know about capabilities so it can interpret
>>> the stream.
>>>
>>> Skip the byte that's added when x-ignore-shared is used to fix the
>>> script.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>>> @@ -582,6 +586,7 @@ def read(self, desc_only = False, dump_memory = False, write_memory = False):
>>>                  config_desc = self.vmsd_desc.get('configuration')
>>>                  section = ConfigurationSection(file, config_desc)
>>>                  section.read()
>>> +                ramargs['ignore_shared'] = section.has_capability('x-ignore-shared')
>>
>> should we consider s/x-ignore-shared/ignore-shared/?
>>
>
> We can consider s/ignore-shared/x-ignore-shared/ if that's what you
> mean. The way you suggested doesn't work because the cap name comes from
> QEMU with the "x-" part in it.
>
> If you meant filtering the x out when parsing the capabilities in this
> script, I think that would cause a sort of a UX issue because we need to
> use x-ignore-shared to set the cap in QMP/HMP.

No.

I mean if we should start supporting ignore-shared.

Later, Juan.