[PATCH v2 0/9] RFC: migration: check required entries and sections are loaded

[PATCH v2 0/9] RFC: migration: check required entries and sections are loaded

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Surprisingly, the migration code doesn't check that required migration entries
and subsections are loaded. Either optional or required sections are both
ignored when missing. According to the documentation a "newer QEMU that knows
about a subsection can (with care) load a stream from an older QEMU that didn't
send the subsection". I propose this behaviour to be limited to "optional"
sections only.

This series has a few preliminary fixes, add new checks that entries are
loaded once and required ones have been loaded, add some tests and
documentation update.

thanks

v2:
 - add "migration: rename vmstate_save_needed->vmstate_section_needed"
 - add "migration: set file error on subsection loading"
 - add subsection tests
 - update the documentation

Marc-André Lureau (9):
  block/fdc: 'phase' is not needed on load
  virtio: make endian_needed() work during loading
  net/slirp: use different IDs for each instance
  migration: rename vmstate_save_needed->vmstate_section_needed
  migration: check required subsections are loaded, once
  migration: check required entries are loaded, once
  migration: set file error on subsection loading
  test-vmstate: add some subsection tests
  docs/migration: reflect the changes about needed subsections

 docs/devel/migration.rst    |  17 +++---
 include/migration/vmstate.h |   2 +-
 hw/block/fdc.c              |   5 ++
 hw/virtio/virtio.c          |   6 +-
 migration/savevm.c          |  45 +++++++++++++-
 migration/vmstate.c         |  45 ++++++++++++--
 net/slirp.c                 |   3 +-
 tests/unit/test-vmstate.c   | 116 ++++++++++++++++++++++++++++++++++++
 8 files changed, 222 insertions(+), 17 deletions(-)

-- 
2.41.0

[PATCH v2 1/9] block/fdc: 'phase' is not needed on load

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

It is reconstructed during fdc_post_load()

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/block/fdc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d7cc4d3ec1..fc71660ba0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1005,6 +1005,11 @@ static bool fdc_phase_needed(void *opaque)
 {
     FDCtrl *fdctrl = opaque;
 
+    /* not needed on load */
+    if (fdctrl->phase == FD_PHASE_RECONSTRUCT) {
+        return false;
+    }
+
     return reconstruct_phase(fdctrl) != fdctrl->phase;
 }
 
-- 
2.41.0

[PATCH v2 2/9] virtio: make endian_needed() work during loading

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

There is no simple way to distinguish when the callback is used for load
or save, AFAICT.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/virtio.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6facd64fbc..8baf9f7ca8 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2502,7 +2502,11 @@ static bool virtio_device_endian_needed(void *opaque)
 {
     VirtIODevice *vdev = opaque;
 
-    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    /* On load, endian is UNKNOWN. The section might be loaded as required. */
+    if (vdev->device_endian == VIRTIO_DEVICE_ENDIAN_UNKNOWN) {
+        return false;
+    }
+
     if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         return vdev->device_endian != virtio_default_endian();
     }
-- 
2.41.0

[PATCH v2 3/9] net/slirp: use different IDs for each instance

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Using always 0, QEMU will end up loading the same instance, even if
multiple have been saved.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/slirp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..af1451b60f 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -47,6 +47,7 @@
 #include "util.h"
 #include "migration/register.h"
 #include "migration/qemu-file-types.h"
+#include "migration/vmstate.h"
 
 static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 {
@@ -659,7 +660,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
      * specific version?
      */
     g_assert(slirp_state_version() == 4);
-    register_savevm_live("slirp", 0, slirp_state_version(),
+    register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY, slirp_state_version(),
                          &savevm_slirp_state, s->slirp);
 
     s->poll_notifier.notify = net_slirp_poll_notify;
-- 
2.41.0

[PATCH v2 4/9] migration: rename vmstate_save_needed->vmstate_section_needed

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The function is used on save at this point. The following commits will
use it on load.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/migration/vmstate.h | 2 +-
 migration/savevm.c          | 2 +-
 migration/vmstate.c         | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..1af181877c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1202,7 +1202,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                          void *opaque, JSONWriter *vmdesc,
                          int version_id, Error **errp);
 
-bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
+bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque);
 
 #define  VMSTATE_INSTANCE_ID_ANY  -1
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 8622f229e5..ca5c7cebe0 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -985,7 +985,7 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
     if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
         return 0;
     }
-    if (se->vmsd && !vmstate_save_needed(se->vmsd, se->opaque)) {
+    if (se->vmsd && !vmstate_section_needed(se->vmsd, se->opaque)) {
         trace_savevm_section_skip(se->idstr, se->section_id);
         return 0;
     }
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 1cf9e45b85..16e33a5d34 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -324,7 +324,7 @@ static void vmsd_desc_field_end(const VMStateDescription *vmsd,
 }
 
 
-bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
+bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
 {
     if (vmsd->needed && !vmsd->needed(opaque)) {
         /* optional section not needed */
@@ -522,7 +522,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
 
     trace_vmstate_subsection_save_top(vmsd->name);
     while (sub && *sub) {
-        if (vmstate_save_needed(*sub, opaque)) {
+        if (vmstate_section_needed(*sub, opaque)) {
             const VMStateDescription *vmsdsub = *sub;
             uint8_t len;
 
-- 
2.41.0

[PATCH v2 5/9] migration: check required subsections are loaded, once

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check that required subsections have been loaded.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 16e33a5d34..d6fe38a5e1 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -451,22 +451,51 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 }
 
 static const VMStateDescription *
-vmstate_get_subsection(const VMStateDescription **sub, char *idstr)
+vmstate_get_subsection(const VMStateDescription **sub, char *idstr, bool *visited)
 {
+    size_t i = 0;
+
     while (sub && *sub) {
         if (strcmp(idstr, (*sub)->name) == 0) {
+            if (visited[i]) {
+                return NULL;
+            }
+            visited[i] = true;
             return *sub;
         }
+        i++;
         sub++;
     }
     return NULL;
 }
 
+static size_t
+vmstate_get_n_subsections(const VMStateDescription **sub)
+{
+    size_t n = 0;
+
+    if (!sub) {
+        return 0;
+    }
+
+    while (sub[n]) {
+        n++;
+    }
+
+    return n;
+}
+
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque)
 {
+    size_t i, n;
+    g_autofree bool *visited = NULL;
+
     trace_vmstate_subsection_load(vmsd->name);
 
+    n = vmstate_get_n_subsections(vmsd->subsections);
+    visited = g_new0(bool, n);
+
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256], *idstr_ret;
         int ret;
@@ -492,7 +521,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
             /* it doesn't have a valid subsection name */
             return 0;
         }
-        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
+        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr, visited);
         if (sub_vmsd == NULL) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
             return -ENOENT;
@@ -509,6 +538,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         }
     }
 
+    for (i = 0; i < n; i++) {
+        if (!visited[i] && vmstate_section_needed(vmsd->subsections[i], opaque)) {
+            trace_vmstate_subsection_load_bad(vmsd->name, vmsd->subsections[i]->name, "(not visited)");
+            return -ENOENT;
+        }
+    }
+
     trace_vmstate_subsection_load_good(vmsd->name);
     return 0;
 }
-- 
2.41.0

[PATCH v2 6/9] migration: check required entries are loaded, once

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/savevm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index ca5c7cebe0..66c9c3095b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -217,6 +217,7 @@ typedef struct SaveStateEntry {
     void *opaque;
     CompatEntry *compat;
     int is_ram;
+    bool visited;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -1739,6 +1740,36 @@ int qemu_save_device_state(QEMUFile *f)
     return qemu_file_get_error(f);
 }
 
+static void savevm_reset_visited(void)
+{
+    SaveStateEntry *se;
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        se->visited = false;
+    }
+}
+
+static bool loadvm_check_visited(Error **errp)
+{
+    SaveStateEntry *se;
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (se->ops && se->ops->is_active && !se->ops->is_active(se->opaque)) {
+            continue;
+        }
+        if (se->vmsd && !vmstate_section_needed(se->vmsd, se->opaque)) {
+            continue;
+        }
+        if (!se->visited) {
+            error_setg(errp, "Missing entry '%s' while loading VM", se->idstr);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
 static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
 {
     SaveStateEntry *se;
@@ -2541,6 +2572,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
                      idstr, instance_id);
         return -EINVAL;
     }
+    if (se->visited) {
+        error_report("error while loading state for instance 0x%"PRIx32" of"
+                     " device '%s'", instance_id, idstr);
+        return -EINVAL;
+    }
 
     /* Validate version */
     if (version_id > se->version_id) {
@@ -2567,6 +2603,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
         return -EINVAL;
     }
 
+    se->visited = true;
+
     return 0;
 }
 
@@ -2874,7 +2912,12 @@ int qemu_loadvm_state(QEMUFile *f)
 
     cpu_synchronize_all_pre_loadvm();
 
+    savevm_reset_visited();
     ret = qemu_loadvm_state_main(f, mis);
+    if (!loadvm_check_visited(&local_err)) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
     qemu_event_set(&mis->main_thread_load_event);
 
     trace_qemu_loadvm_state_post_main(ret);
-- 
2.41.0

[PATCH v2 7/9] migration: set file error on subsection loading

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

commit 13cde50889237 ("vmstate: Return error in case of error") sets
QemuFile error to stop reading from it and report to the caller (checked
by unit tests). We should do the same on subsection loading error.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 migration/vmstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index d6fe38a5e1..be2193158f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -179,6 +179,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
     assert(field->flags == VMS_END);
     ret = vmstate_subsection_load(f, vmsd, opaque);
     if (ret != 0) {
+        qemu_file_set_error(f, ret);
         return ret;
     }
     if (vmsd->post_load) {
-- 
2.41.0

[PATCH v2 8/9] test-vmstate: add some subsection tests

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check subsection support, and optional handling.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index 0b7d5ecd68..7f4a02b4c0 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -1479,6 +1479,118 @@ static void test_tmp_struct(void)
     g_assert_cmpint(obj.f, ==, 8); /* From the child->parent */
 }
 
+static bool sub_optional_needed = true;
+
+static bool sub_optional_needed_cb(void *opaque)
+{
+    return sub_optional_needed;
+}
+
+static const VMStateDescription vmstate_sub_optional_a = {
+    .name = "sub/optional/a",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = sub_optional_needed_cb,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_sub_optional = {
+    .name = "sub/optional",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_sub_optional_a,
+    }
+};
+
+static uint8_t wire_sub_optional[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static uint8_t wire_sub_optional_missing[] = {
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_needed(void)
+{
+    sub_optional_needed = true;
+    save_vmstate(&vmstate_sub_optional, NULL);
+
+    compare_vmstate(wire_sub_optional, sizeof(wire_sub_optional));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional,
+                             sizeof(wire_sub_optional)));
+
+    FAILURE(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional_missing,
+                             sizeof(wire_sub_optional_missing)));
+
+}
+
+static void test_sub_optional_missing(void)
+{
+    sub_optional_needed = false;
+    save_vmstate(&vmstate_sub_optional, NULL);
+
+    compare_vmstate(wire_sub_optional_missing, sizeof(wire_sub_optional_missing));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional,
+                             sizeof(wire_sub_optional)));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional_missing,
+                             sizeof(wire_sub_optional_missing)));
+}
+
+static uint8_t wire_sub_dup[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_dup(void)
+{
+    sub_optional_needed = false;
+
+    FAILURE(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_dup,
+                             sizeof(wire_sub_dup)));
+}
+
+static uint8_t wire_sub_unknown[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'b',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_unknown(void)
+{
+    sub_optional_needed = false;
+
+    FAILURE(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_unknown,
+                             sizeof(wire_sub_unknown)));
+}
+
 int main(int argc, char **argv)
 {
     g_autofree char *temp_file = g_strdup_printf("%s/vmst.test.XXXXXX",
@@ -1519,6 +1631,10 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/qlist/save/saveqlist", test_save_qlist);
     g_test_add_func("/vmstate/qlist/load/loadqlist", test_load_qlist);
     g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
+    g_test_add_func("/vmstate/subsection/needed", test_sub_optional_needed);
+    g_test_add_func("/vmstate/subsection/missing", test_sub_optional_missing);
+    g_test_add_func("/vmstate/subsection/dup", test_sub_optional_dup);
+    g_test_add_func("/vmstate/subsection/unknown", test_sub_optional_unknown);
     g_test_run();
 
     close(temp_fd);
-- 
2.41.0

[PATCH v2 9/9] docs/migration: reflect the changes about needed subsections

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 docs/devel/migration.rst | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index c3e1400c0c..50f313f178 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -240,17 +240,16 @@ a newer form of device, or adding that state that you previously
 forgot to migrate.  This is best solved using a subsection.
 
 A subsection is "like" a device vmstate, but with a particularity, it
-has a Boolean function that tells if that values are needed to be sent
-or not.  If this functions returns false, the subsection is not sent.
-Subsections have a unique name, that is looked for on the receiving
-side.
+has a Boolean function that tells if that values are needed or not. If
+this functions returns false, the subsection is not sent. Subsections
+have a unique name, that is looked for on the receiving side.
 
 On the receiving side, if we found a subsection for a device that we
-don't understand, we just fail the migration.  If we understand all
-the subsections, then we load the state with success.  There's no check
-that a subsection is loaded, so a newer QEMU that knows about a subsection
-can (with care) load a stream from an older QEMU that didn't send
-the subsection.
+don't understand, we just fail the migration. If we understand all the
+subsections, then we load the state with success. There's no check
+that an optional subsection is loaded, so a newer QEMU that knows
+about a subsection can (with care) load a stream from an older QEMU
+that didn't send the subsection.
 
 If the new data is only needed in a rare case, then the subsection
 can be made conditional on that case and the migration will still
-- 
2.41.0

Re: [PATCH v2 3/9] net/slirp: use different IDs for each instance

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Using always 0, QEMU will end up loading the same instance, even if
> multiple have been saved.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Similar (but different) fix on next Migration PULL request.

Later, Juan.

> ---
>  net/slirp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index c33b3e02e7..af1451b60f 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -47,6 +47,7 @@
>  #include "util.h"
>  #include "migration/register.h"
>  #include "migration/qemu-file-types.h"
> +#include "migration/vmstate.h"
>  
>  static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>  {
> @@ -659,7 +660,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
>       * specific version?
>       */
>      g_assert(slirp_state_version() == 4);
> -    register_savevm_live("slirp", 0, slirp_state_version(),
> +    register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY, slirp_state_version(),
>                           &savevm_slirp_state, s->slirp);
>  
>      s->poll_notifier.notify = net_slirp_poll_notify;

Re: [PATCH v2 7/9] migration: set file error on subsection loading

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> commit 13cde50889237 ("vmstate: Return error in case of error") sets
> QemuFile error to stop reading from it and report to the caller (checked
> by unit tests). We should do the same on subsection loading error.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

queued.

Re: [PATCH v2 4/9] migration: rename vmstate_save_needed->vmstate_section_needed

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function is used on save at this point. The following commits will
> use it on load.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

queued.

Re: [PATCH v2 5/9] migration: check required subsections are loaded, once

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Check that required subsections have been loaded.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

I will let other people to comment on this before merging.

I can see the (pontential problem) that Peter said: We still don't have
enough state.

But I can also see the problem that you are trying to fix: A needed
subsection didn't came.

> @@ -492,7 +521,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>              /* it doesn't have a valid subsection name */
>              return 0;
>          }
> -        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> +        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr, visited);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
>              return -ENOENT;

I fully agree that a given subsection shouldn't be loaded more than once.
The part needed for this can get in at any point.



> @@ -509,6 +538,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          }
>      }
>  
> +    for (i = 0; i < n; i++) {
> +        if (!visited[i] && vmstate_section_needed(vmsd->subsections[i], opaque)) {
> +            trace_vmstate_subsection_load_bad(vmsd->name, vmsd->subsections[i]->name, "(not visited)");
> +            return -ENOENT;
> +        }
> +    }
> +
>      trace_vmstate_subsection_load_good(vmsd->name);
>      return 0;
>  }

This part is the only one where I can see there could be some
discussion.  So I wil wait to see what other people think.

Later, Juan.

Re: [PATCH v2 6/9] migration: check required entries are loaded, once

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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


> @@ -2541,6 +2572,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
>                       idstr, instance_id);
>          return -EINVAL;
>      }
> +    if (se->visited) {
> +        error_report("error while loading state for instance 0x%"PRIx32" of"
> +                     " device '%s'", instance_id, idstr);
> +        return -EINVAL;
> +    }

When this is a subsection (always) It would be a good idea to know what
section we are talking about. but not sure how easy is to get that information.

Later, Juan.

Re: [PATCH v2 8/9] test-vmstate: add some subsection tests

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Check subsection support, and optional handling.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

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

Re: [PATCH v2 9/9] docs/migration: reflect the changes about needed subsections

[Juan Quintela]

marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/devel/migration.rst | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index c3e1400c0c..50f313f178 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -240,17 +240,16 @@ a newer form of device, or adding that state that you previously
>  forgot to migrate.  This is best solved using a subsection.
>  
>  A subsection is "like" a device vmstate, but with a particularity, it
> -has a Boolean function that tells if that values are needed to be sent
> -or not.  If this functions returns false, the subsection is not sent.
> -Subsections have a unique name, that is looked for on the receiving
> -side.
> +has a Boolean function that tells if that values are needed or not. If
> +this functions returns false, the subsection is not sent. Subsections
> +have a unique name, that is looked for on the receiving side.
>  
>  On the receiving side, if we found a subsection for a device that we
> -don't understand, we just fail the migration.  If we understand all
> -the subsections, then we load the state with success.  There's no check
> -that a subsection is loaded, so a newer QEMU that knows about a subsection
> -can (with care) load a stream from an older QEMU that didn't send
> -the subsection.
> +don't understand, we just fail the migration. If we understand all the
> +subsections, then we load the state with success. There's no check
> +that an optional subsection is loaded, so a newer QEMU that knows
> +about a subsection can (with care) load a stream from an older QEMU
> +that didn't send the subsection.

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

Just wondering.  What device propmted you to write this series?

Later, Juan.

Re: [PATCH v2 9/9] docs/migration: reflect the changes about needed subsections

[Marc-André Lureau]

Hi

On Tue, Oct 24, 2023 at 2:47 PM Juan Quintela <quintela@redhat.com> wrote:
>
> marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  docs/devel/migration.rst | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> > index c3e1400c0c..50f313f178 100644
> > --- a/docs/devel/migration.rst
> > +++ b/docs/devel/migration.rst
> > @@ -240,17 +240,16 @@ a newer form of device, or adding that state that you previously
> >  forgot to migrate.  This is best solved using a subsection.
> >
> >  A subsection is "like" a device vmstate, but with a particularity, it
> > -has a Boolean function that tells if that values are needed to be sent
> > -or not.  If this functions returns false, the subsection is not sent.
> > -Subsections have a unique name, that is looked for on the receiving
> > -side.
> > +has a Boolean function that tells if that values are needed or not. If
> > +this functions returns false, the subsection is not sent. Subsections
> > +have a unique name, that is looked for on the receiving side.
> >
> >  On the receiving side, if we found a subsection for a device that we
> > -don't understand, we just fail the migration.  If we understand all
> > -the subsections, then we load the state with success.  There's no check
> > -that a subsection is loaded, so a newer QEMU that knows about a subsection
> > -can (with care) load a stream from an older QEMU that didn't send
> > -the subsection.
> > +don't understand, we just fail the migration. If we understand all the
> > +subsections, then we load the state with success. There's no check
> > +that an optional subsection is loaded, so a newer QEMU that knows
> > +about a subsection can (with care) load a stream from an older QEMU
> > +that didn't send the subsection.
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Just wondering.  What device propmted you to write this series?

When I worked on ramfb, I did various testing with the subsection
handling and was surprised by the default lazy behaviour. I initially
thought it was a bug to ignore missing sections (both needed and
not-needed), then I realize from the doc that it was partially by
design. I thought it was clearer to make "needed' section actually
required on load as well. I wonder though of the potential of breakage
from old QEMU versions, how do we test cross-version migration? Do you
think also "needed" section are actually required? Perhaps we need
better wording and documentation instead...

Re: [PATCH v2 9/9] docs/migration: reflect the changes about needed subsections

[Juan Quintela]

Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Hi
>
> On Tue, Oct 24, 2023 at 2:47 PM Juan Quintela <quintela@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com wrote:
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  docs/devel/migration.rst | 17 ++++++++---------
>> >  1 file changed, 8 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
>> > index c3e1400c0c..50f313f178 100644
>> > --- a/docs/devel/migration.rst
>> > +++ b/docs/devel/migration.rst
>> > @@ -240,17 +240,16 @@ a newer form of device, or adding that state that you previously
>> >  forgot to migrate.  This is best solved using a subsection.
>> >
>> >  A subsection is "like" a device vmstate, but with a particularity, it
>> > -has a Boolean function that tells if that values are needed to be sent
>> > -or not.  If this functions returns false, the subsection is not sent.
>> > -Subsections have a unique name, that is looked for on the receiving
>> > -side.
>> > +has a Boolean function that tells if that values are needed or not. If
>> > +this functions returns false, the subsection is not sent. Subsections
>> > +have a unique name, that is looked for on the receiving side.
>> >
>> >  On the receiving side, if we found a subsection for a device that we
>> > -don't understand, we just fail the migration.  If we understand all
>> > -the subsections, then we load the state with success.  There's no check
>> > -that a subsection is loaded, so a newer QEMU that knows about a subsection
>> > -can (with care) load a stream from an older QEMU that didn't send
>> > -the subsection.
>> > +don't understand, we just fail the migration. If we understand all the
>> > +subsections, then we load the state with success. There's no check
>> > +that an optional subsection is loaded, so a newer QEMU that knows
>> > +about a subsection can (with care) load a stream from an older QEMU
>> > +that didn't send the subsection.
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> Just wondering.  What device propmted you to write this series?
>
> When I worked on ramfb, I did various testing with the subsection
> handling and was surprised by the default lazy behaviour. I initially
> thought it was a bug to ignore missing sections (both needed and
> not-needed), then I realize from the doc that it was partially by
> design. I thought it was clearer to make "needed' section actually
> required on load as well. I wonder though of the potential of breakage
> from old QEMU versions, how do we test cross-version migration? Do you
> think also "needed" section are actually required? Perhaps we need
> better wording and documentation instead...

This was designed for a world that no longer exists.
We used to "promise" that we will allow migration for:

$ qemu-n -M pc -> qemu-(n+1) -M pc

And pray that it worked.

We have dropped that long ago (as imposible to do/test).  And now we
only promise that:

$ qemu-n -M pc-n -> qemu-(n+1) -M pc-n

And in this case, making the needed versions required looks like a good
idea to me.

As said, waiting for others to chime in.
The changes that are independent are already on my queue.

Thanks, Juan.

Re: [PATCH v2 5/9] migration: check required subsections are loaded, once

[Peter Xu]

On Tue, Oct 24, 2023 at 12:41:56PM +0200, Juan Quintela wrote:
> > @@ -509,6 +538,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >          }
> >      }
> >  
> > +    for (i = 0; i < n; i++) {
> > +        if (!visited[i] && vmstate_section_needed(vmsd->subsections[i], opaque)) {
> > +            trace_vmstate_subsection_load_bad(vmsd->name, vmsd->subsections[i]->name, "(not visited)");
> > +            return -ENOENT;
> > +        }
> > +    }
> > +
> >      trace_vmstate_subsection_load_good(vmsd->name);
> >      return 0;
> >  }
> 
> This part is the only one where I can see there could be some
> discussion.  So I wil wait to see what other people think.

Previous email:

https://lore.kernel.org/qemu-devel/ZR2P1RbxCfBdYBaQ@x1n/

I still think it is safer to not fail unless justified that we won't hit
surprises in the ->needed().  There are a lot so I assume it's non-trivial
to justify.

We can switch the tracepoint into error_report() in that case, though, as
long as it won't fail the migration.

Thanks,

-- 
Peter Xu