[Qemu-devel] [PATCH 0/5] vmstate: handle arrays with null ptrs

[Qemu-devel] [PATCH 0/5] vmstate: handle arrays with null ptrs

[Halil Pasic]

This is basically the next iteration of the patch set "[RFC PATCH v2 0/8]
VMS_ARRAY_OF_POINTER with null pointers". Mostly a rebase on top of
current master. I have kept the r-b's for patches which remained
conceptually the same.  If someone disagrees please complain.

RFC v2 -> v1:
* rebase
* changed marker as suggested by Dave (incl. simplifications)
* fixed issues pointed out by Dave
* reworded some commit messages
* added a test for array of ptr to primitive

Halil Pasic (5):
  migration/vmstate: renames in (load|save)_state
  migration/vmstate: split up vmstate_base_addr
  migration/vmstate: fix array of ptr with nullptrs
  tests/test-vmstate.c: test array of ptr with null
  tests/test-vmstate.c: test array of ptr to primitive

 include/migration/vmstate.h |   4 ++
 migration/vmstate.c         |  90 ++++++++++++++++++++++-------------
 tests/test-vmstate.c        | 113 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 33 deletions(-)

-- 
2.8.4

[Qemu-devel] [PATCH 1/5] migration/vmstate: renames in (load|save)_state

[Halil Pasic]

The vmstate_(load|save)_state start out with an a void *opaque pointing
to some struct, and manipulate one or more elements of one field within
that struct.

First the field within the struct is pinpointed as opaque + offset, then
if this is a pointer the pointer is dereferenced to obtain a pointer to
the first element of the vmstate field. Pointers to further elements if
any are calculated as first_element + i * element_size (where i is the
zero based index of the element in question).

Currently base_addr and addr is used as a variable name for the pointer
to the first element and the pointer to the current element being
processed. This is suboptimal because base_addr is somewhat
counter-intuitive (because obtained as base + offset) and both base_addr
and addr not very descriptive (that we have a pointer should be clear
from the fact that it is declared as a pointer).

Let make things easier to understand by renaming base_addr to first_elem
and addr to curr_elem. This has the additional benefit of harmonizing
with other names within the scope (n_elems, vmstate_n_elems).

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---

This patch roots in the difficulties I faced when trying to figure out
the code in question. In the meanwhile I'm quite fine with the current
names because I have it already figured out (after a couple of months
not looking on this code however, who knows). Thus my main motivation
is making things easier for the next new guy, but if the old guys say
not worth it I can very well accept that too.
---
 migration/vmstate.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index b4d8ae9..36efa80 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -116,21 +116,21 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
              field->field_exists(opaque, version_id)) ||
             (!field->field_exists &&
              field->version_id <= version_id)) {
-            void *base_addr = vmstate_base_addr(opaque, field, true);
+            void *first_elem = vmstate_base_addr(opaque, field, true);
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
 
             for (i = 0; i < n_elems; i++) {
-                void *addr = base_addr + size * i;
+                void *curr_elem = first_elem + size * i;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
-                    addr = *(void **)addr;
+                    curr_elem = *(void **)curr_elem;
                 }
                 if (field->flags & VMS_STRUCT) {
-                    ret = vmstate_load_state(f, field->vmsd, addr,
+                    ret = vmstate_load_state(f, field->vmsd, curr_elem,
                                              field->vmsd->version_id);
                 } else {
-                   ret = field->info->get(f, addr, size, field);
+                    ret = field->info->get(f, curr_elem, size, field);
                 }
                 if (ret >= 0) {
                     ret = qemu_file_get_error(f);
@@ -321,7 +321,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
     while (field->name) {
         if (!field->field_exists ||
             field->field_exists(opaque, vmsd->version_id)) {
-            void *base_addr = vmstate_base_addr(opaque, field, false);
+            void *first_elem = vmstate_base_addr(opaque, field, false);
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
             int64_t old_offset, written_bytes;
@@ -329,18 +329,18 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
 
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             for (i = 0; i < n_elems; i++) {
-                void *addr = base_addr + size * i;
+                void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
                 old_offset = qemu_ftell_fast(f);
-
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
-                    addr = *(void **)addr;
+                    assert(curr_elem);
+                    curr_elem = *(void **)curr_elem;
                 }
                 if (field->flags & VMS_STRUCT) {
-                    vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+                    vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
                 } else {
-                    field->info->put(f, addr, size, field, vmdesc_loop);
+                    field->info->put(f, curr_elem, size, field, vmdesc_loop);
                 }
 
                 written_bytes = qemu_ftell_fast(f) - old_offset;
-- 
2.8.4

[Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr

[Halil Pasic]

Currently vmstate_base_addr does several things: it pinpoints the field
within the struct, possibly allocates memory and possibly does the first
pointer dereference. Obviously allocation is needed only for load.

Let us split up the functionality in vmstate_base_addr and move the
address manipulations (that is everything but the allocation logic) to
load and save so it becomes more obvious what is actually going on. Like
this all the address calculations (and the handling of the flags
controlling these) is in one place and the sequence is more obvious.

The newly introduced function vmstate_handle_alloc also fixes the
allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is
substantially simpler than the original vmstate_base_addr.

In load and save some asserts are added so it's easier to debug
situations where we would end up with a null pointer dereference.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
 migration/vmstate.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 36efa80..836a7a4 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
     return size;
 }
 
-static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
+static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)
 {
-    void *base_addr = opaque + field->offset;
-
-    if (field->flags & VMS_POINTER) {
-        if (alloc && (field->flags & VMS_ALLOC)) {
-            gsize size = 0;
-            if (field->flags & VMS_VBUFFER) {
-                size = vmstate_size(opaque, field);
-            } else {
-                int n_elems = vmstate_n_elems(opaque, field);
-                if (n_elems) {
-                    size = n_elems * field->size;
-                }
-            }
-            if (size) {
-                *(void **)base_addr = g_malloc(size);
-            }
+    if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
+        gsize size = vmstate_size(opaque, field);
+        size *= vmstate_n_elems(opaque, field);
+        if (size) {
+            *(void **)ptr = g_malloc(size);
         }
-        base_addr = *(void **)base_addr;
     }
-
-    return base_addr;
 }
 
 int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
@@ -116,10 +102,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
              field->field_exists(opaque, version_id)) ||
             (!field->field_exists &&
              field->version_id <= version_id)) {
-            void *first_elem = vmstate_base_addr(opaque, field, true);
+            void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
 
+            vmstate_handle_alloc(first_elem, field, opaque);
+            if (field->flags & VMS_POINTER) {
+                first_elem = *(void **)first_elem;
+                assert(first_elem  || !n_elems);
+            }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
 
@@ -321,13 +312,17 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
     while (field->name) {
         if (!field->field_exists ||
             field->field_exists(opaque, vmsd->version_id)) {
-            void *first_elem = vmstate_base_addr(opaque, field, false);
+            void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
             int64_t old_offset, written_bytes;
             QJSON *vmdesc_loop = vmdesc;
 
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
+            if (field->flags & VMS_POINTER) {
+                first_elem = *(void **)first_elem;
+                assert(first_elem  || !n_elems);
+            }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
 
-- 
2.8.4

[Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Halil Pasic]

Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the
reward for trying to migrate an array with some null pointers in it was
an illegal memory access, that is a swift and painless death of the
process.  Let's make vmstate cope with this scenario.

The general approach is, when we encounter a null pointer (element),
instead of following the pointer to save/load the data behind it, we
save/load a placeholder. This way we can detect if we expected a null
pointer at the load side but not null data was saved instead.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>

---
We will need this to load/save some on demand created state in the
(s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
an example).
---
 include/migration/vmstate.h |  4 ++++
 migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 63e7b02..f2dbf84 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16;
 extern const VMStateInfo vmstate_info_uint32;
 extern const VMStateInfo vmstate_info_uint64;
 
+/** Put this in the stream when migrating a null pointer.*/
+#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
+extern const VMStateInfo vmstate_info_nullptr;
+
 extern const VMStateInfo vmstate_info_float64;
 extern const VMStateInfo vmstate_info_cpudouble;
 
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 836a7a4..cb81cef 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     curr_elem = *(void **)curr_elem;
                 }
-                if (field->flags & VMS_STRUCT) {
+                if (!curr_elem) {
+                    /* if null pointer check placeholder and do not follow */
+                    assert(field->flags & VMS_ARRAY_OF_POINTER);
+                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
+                } else if (field->flags & VMS_STRUCT) {
                     ret = vmstate_load_state(f, field->vmsd, curr_elem,
                                              field->vmsd->version_id);
                 } else {
@@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
                 }
-                if (field->flags & VMS_STRUCT) {
+                if (!curr_elem) {
+                    /* if null pointer write placeholder and do not follow */
+                    assert(field->flags & VMS_ARRAY_OF_POINTER);
+                    vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
+                } else if (field->flags & VMS_STRUCT) {
                     vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
                 } else {
                     field->info->put(f, curr_elem, size, field, vmdesc_loop);
@@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
     .put  = put_uint64,
 };
 
+static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
+
+{
+    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
+}
+
+static int put_nullptr(QEMUFile *f, void *pv, size_t size,
+                        VMStateField *field, QJSON *vmdesc)
+
+{
+    assert(pv == NULL);
+    qemu_put_byte(f, VMS_NULLPTR_MARKER);
+    return 0;
+}
+
+const VMStateInfo vmstate_info_nullptr = {
+    .name = "uint64",
+    .get  = get_nullptr,
+    .put  = put_nullptr,
+};
+
 /* 64 bit unsigned int. See that the received value is the same than the one
    in the field */
 
-- 
2.8.4

[Qemu-devel] [PATCH 4/5] tests/test-vmstate.c: test array of ptr with null

[Halil Pasic]

Add test for VMSTATE_ARRAY_OF_POINTER_TO_STRUCT with an array
containing some null pointer.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/test-vmstate.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d0dd390..b68a0b3 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -487,6 +487,8 @@ const VMStateDescription vmsd_tst = {
     }
 };
 
+/* test array migration */
+
 #define AR_SIZE 4
 
 typedef struct {
@@ -542,6 +544,52 @@ static void test_arr_ptr_str_no0_load(void)
     }
 }
 
+static void test_arr_ptr_str_0_save(void)
+{
+    TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+    TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        VMS_NULLPTR_MARKER,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_vmstate(&vmsd_arps, &sample);
+    compare_vmstate(wire_sample, sizeof(wire_sample));
+}
+
+static void test_arr_ptr_str_0_load(void)
+{
+    TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
+    TestStructTriv ar[AR_SIZE] = {};
+    TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+    int idx;
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        VMS_NULLPTR_MARKER,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_buffer(wire_sample, sizeof(wire_sample));
+    SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+                          wire_sample, sizeof(wire_sample)));
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        /* compare the target array ar with the ground truth array ar_gt */
+        g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
+    }
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        if (idx == 1) {
+            g_assert_cmpint((uint64_t)(obj.ar[idx]), ==, 0);
+        } else {
+            g_assert_cmpint((uint64_t)(obj.ar[idx]), !=, 0);
+        }
+    }
+}
+
 /* test QTAILQ migration */
 typedef struct TestQtailqElement TestQtailqElement;
 
@@ -792,6 +840,9 @@ int main(int argc, char **argv)
                     test_arr_ptr_str_no0_save);
     g_test_add_func("/vmstate/array/ptr/str/no0/load",
                     test_arr_ptr_str_no0_load);
+    g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
+    g_test_add_func("/vmstate/array/ptr/str/0/load",
+                    test_arr_ptr_str_0_load);
     g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
     g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
     g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
-- 
2.8.4

[Qemu-devel] [PATCH 5/5] tests/test-vmstate.c: test array of ptr to primitive

[Halil Pasic]

Let's have a test for ptr arrays to some primitive type with some
not-null and null ptrs intermixed.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>

---

Mainly for the sake of completeness and also to demonstrate that it works
since in the previous version I wrongly stated it does not. If guys think
we do not need this, I'm happy with just dropping it.
---
 tests/test-vmstate.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index b68a0b3..82ab743 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -590,6 +590,64 @@ static void test_arr_ptr_str_0_load(void)
     }
 }
 
+typedef struct TestArrayOfPtrToInt {
+    int32_t *ar[AR_SIZE];
+} TestArrayOfPtrToInt;
+
+const VMStateDescription vmsd_arpp = {
+    .name = "test/arps",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_ARRAY_OF_POINTER(ar, TestArrayOfPtrToInt,
+                AR_SIZE, 0, vmstate_info_int32, int32_t*),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void test_arr_ptr_prim_0_save(void)
+{
+    int32_t ar[AR_SIZE] = {0 , 1, 2, 3};
+    TestArrayOfPtrToInt  sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        VMS_NULLPTR_MARKER,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_vmstate(&vmsd_arpp, &sample);
+    compare_vmstate(wire_sample, sizeof(wire_sample));
+}
+
+static void test_arr_ptr_prim_0_load(void)
+{
+    int32_t ar_gt[AR_SIZE] = {0, 1, 2, 3};
+    int32_t ar[AR_SIZE] = {3 , 42, 1, 0};
+    TestArrayOfPtrToInt obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+    int idx;
+    uint8_t wire_sample[] = {
+        0x00, 0x00, 0x00, 0x00,
+        VMS_NULLPTR_MARKER,
+        0x00, 0x00, 0x00, 0x02,
+        0x00, 0x00, 0x00, 0x03,
+        QEMU_VM_EOF
+    };
+
+    save_buffer(wire_sample, sizeof(wire_sample));
+    SUCCESS(load_vmstate_one(&vmsd_arpp, &obj, 1,
+                          wire_sample, sizeof(wire_sample)));
+    for (idx = 0; idx < AR_SIZE; ++idx) {
+        /* compare the target array ar with the ground truth array ar_gt */
+        if (idx == 1) {
+            g_assert_cmpint(42, ==, ar[idx]);
+        } else {
+            g_assert_cmpint(ar_gt[idx], ==, ar[idx]);
+        }
+    }
+}
+
 /* test QTAILQ migration */
 typedef struct TestQtailqElement TestQtailqElement;
 
@@ -843,6 +901,10 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
     g_test_add_func("/vmstate/array/ptr/str/0/load",
                     test_arr_ptr_str_0_load);
+    g_test_add_func("/vmstate/array/ptr/prim/0/save",
+                    test_arr_ptr_prim_0_save);
+    g_test_add_func("/vmstate/array/ptr/prim/0/load",
+                    test_arr_ptr_prim_0_load);
     g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
     g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
     g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
-- 
2.8.4

Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the
> reward for trying to migrate an array with some null pointers in it was
> an illegal memory access, that is a swift and painless death of the
> process.  Let's make vmstate cope with this scenario.
> 
> The general approach is, when we encounter a null pointer (element),
> instead of following the pointer to save/load the data behind it, we
> save/load a placeholder. This way we can detect if we expected a null
> pointer at the load side but not null data was saved instead.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> 
> ---
> We will need this to load/save some on demand created state in the
> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
> an example).
> ---
>  include/migration/vmstate.h |  4 ++++
>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 63e7b02..f2dbf84 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16;
>  extern const VMStateInfo vmstate_info_uint32;
>  extern const VMStateInfo vmstate_info_uint64;
>  
> +/** Put this in the stream when migrating a null pointer.*/
> +#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
> +extern const VMStateInfo vmstate_info_nullptr;
> +
>  extern const VMStateInfo vmstate_info_float64;
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 836a7a4..cb81cef 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>                      curr_elem = *(void **)curr_elem;
>                  }
> -                if (field->flags & VMS_STRUCT) {
> +                if (!curr_elem) {
> +                    /* if null pointer check placeholder and do not follow */
> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);

That can return an error instead of asserting.

> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);

You've ignored the return value of the get; that should be  ret = 

> +                } else if (field->flags & VMS_STRUCT) {
>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>                                               field->vmsd->version_id);
>                  } else {
> @@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                      assert(curr_elem);
>                      curr_elem = *(void **)curr_elem;
>                  }
> -                if (field->flags & VMS_STRUCT) {
> +                if (!curr_elem) {
> +                    /* if null pointer write placeholder and do not follow */
> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> +                    vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
> +                } else if (field->flags & VMS_STRUCT) {
>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>                  } else {
>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>      .put  = put_uint64,
>  };
>  
> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> +
> +{
> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> +}
> +
> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
> +                        VMStateField *field, QJSON *vmdesc)
> +
> +{
> +    assert(pv == NULL);
> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
> +    return 0;
> +}

Again that assert could just turn into a return -EINVAL

Dave

> +
> +const VMStateInfo vmstate_info_nullptr = {
> +    .name = "uint64",
> +    .get  = get_nullptr,
> +    .put  = put_nullptr,
> +};
> +
>  /* 64 bit unsigned int. See that the received value is the same than the one
>     in the field */
>  
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Halil Pasic]

On 02/17/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the
>> reward for trying to migrate an array with some null pointers in it was
>> an illegal memory access, that is a swift and painless death of the
>> process.  Let's make vmstate cope with this scenario.
>>
>> The general approach is, when we encounter a null pointer (element),
>> instead of following the pointer to save/load the data behind it, we
>> save/load a placeholder. This way we can detect if we expected a null
>> pointer at the load side but not null data was saved instead.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
>>
>> ---
>> We will need this to load/save some on demand created state in the
>> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
>> an example).
>> ---
>>  include/migration/vmstate.h |  4 ++++
>>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 63e7b02..f2dbf84 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16;
>>  extern const VMStateInfo vmstate_info_uint32;
>>  extern const VMStateInfo vmstate_info_uint64;
>>  
>> +/** Put this in the stream when migrating a null pointer.*/
>> +#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
>> +extern const VMStateInfo vmstate_info_nullptr;
>> +
>>  extern const VMStateInfo vmstate_info_float64;
>>  extern const VMStateInfo vmstate_info_cpudouble;
>>  
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 836a7a4..cb81cef 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>>                      curr_elem = *(void **)curr_elem;
>>                  }
>> -                if (field->flags & VMS_STRUCT) {
>> +                if (!curr_elem) {
>> +                    /* if null pointer check placeholder and do not follow */
>> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> 
> That can return an error instead of asserting.
> 
>> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> 
> You've ignored the return value of the get; that should be  ret = 
> 
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>>                                               field->vmsd->version_id);
>>                  } else {
>> @@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>                      assert(curr_elem);
>>                      curr_elem = *(void **)curr_elem;
>>                  }
>> -                if (field->flags & VMS_STRUCT) {
>> +                if (!curr_elem) {
>> +                    /* if null pointer write placeholder and do not follow */
>> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
>> +                    vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>>                  } else {
>>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
>> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>>      .put  = put_uint64,
>>  };
>>  
>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>> +
>> +{
>> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>> +}
>> +
>> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>> +                        VMStateField *field, QJSON *vmdesc)
>> +
>> +{
>> +    assert(pv == NULL);
>> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
>> +    return 0;
>> +}
> 
> Again that assert could just turn into a return -EINVAL
> 
> Dave
> 

@Dave: I have accidentally answered with 'reply' instead of 'reply all'
yesterday. Sorry for the noise!

Thanks for the review! You are right, my code is messy.

Yet I'm not sure what you propose is the best way to clean it up. My
concern is, that we are loosing some info about what exactly went wrong
(and where), if returning -EINVAL is not combined with additional
logging/error reporting. 

After re-checking the asserts they all indicate programming errors and
there is not much (IMHO) client code can do to recover and the user
should never observe. Unfortunately, I lack knowledge about how is the
client code handling errors and if there is something we need to clean
up. I assumed asserting is OK because of the per-existing asserts.

You are perfectly right about ignoring the return value of
vmstate_info_nullptr.get and at least I will have to fix that.
IMHO we have three options to fix this code:
a) Make vmstate_info_nullptr.get assert too as we expect null pointer
and we got not null is pretty much an indicator that we can't proceed
sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
b) Follow your suggestions without anything on top of it.
c) Follow your suggestions and add error_report stating what exactly
went wrong.

My priority is to get this in, so I will do what you say, or send
option b) late on Wednesday if no guidance until then.

Regards,
Halil

>> +
>> +const VMStateInfo vmstate_info_nullptr = {
>> +    .name = "uint64",
>> +    .get  = get_nullptr,
>> +    .put  = put_nullptr,
>> +};
>> +
>>  /* 64 bit unsigned int. See that the received value is the same than the one
>>     in the field */
>>  
>> -- 
>> 2.8.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH 4/5] tests/test-vmstate.c: test array of ptr with null

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Add test for VMSTATE_ARRAY_OF_POINTER_TO_STRUCT with an array
> containing some null pointer.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  tests/test-vmstate.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d0dd390..b68a0b3 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -487,6 +487,8 @@ const VMStateDescription vmsd_tst = {
>      }
>  };
>  
> +/* test array migration */
> +
>  #define AR_SIZE 4
>  
>  typedef struct {
> @@ -542,6 +544,52 @@ static void test_arr_ptr_str_no0_load(void)
>      }
>  }
>  
> +static void test_arr_ptr_str_0_save(void)
> +{
> +    TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
> +    TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> +    uint8_t wire_sample[] = {
> +        0x00, 0x00, 0x00, 0x00,
> +        VMS_NULLPTR_MARKER,
> +        0x00, 0x00, 0x00, 0x02,
> +        0x00, 0x00, 0x00, 0x03,
> +        QEMU_VM_EOF
> +    };
> +
> +    save_vmstate(&vmsd_arps, &sample);
> +    compare_vmstate(wire_sample, sizeof(wire_sample));
> +}
> +
> +static void test_arr_ptr_str_0_load(void)
> +{
> +    TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
> +    TestStructTriv ar[AR_SIZE] = {};
> +    TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> +    int idx;
> +    uint8_t wire_sample[] = {
> +        0x00, 0x00, 0x00, 0x00,
> +        VMS_NULLPTR_MARKER,
> +        0x00, 0x00, 0x00, 0x02,
> +        0x00, 0x00, 0x00, 0x03,
> +        QEMU_VM_EOF
> +    };

If possible; it would be better if you could share the wire_sample, and ar_gt between
the two functions rather than copying them.

Dave

> +    save_buffer(wire_sample, sizeof(wire_sample));
> +    SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
> +                          wire_sample, sizeof(wire_sample)));
> +    for (idx = 0; idx < AR_SIZE; ++idx) {
> +        /* compare the target array ar with the ground truth array ar_gt */
> +        g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
> +    }
> +    for (idx = 0; idx < AR_SIZE; ++idx) {
> +        if (idx == 1) {
> +            g_assert_cmpint((uint64_t)(obj.ar[idx]), ==, 0);
> +        } else {
> +            g_assert_cmpint((uint64_t)(obj.ar[idx]), !=, 0);
> +        }
> +    }
> +}
> +
>  /* test QTAILQ migration */
>  typedef struct TestQtailqElement TestQtailqElement;
>  
> @@ -792,6 +840,9 @@ int main(int argc, char **argv)
>                      test_arr_ptr_str_no0_save);
>      g_test_add_func("/vmstate/array/ptr/str/no0/load",
>                      test_arr_ptr_str_no0_load);
> +    g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
> +    g_test_add_func("/vmstate/array/ptr/str/0/load",
> +                    test_arr_ptr_str_0_load);
>      g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
>      g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
>      g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/5] tests/test-vmstate.c: test array of ptr with null

[Halil Pasic]

On 02/21/2017 11:49 AM, Dr. David Alan Gilbert wrote:
>> +static void test_arr_ptr_str_0_save(void)
>> +{
>> +    TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
>> +    TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
>> +    uint8_t wire_sample[] = {
>> +        0x00, 0x00, 0x00, 0x00,
>> +        VMS_NULLPTR_MARKER,
>> +        0x00, 0x00, 0x00, 0x02,
>> +        0x00, 0x00, 0x00, 0x03,
>> +        QEMU_VM_EOF
>> +    };
>> +
>> +    save_vmstate(&vmsd_arps, &sample);
>> +    compare_vmstate(wire_sample, sizeof(wire_sample));
>> +}
>> +
>> +static void test_arr_ptr_str_0_load(void)
>> +{
>> +    TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
>> +    TestStructTriv ar[AR_SIZE] = {};
>> +    TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
>> +    int idx;
>> +    uint8_t wire_sample[] = {
>> +        0x00, 0x00, 0x00, 0x00,
>> +        VMS_NULLPTR_MARKER,
>> +        0x00, 0x00, 0x00, 0x02,
>> +        0x00, 0x00, 0x00, 0x03,
>> +        QEMU_VM_EOF
>> +    };
> If possible; it would be better if you could share the wire_sample, and ar_gt between
> the two functions rather than copying them.
> 
> Dave
> 

Certainly possible. I did it this way because I prefer
unit tests as self contained as possible, but consistent
style is also important.

Will factor wire_sample out for the next version.

Cheers,
Halil

Re: [Qemu-devel] [PATCH 5/5] tests/test-vmstate.c: test array of ptr to primitive

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Let's have a test for ptr arrays to some primitive type with some
> not-null and null ptrs intermixed.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> 
> ---
> 
> Mainly for the sake of completeness and also to demonstrate that it works
> since in the previous version I wrongly stated it does not. If guys think
> we do not need this, I'm happy with just dropping it.
> ---
>  tests/test-vmstate.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index b68a0b3..82ab743 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -590,6 +590,64 @@ static void test_arr_ptr_str_0_load(void)
>      }
>  }
>  
> +typedef struct TestArrayOfPtrToInt {
> +    int32_t *ar[AR_SIZE];
> +} TestArrayOfPtrToInt;
> +
> +const VMStateDescription vmsd_arpp = {
> +    .name = "test/arps",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_ARRAY_OF_POINTER(ar, TestArrayOfPtrToInt,
> +                AR_SIZE, 0, vmstate_info_int32, int32_t*),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void test_arr_ptr_prim_0_save(void)
> +{
> +    int32_t ar[AR_SIZE] = {0 , 1, 2, 3};
> +    TestArrayOfPtrToInt  sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> +    uint8_t wire_sample[] = {
> +        0x00, 0x00, 0x00, 0x00,
> +        VMS_NULLPTR_MARKER,
> +        0x00, 0x00, 0x00, 0x02,
> +        0x00, 0x00, 0x00, 0x03,
> +        QEMU_VM_EOF
> +    };
> +
> +    save_vmstate(&vmsd_arpp, &sample);
> +    compare_vmstate(wire_sample, sizeof(wire_sample));
> +}
> +
> +static void test_arr_ptr_prim_0_load(void)
> +{
> +    int32_t ar_gt[AR_SIZE] = {0, 1, 2, 3};
> +    int32_t ar[AR_SIZE] = {3 , 42, 1, 0};
> +    TestArrayOfPtrToInt obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> +    int idx;
> +    uint8_t wire_sample[] = {
> +        0x00, 0x00, 0x00, 0x00,
> +        VMS_NULLPTR_MARKER,
> +        0x00, 0x00, 0x00, 0x02,
> +        0x00, 0x00, 0x00, 0x03,
> +        QEMU_VM_EOF
> +    };

Again you could share the wire_sample and ar_gt between these two functions,
other than that:

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

Dave

> +    save_buffer(wire_sample, sizeof(wire_sample));
> +    SUCCESS(load_vmstate_one(&vmsd_arpp, &obj, 1,
> +                          wire_sample, sizeof(wire_sample)));
> +    for (idx = 0; idx < AR_SIZE; ++idx) {
> +        /* compare the target array ar with the ground truth array ar_gt */
> +        if (idx == 1) {
> +            g_assert_cmpint(42, ==, ar[idx]);
> +        } else {
> +            g_assert_cmpint(ar_gt[idx], ==, ar[idx]);
> +        }
> +    }
> +}
> +
>  /* test QTAILQ migration */
>  typedef struct TestQtailqElement TestQtailqElement;
>  
> @@ -843,6 +901,10 @@ int main(int argc, char **argv)
>      g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
>      g_test_add_func("/vmstate/array/ptr/str/0/load",
>                      test_arr_ptr_str_0_load);
> +    g_test_add_func("/vmstate/array/ptr/prim/0/save",
> +                    test_arr_ptr_prim_0_save);
> +    g_test_add_func("/vmstate/array/ptr/prim/0/load",
> +                    test_arr_ptr_prim_0_load);
>      g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
>      g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
>      g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Currently vmstate_base_addr does several things: it pinpoints the field
> within the struct, possibly allocates memory and possibly does the first
> pointer dereference. Obviously allocation is needed only for load.
> 
> Let us split up the functionality in vmstate_base_addr and move the
> address manipulations (that is everything but the allocation logic) to
> load and save so it becomes more obvious what is actually going on. Like
> this all the address calculations (and the handling of the flags
> controlling these) is in one place and the sequence is more obvious.
> 
> The newly introduced function vmstate_handle_alloc also fixes the
> allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is
> substantially simpler than the original vmstate_base_addr.

Note that VMSTATE_VBUFFER_MULTIPLY does use VMS_VBUFFER|VMS_MULTIPLY - but
not with alloc (and I started using VMSTATE_VBUFFER_MULTIPLY in a recent
patch that went in).  I'm not sure I see what the error is that you fixed.

However, I'm ok with it:

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

> In load and save some asserts are added so it's easier to debug
> situations where we would end up with a null pointer dereference.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  migration/vmstate.c | 39 +++++++++++++++++----------------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 36efa80..836a7a4 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
>      return size;
>  }
>  
> -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
> +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)

>  {
> -    void *base_addr = opaque + field->offset;
> -
> -    if (field->flags & VMS_POINTER) {
> -        if (alloc && (field->flags & VMS_ALLOC)) {
> -            gsize size = 0;
> -            if (field->flags & VMS_VBUFFER) {
> -                size = vmstate_size(opaque, field);
> -            } else {
> -                int n_elems = vmstate_n_elems(opaque, field);
> -                if (n_elems) {
> -                    size = n_elems * field->size;
> -                }
> -            }
> -            if (size) {
> -                *(void **)base_addr = g_malloc(size);
> -            }
> +    if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
> +        gsize size = vmstate_size(opaque, field);
> +        size *= vmstate_n_elems(opaque, field);
> +        if (size) {
> +            *(void **)ptr = g_malloc(size);
>          }
> -        base_addr = *(void **)base_addr;
>      }
> -
> -    return base_addr;
>  }
>  
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> @@ -116,10 +102,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>               field->field_exists(opaque, version_id)) ||
>              (!field->field_exists &&
>               field->version_id <= version_id)) {
> -            void *first_elem = vmstate_base_addr(opaque, field, true);
> +            void *first_elem = opaque + field->offset;
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>  
> +            vmstate_handle_alloc(first_elem, field, opaque);
> +            if (field->flags & VMS_POINTER) {
> +                first_elem = *(void **)first_elem;
> +                assert(first_elem  || !n_elems);
> +            }
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>  
> @@ -321,13 +312,17 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>      while (field->name) {
>          if (!field->field_exists ||
>              field->field_exists(opaque, vmsd->version_id)) {
> -            void *first_elem = vmstate_base_addr(opaque, field, false);
> +            void *first_elem = opaque + field->offset;
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>              int64_t old_offset, written_bytes;
>              QJSON *vmdesc_loop = vmdesc;
>  
>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> +            if (field->flags & VMS_POINTER) {
> +                first_elem = *(void **)first_elem;
> +                assert(first_elem  || !n_elems);
> +            }
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>  
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 02/17/2017 08:42 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the
> >> reward for trying to migrate an array with some null pointers in it was
> >> an illegal memory access, that is a swift and painless death of the
> >> process.  Let's make vmstate cope with this scenario.
> >>
> >> The general approach is, when we encounter a null pointer (element),
> >> instead of following the pointer to save/load the data behind it, we
> >> save/load a placeholder. This way we can detect if we expected a null
> >> pointer at the load side but not null data was saved instead.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> >>
> >> ---
> >> We will need this to load/save some on demand created state in the
> >> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
> >> an example).
> >> ---
> >>  include/migration/vmstate.h |  4 ++++
> >>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
> >>  2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 63e7b02..f2dbf84 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16;
> >>  extern const VMStateInfo vmstate_info_uint32;
> >>  extern const VMStateInfo vmstate_info_uint64;
> >>  
> >> +/** Put this in the stream when migrating a null pointer.*/
> >> +#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
> >> +extern const VMStateInfo vmstate_info_nullptr;
> >> +
> >>  extern const VMStateInfo vmstate_info_float64;
> >>  extern const VMStateInfo vmstate_info_cpudouble;
> >>  
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 836a7a4..cb81cef 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
> >>                      curr_elem = *(void **)curr_elem;
> >>                  }
> >> -                if (field->flags & VMS_STRUCT) {
> >> +                if (!curr_elem) {
> >> +                    /* if null pointer check placeholder and do not follow */
> >> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> > 
> > That can return an error instead of asserting.
> > 
> >> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> > 
> > You've ignored the return value of the get; that should be  ret = 
> > 
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
> >>                                               field->vmsd->version_id);
> >>                  } else {
> >> @@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>                      assert(curr_elem);
> >>                      curr_elem = *(void **)curr_elem;
> >>                  }
> >> -                if (field->flags & VMS_STRUCT) {
> >> +                if (!curr_elem) {
> >> +                    /* if null pointer write placeholder and do not follow */
> >> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
> >> +                    vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
> >> +                } else if (field->flags & VMS_STRUCT) {
> >>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> >>                  } else {
> >>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
> >> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
> >>      .put  = put_uint64,
> >>  };
> >>  
> >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
> >> +
> >> +{
> >> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> >> +}
> >> +
> >> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
> >> +                        VMStateField *field, QJSON *vmdesc)
> >> +
> >> +{
> >> +    assert(pv == NULL);
> >> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
> >> +    return 0;
> >> +}
> > 
> > Again that assert could just turn into a return -EINVAL
> > 
> > Dave
> > 
> 
> @Dave: I have accidentally answered with 'reply' instead of 'reply all'
> yesterday. Sorry for the noise!
> 
> Thanks for the review! You are right, my code is messy.

It's not that bad - just a few minor issues.

> Yet I'm not sure what you propose is the best way to clean it up. My
> concern is, that we are loosing some info about what exactly went wrong
> (and where), if returning -EINVAL is not combined with additional
> logging/error reporting. 
>
> After re-checking the asserts they all indicate programming errors and
> there is not much (IMHO) client code can do to recover and the user
> should never observe. Unfortunately, I lack knowledge about how is the
> client code handling errors and if there is something we need to clean
> up. I assumed asserting is OK because of the per-existing asserts.

> You are perfectly right about ignoring the return value of
> vmstate_info_nullptr.get and at least I will have to fix that.
> IMHO we have three options to fix this code:
> a) Make vmstate_info_nullptr.get assert too as we expect null pointer
> and we got not null is pretty much an indicator that we can't proceed
> sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
> b) Follow your suggestions without anything on top of it.
> c) Follow your suggestions and add error_report stating what exactly
> went wrong.
> 
> My priority is to get this in, so I will do what you say, or send
> option b) late on Wednesday if no guidance until then.

It's OK to add an error_report as needed; in particular I try and avoid
assert's on the save path where possible, since the user apparently
has a happy running VM, so even if the migration code has an error in it
I'd rather the user didn't lose their VM.  The load path it's OK to
add the asserts since they've not got a working VM yet.

Dave

> 
> Regards,
> Halil
> 
> >> +
> >> +const VMStateInfo vmstate_info_nullptr = {
> >> +    .name = "uint64",
> >> +    .get  = get_nullptr,
> >> +    .put  = put_nullptr,
> >> +};
> >> +
> >>  /* 64 bit unsigned int. See that the received value is the same than the one
> >>     in the field */
> >>  
> >> -- 
> >> 2.8.4
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Halil Pasic]

On 02/21/2017 01:22 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 02/17/2017 08:42 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the
>>>> reward for trying to migrate an array with some null pointers in it was
>>>> an illegal memory access, that is a swift and painless death of the
>>>> process.  Let's make vmstate cope with this scenario.
>>>>
>>>> The general approach is, when we encounter a null pointer (element),
>>>> instead of following the pointer to save/load the data behind it, we
>>>> save/load a placeholder. This way we can detect if we expected a null
>>>> pointer at the load side but not null data was saved instead.
>>>>
>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>>> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
>>>>
>>>> ---
>>>> We will need this to load/save some on demand created state in the
>>>> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for
>>>> an example).
>>>> ---
>>>>  include/migration/vmstate.h |  4 ++++
>>>>  migration/vmstate.c         | 33 +++++++++++++++++++++++++++++++--
>>>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>> index 63e7b02..f2dbf84 100644
>>>> --- a/include/migration/vmstate.h
>>>> +++ b/include/migration/vmstate.h
>>>> @@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16;
>>>>  extern const VMStateInfo vmstate_info_uint32;
>>>>  extern const VMStateInfo vmstate_info_uint64;
>>>>  
>>>> +/** Put this in the stream when migrating a null pointer.*/
>>>> +#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
>>>> +extern const VMStateInfo vmstate_info_nullptr;
>>>> +
>>>>  extern const VMStateInfo vmstate_info_float64;
>>>>  extern const VMStateInfo vmstate_info_cpudouble;
>>>>  
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 836a7a4..cb81cef 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>>>>                      curr_elem = *(void **)curr_elem;
>>>>                  }
>>>> -                if (field->flags & VMS_STRUCT) {
>>>> +                if (!curr_elem) {
>>>> +                    /* if null pointer check placeholder and do not follow */
>>>> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
>>>
>>> That can return an error instead of asserting.
>>>
>>>> +                    vmstate_info_nullptr.get(f, curr_elem, size, NULL);
>>>
>>> You've ignored the return value of the get; that should be  ret = 
>>>
>>>> +                } else if (field->flags & VMS_STRUCT) {
>>>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>>>>                                               field->vmsd->version_id);
>>>>                  } else {
>>>> @@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>                      assert(curr_elem);
>>>>                      curr_elem = *(void **)curr_elem;
>>>>                  }
>>>> -                if (field->flags & VMS_STRUCT) {
>>>> +                if (!curr_elem) {
>>>> +                    /* if null pointer write placeholder and do not follow */
>>>> +                    assert(field->flags & VMS_ARRAY_OF_POINTER);
>>>> +                    vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
>>>> +                } else if (field->flags & VMS_STRUCT) {
>>>>                      vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>>>>                  } else {
>>>>                      field->info->put(f, curr_elem, size, field, vmdesc_loop);
>>>> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = {
>>>>      .put  = put_uint64,
>>>>  };
>>>>  
>>>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>>>> +
>>>> +{
>>>> +    return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>>>> +}
>>>> +
>>>> +static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>>>> +                        VMStateField *field, QJSON *vmdesc)
>>>> +
>>>> +{
>>>> +    assert(pv == NULL);
>>>> +    qemu_put_byte(f, VMS_NULLPTR_MARKER);
>>>> +    return 0;
>>>> +}
>>>
>>> Again that assert could just turn into a return -EINVAL
>>>
>>> Dave
>>>
>>
>> @Dave: I have accidentally answered with 'reply' instead of 'reply all'
>> yesterday. Sorry for the noise!
>>
>> Thanks for the review! You are right, my code is messy.
> 
> It's not that bad - just a few minor issues.
> 
>> Yet I'm not sure what you propose is the best way to clean it up. My
>> concern is, that we are loosing some info about what exactly went wrong
>> (and where), if returning -EINVAL is not combined with additional
>> logging/error reporting. 
>>
>> After re-checking the asserts they all indicate programming errors and
>> there is not much (IMHO) client code can do to recover and the user
>> should never observe. Unfortunately, I lack knowledge about how is the
>> client code handling errors and if there is something we need to clean
>> up. I assumed asserting is OK because of the per-existing asserts.
> 
>> You are perfectly right about ignoring the return value of
>> vmstate_info_nullptr.get and at least I will have to fix that.
>> IMHO we have three options to fix this code:
>> a) Make vmstate_info_nullptr.get assert too as we expect null pointer
>> and we got not null is pretty much an indicator that we can't proceed
>> sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts.
>> b) Follow your suggestions without anything on top of it.
>> c) Follow your suggestions and add error_report stating what exactly
>> went wrong.
>>
>> My priority is to get this in, so I will do what you say, or send
>> option b) late on Wednesday if no guidance until then.
> 
> It's OK to add an error_report as needed; in particular I try and avoid
> assert's on the save path where possible, since the user apparently
> has a happy running VM, so even if the migration code has an error in it
> I'd rather the user didn't lose their VM.  The load path it's OK to
> add the asserts since they've not got a working VM yet.
> 
> Dave
> 

You are right, distinguishing between save and load paths makes a lot
of sense. So on the load path I will use asserts, and on the save path,
that is

static int put_nullptr(QEMUFile *f, void *pv, size_t size,
                        VMStateField *field, QJSON *vmdesc)

{

here, I could do:
-    assert(pv == NULL);
+    if (pv != NULL) {
+        error_report("put_nullptr must be called with pv == NULL");
+        return -EINVAL; 
+    }
    qemu_put_byte(f, VMS_NULLPTR_MARKER);
    return 0;
}

And of course, I will fix ignoring the return value too. Would that be
satisfactory? If not, please complain.

By the way, I could also omit the check on pv, and ignore pv altogether.
A call to put_nullptr means we encountered a nullptr and the check
is just for catching buggy client code -- what might be an overkill
in this case.

Thank you very much. FYI I intend to send out the next version tomorrow.

Regards,
Halil

Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs

[Halil Pasic]

On 02/21/2017 01:55 PM, Halil Pasic wrote:
>> It's OK to add an error_report as needed; in particular I try and avoid
>> assert's on the save path where possible, since the user apparently
>> has a happy running VM, so even if the migration code has an error in it
>> I'd rather the user didn't lose their VM.  The load path it's OK to
>> add the asserts since they've not got a working VM yet.
>>
>> Dave
>>
> You are right, distinguishing between save and load paths makes a lot
> of sense. So on the load path I will use asserts, and on the save path,
> that is
> 
> static int put_nullptr(QEMUFile *f, void *pv, size_t size,
>                         VMStateField *field, QJSON *vmdesc)
> 
> {
> 
> here, I could do:
> -    assert(pv == NULL);
> +    if (pv != NULL) {
> +        error_report("put_nullptr must be called with pv == NULL");
> +        return -EINVAL; 
> +    }
>     qemu_put_byte(f, VMS_NULLPTR_MARKER);
>     return 0;
> }
> 
> And of course, I will fix ignoring the return value too. Would that be
> satisfactory? If not, please complain.
> 
> By the way, I could also omit the check on pv, and ignore pv altogether.
> A call to put_nullptr means we encountered a nullptr and the check
> is just for catching buggy client code -- what might be an overkill
> in this case.
> 
> Thank you very much. FYI I intend to send out the next version tomorrow.
> 
> Regards,
> Halil
> 
> 
> 

Just realized two things:
1. vmstate_save_state does not have a return value and does not handle
such. So I won't fail the migration from put_nullptr.
2. Since we have no rule on NDEBUG, I think  I should not rely on assert doing
anything. So where important for correctness I will do return value and
error_report. 

Re: [Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr

[Halil Pasic]

On 02/21/2017 01:07 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Currently vmstate_base_addr does several things: it pinpoints the field
>> within the struct, possibly allocates memory and possibly does the first
>> pointer dereference. Obviously allocation is needed only for load.
>>
>> Let us split up the functionality in vmstate_base_addr and move the
>> address manipulations (that is everything but the allocation logic) to
>> load and save so it becomes more obvious what is actually going on. Like
>> this all the address calculations (and the handling of the flags
>> controlling these) is in one place and the sequence is more obvious.
>>
>> The newly introduced function vmstate_handle_alloc also fixes the
>> allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is

s/VMS_VBUFFER| VMS_MULTIPLY/VMS_VBUFFER|VMS_MULTIPLY|VMS_ALLOC/

>> substantially simpler than the original vmstate_base_addr.
> 
> Note that VMSTATE_VBUFFER_MULTIPLY does use VMS_VBUFFER|VMS_MULTIPLY - but
> not with alloc (and I started using VMSTATE_VBUFFER_MULTIPLY in a recent
> patch that went in).  

Adding |VMS_ALLOC to avoid confusion. I was hoping it's clear
form the context that we are talking about scenarios where
allocation is relevant.

> I'm not sure I see what the error is that you fixed.

I'm referring to number of bytes allocated. It was
vmstate_n_elems(opaque, field) * field->size
and became
vmstate_n_elems(opaque, field) * vmstate_size(opaque, field)

vmstate_size does extra handling for VMS_VBUFFER and
VMS_MULTIPLY.


> 
> However, I'm ok with it:
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 

Cheers,
Halil

>> In load and save some asserts are added so it's easier to debug
>> situations where we would end up with a null pointer dereference.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>>  migration/vmstate.c | 39 +++++++++++++++++----------------------
>>  1 file changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 36efa80..836a7a4 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -52,29 +52,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
>>      return size;
>>  }
>>  
>> -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
>> +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)
> 
>>  {
>> -    void *base_addr = opaque + field->offset;
>> -
>> -    if (field->flags & VMS_POINTER) {
>> -        if (alloc && (field->flags & VMS_ALLOC)) {
>> -            gsize size = 0;
>> -            if (field->flags & VMS_VBUFFER) {
>> -                size = vmstate_size(opaque, field);
>> -            } else {
>> -                int n_elems = vmstate_n_elems(opaque, field);
>> -                if (n_elems) {
>> -                    size = n_elems * field->size;
>> -                }
>> -            }
>> -            if (size) {
>> -                *(void **)base_addr = g_malloc(size);
>> -            }
>> +    if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
>> +        gsize size = vmstate_size(opaque, field);
>> +        size *= vmstate_n_elems(opaque, field);
>> +        if (size) {
>> +            *(void **)ptr = g_malloc(size);
>>          }
>> -        base_addr = *(void **)base_addr;
>>      }
>> -
>> -    return base_addr;
>>  }
>>  
>>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>> @@ -116,10 +102,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>               field->field_exists(opaque, version_id)) ||
>>              (!field->field_exists &&
>>               field->version_id <= version_id)) {
>> -            void *first_elem = vmstate_base_addr(opaque, field, true);
>> +            void *first_elem = opaque + field->offset;
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>  
>> +            vmstate_handle_alloc(first_elem, field, opaque);
>> +            if (field->flags & VMS_POINTER) {
>> +                first_elem = *(void **)first_elem;
>> +                assert(first_elem  || !n_elems);
>> +            }
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>  
>> @@ -321,13 +312,17 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>      while (field->name) {
>>          if (!field->field_exists ||
>>              field->field_exists(opaque, vmsd->version_id)) {
>> -            void *first_elem = vmstate_base_addr(opaque, field, false);
>> +            void *first_elem = opaque + field->offset;
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>              int64_t old_offset, written_bytes;
>>              QJSON *vmdesc_loop = vmdesc;
>>  
>>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>> +            if (field->flags & VMS_POINTER) {
>> +                first_elem = *(void **)first_elem;
>> +                assert(first_elem  || !n_elems);
>> +            }
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>  
>> -- 
>> 2.8.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>