All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start
@ 2016-10-18 10:57 Halil Pasic
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 10:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Halil Pasic

The member VMStateField.start was solely used to implement the partial
data migration for VBUFFER data (basically provide migration for a
sub-buffer). However the implementation of this feature is broken, but
this goes unnoticed since the feature is not used at all.

So what the series does is first add some tests for VBUFFER, then add a
test which proves that the VMS_ALLOC and used together with .start != 0
is broken.  Then we immediately revert this last patch since we are
going to drop it instead of fixing it.  Lastly simplify things by
dropping VMStateField.start altogether.

An additional benefit is that .start can be re introduced to be used for
linked structures as proposed by Jianjun in "[QEMU PATCH v6 2/2]
migration: migrate QTAILQ".

Guenther Hutzl (1):
  tests/test-vmstate.c: Add vBuffer test

Halil Pasic (3):
  tests/test-vmstate.c: prove VMStateField.start broken
  Revert "tests/test-vmstate.c: prove VMStateField.start broken"
  migration: drop unused VMStateField.start

 hw/char/exynos4210_uart.c   |   2 +-
 hw/display/g364fb.c         |   2 +-
 hw/dma/pl330.c              |   8 +--
 hw/intc/exynos4210_gic.c    |   2 +-
 hw/ipmi/isa_ipmi_bt.c       |   4 +-
 hw/ipmi/isa_ipmi_kcs.c      |   4 +-
 hw/net/vmxnet3.c            |   2 +-
 hw/nvram/mac_nvram.c        |   2 +-
 hw/nvram/spapr_nvram.c      |   2 +-
 hw/sd/sdhci.c               |   2 +-
 hw/timer/m48t59.c           |   2 +-
 include/migration/vmstate.h |  20 +++-----
 migration/savevm.c          |   2 +-
 migration/vmstate.c         |   4 +-
 target-s390x/machine.c      |   2 +-
 tests/test-vmstate.c        | 115 +++++++++++++++++++++++++++++++++++++++++++-
 util/fifo8.c                |   2 +-
 17 files changed, 141 insertions(+), 36 deletions(-)

-- 
2.8.4

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test
  2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
@ 2016-10-18 10:57 ` Halil Pasic
  2016-10-20 11:52   ` Dr. David Alan Gilbert
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken Halil Pasic
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 10:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Halil Pasic

From: Guenther Hutzl <hutzl@linux.vnet.ibm.com>

The unit test test-vmstate.c is missing tests for some of the complex
vmstate macros. This patch adds a new test for VMSTATE_VBUFFER
and VMSTATE_VBUFFER_ALLOC_UINT32. The added test does not cover
start != 0 because it's broken and unused so our intention is to
remove start altogether.

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

A proof for the brokenness of start is provided in a separate patch
so it can be easily reverted or not picked (thus we separate what is
intended to stay, and what is intended to go away).
---
 tests/test-vmstate.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index d8da26f..9a57aa0 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -475,6 +475,119 @@ static void test_load_skip(void)
     qemu_fclose(loading);
 }
 
+/* vBuffer tests */
+#define BUF_SIZE 10
+
+typedef struct TestVBuffer {
+    uint8_t  u8_1;
+    int32_t  buffer_size;
+    uint8_t  *vBuffer_1;
+    uint32_t buffer_alloc_size;
+    uint8_t  *vBuffer_alloc_1;
+    uint8_t  u8_2;
+} TestVBuffer;
+
+/* buffers padded with 0xFE at the end to easier detect overflow */
+uint8_t buf1[BUF_SIZE + 1] = { 1,  2,  3,  4,  5,  6,  7,  8,  9,  0, 0xfe};
+uint8_t buf2[BUF_SIZE + 1] = { 1,  2,  3,  4,  5,  6,  7,  8,  9,  0, 0xfe};
+
+TestVBuffer obj_vbuffer = {
+    .u8_1 = 100,
+    .buffer_size = BUF_SIZE,
+    .vBuffer_1 = buf1,
+    .buffer_alloc_size = BUF_SIZE,
+    .vBuffer_alloc_1 = buf2,
+    .u8_2 = 200,
+};
+
+static const VMStateDescription vmstate_vbuffer = {
+    .name = "complex/vbuffer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(u8_1, TestVBuffer),
+        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL, 0,
+                        buffer_size),
+        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL, 0,
+                        buffer_alloc_size),
+        VMSTATE_UINT8(u8_2, TestVBuffer),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+uint8_t wire_vbuffer[] = {
+    /* u8_1 */            0x64,
+    /* vBuffer_1 */       0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
+                          0x00,
+    /* vBuffer_alloc_1 */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
+                          0x00,
+    /* u8_2 */            0xc8,
+    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
+};
+
+static void obj_vbuffer_copy(void *target, void *source)
+{
+    /* this proc copies all struct TestVBuffer entries from source to target
+       except the vBuffer pointers which should already point to the correct
+       locations. The buffer contents are also copied */
+    TestVBuffer *s = (TestVBuffer *)source;
+    TestVBuffer *t = (TestVBuffer *)target;
+
+    t->u8_1 = s->u8_1;
+    t->u8_2 = s->u8_2;
+    t->buffer_size = s->buffer_size;
+    t->buffer_alloc_size = s->buffer_alloc_size;
+    if (t->vBuffer_1 && s->vBuffer_1) {
+        memcpy(t->vBuffer_1, s->vBuffer_1, BUF_SIZE);
+    }
+    if (t->vBuffer_alloc_1 && s->vBuffer_alloc_1) {
+        memcpy(t->vBuffer_alloc_1, s->vBuffer_alloc_1, BUF_SIZE);
+    }
+}
+
+static void test_complex_vbuffer(void)
+{
+    uint8_t buffer[BUF_SIZE];
+    uint8_t buffer_clone[BUF_SIZE];
+    TestVBuffer obj = {
+        .u8_1 = 0,
+        .buffer_size = BUF_SIZE,
+        .vBuffer_1 = buffer,
+        .buffer_alloc_size = BUF_SIZE,
+        .vBuffer_alloc_1 = NULL,
+        .u8_2 = 0,
+    };
+    TestVBuffer obj_clone = {
+        .u8_1 = 0,
+        .buffer_size = BUF_SIZE,
+        .vBuffer_1 = buffer_clone,
+        .buffer_alloc_size = BUF_SIZE,
+        .vBuffer_alloc_1 = NULL,
+        .u8_2 = 0,
+    };
+
+    memset(buffer, 0, BUF_SIZE);
+    memset(buffer_clone, 0, BUF_SIZE);
+
+    save_vmstate(&vmstate_vbuffer, &obj_vbuffer);
+
+    compare_vmstate(wire_vbuffer, sizeof(wire_vbuffer));
+
+    SUCCESS(load_vmstate(&vmstate_vbuffer, &obj, &obj_clone,
+                         obj_vbuffer_copy, 1, wire_vbuffer,
+                         sizeof(wire_vbuffer)));
+
+#define FIELD_EQUAL(name)  g_assert_cmpint(obj.name, ==, obj_vbuffer.name)
+#define BUFFER_EQUAL(name) SUCCESS(memcmp(obj.name, obj_vbuffer.name, BUF_SIZE))
+
+    FIELD_EQUAL(u8_1);
+    BUFFER_EQUAL(vBuffer_1);
+    BUFFER_EQUAL(vBuffer_alloc_1);
+    FIELD_EQUAL(u8_2);
+}
+#undef FIELD_EQUAL
+#undef BUFFER_EQUAL
+
 int main(int argc, char **argv)
 {
     temp_fd = mkstemp(temp_file);
@@ -489,6 +602,7 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
     g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
     g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
+    g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer);
     g_test_run();
 
     close(temp_fd);
-- 
2.8.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
@ 2016-10-18 10:57 ` Halil Pasic
  2016-10-18 13:27   ` Dr. David Alan Gilbert
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 3/4] Revert "tests/test-vmstate.c: prove VMStateField.start broken" Halil Pasic
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 10:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Halil Pasic

The handling of VMStateField.start is currently quite broken if
VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but
fortunately also quite underutilized -- nobody is using .start != 0.

Let's prove with this patch that it's really broken (as a first
step towards fixing things up).

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

The idea is to remove .start support and this patch should
be reverted, as soon this happens, or even better just
dropped. If however dropping the support for .start encounters
resistance, this patch should prove useful in an unexpected
way.
---
 tests/test-vmstate.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 9a57aa0..a2ef4a8 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -588,6 +588,53 @@ static void test_complex_vbuffer(void)
 #undef FIELD_EQUAL
 #undef BUFFER_EQUAL
 
+typedef struct {
+    uint32_t vbuff_size;
+    uint8_t *vbuff;
+    uint64_t stuff;
+} TestVBufStart;
+
+static const VMStateDescription vmstate_vbuff_alloc_start = {
+    .name = "test/vbuff_alloc_start",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(stuff, TestVBufStart),
+        VMSTATE_UINT32(vbuff_size, TestVBufStart),
+        VMSTATE_VBUFFER_ALLOC_UINT32(vbuff, TestVBufStart, 1, 0, 1, vbuff_size),
+        VMSTATE_END_OF_LIST()
+    }
+
+};
+
+static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj,
+        int version_id)
+{
+        QEMUFile *fload = open_test_file(false);
+
+        SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id));
+        qemu_fclose(fload);
+}
+
+static void test_vbuff_alloc_start(void)
+{
+    uint8_t my_vbuff1[] = {0, 1, 2, 3};
+    uint8_t my_vbuff2[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+    TestVBufStart sample = {
+        .stuff = 1,
+        .vbuff_size = 3,
+        .vbuff = my_vbuff1,
+    };
+    TestVBufStart load_obj = {
+        .vbuff = my_vbuff2,
+    };
+
+    save_vmstate(&vmstate_vbuff_alloc_start, &sample);
+    load_vmstate_one_obj(&vmstate_vbuff_alloc_start, &load_obj, 1);
+    g_assert_cmpint(load_obj.stuff, ==, 0);
+    g_assert_cmpint((uint64_t) load_obj.vbuff, !=, (uint64_t) my_vbuff2);
+}
+
 int main(int argc, char **argv)
 {
     temp_fd = mkstemp(temp_file);
@@ -603,8 +650,8 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
     g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
     g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer);
+    g_test_add_func("/vmstate/vbuff/alloc_start", test_vbuff_alloc_start);
     g_test_run();
-
     close(temp_fd);
     unlink(temp_file);
 
-- 
2.8.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 3/4] Revert "tests/test-vmstate.c: prove VMStateField.start broken"
  2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken Halil Pasic
@ 2016-10-18 10:57 ` Halil Pasic
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start Halil Pasic
  2016-10-18 11:24 ` [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start no-reply
  4 siblings, 0 replies; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 10:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Halil Pasic

This reverts the effect of the previous patch in the series as
a preparation for getting rid of VMStateField.start.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
 tests/test-vmstate.c | 48 ------------------------------------------------
 1 file changed, 48 deletions(-)

diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index a2ef4a8..4ea64b7 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -588,53 +588,6 @@ static void test_complex_vbuffer(void)
 #undef FIELD_EQUAL
 #undef BUFFER_EQUAL
 
-typedef struct {
-    uint32_t vbuff_size;
-    uint8_t *vbuff;
-    uint64_t stuff;
-} TestVBufStart;
-
-static const VMStateDescription vmstate_vbuff_alloc_start = {
-    .name = "test/vbuff_alloc_start",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_UINT64(stuff, TestVBufStart),
-        VMSTATE_UINT32(vbuff_size, TestVBufStart),
-        VMSTATE_VBUFFER_ALLOC_UINT32(vbuff, TestVBufStart, 1, 0, 1, vbuff_size),
-        VMSTATE_END_OF_LIST()
-    }
-
-};
-
-static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj,
-        int version_id)
-{
-        QEMUFile *fload = open_test_file(false);
-
-        SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id));
-        qemu_fclose(fload);
-}
-
-static void test_vbuff_alloc_start(void)
-{
-    uint8_t my_vbuff1[] = {0, 1, 2, 3};
-    uint8_t my_vbuff2[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
-    TestVBufStart sample = {
-        .stuff = 1,
-        .vbuff_size = 3,
-        .vbuff = my_vbuff1,
-    };
-    TestVBufStart load_obj = {
-        .vbuff = my_vbuff2,
-    };
-
-    save_vmstate(&vmstate_vbuff_alloc_start, &sample);
-    load_vmstate_one_obj(&vmstate_vbuff_alloc_start, &load_obj, 1);
-    g_assert_cmpint(load_obj.stuff, ==, 0);
-    g_assert_cmpint((uint64_t) load_obj.vbuff, !=, (uint64_t) my_vbuff2);
-}
-
 int main(int argc, char **argv)
 {
     temp_fd = mkstemp(temp_file);
@@ -650,7 +603,6 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
     g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
     g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer);
-    g_test_add_func("/vmstate/vbuff/alloc_start", test_vbuff_alloc_start);
     g_test_run();
     close(temp_fd);
     unlink(temp_file);
-- 
2.8.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start
  2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
                   ` (2 preceding siblings ...)
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 3/4] Revert "tests/test-vmstate.c: prove VMStateField.start broken" Halil Pasic
@ 2016-10-18 10:57 ` Halil Pasic
  2016-10-20 12:00   ` Dr. David Alan Gilbert
  2016-10-18 11:24 ` [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start no-reply
  4 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 10:57 UTC (permalink / raw)
  To: qemu-devel; +Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Halil Pasic

The member VMStateField.start was solely used to implement the partial
data migration for VBUFFER data (basically provide migration for a
sub-buffer). However the implementation of this feature seems broken to
me, but this goes unnoticed since the feature is not used at all.
Instead of fixing it lets try simplify things by dropping it.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
---
 hw/char/exynos4210_uart.c   |  2 +-
 hw/display/g364fb.c         |  2 +-
 hw/dma/pl330.c              |  8 ++++----
 hw/intc/exynos4210_gic.c    |  2 +-
 hw/ipmi/isa_ipmi_bt.c       |  4 ++--
 hw/ipmi/isa_ipmi_kcs.c      |  4 ++--
 hw/net/vmxnet3.c            |  2 +-
 hw/nvram/mac_nvram.c        |  2 +-
 hw/nvram/spapr_nvram.c      |  2 +-
 hw/sd/sdhci.c               |  2 +-
 hw/timer/m48t59.c           |  2 +-
 include/migration/vmstate.h | 20 ++++++--------------
 migration/savevm.c          |  2 +-
 migration/vmstate.c         |  4 ++--
 target-s390x/machine.c      |  2 +-
 tests/test-vmstate.c        |  4 ++--
 util/fifo8.c                |  2 +-
 17 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 1107578..7b1b4b1 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -565,7 +565,7 @@ static const VMStateDescription vmstate_exynos4210_uart_fifo = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(sp, Exynos4210UartFIFO),
         VMSTATE_UINT32(rp, Exynos4210UartFIFO),
-        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, 0, size),
+        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, size),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
index 70ef2c7..8cdc205 100644
--- a/hw/display/g364fb.c
+++ b/hw/display/g364fb.c
@@ -464,7 +464,7 @@ static const VMStateDescription vmstate_g364fb = {
     .minimum_version_id = 1,
     .post_load = g364fb_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, 0, vram_size),
+        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
         VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
         VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
         VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index c0bd9fe..32cf839 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -173,8 +173,8 @@ static const VMStateDescription vmstate_pl330_fifo = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, 0, buf_size),
-        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, 0, buf_size),
+        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, buf_size),
+        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, buf_size),
         VMSTATE_UINT32(head, PL330Fifo),
         VMSTATE_UINT32(num, PL330Fifo),
         VMSTATE_UINT32(buf_size, PL330Fifo),
@@ -282,8 +282,8 @@ static const VMStateDescription vmstate_pl330 = {
         VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
         VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
                                      vmstate_pl330_chan, PL330Chan),
-        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, 0, num_chnls),
-        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, 0, num_chnls),
+        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
+        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
         VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
         VMSTATE_STRUCT(read_queue, PL330State, 0, vmstate_pl330_queue,
                        PL330Queue),
diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index fd7a8f3..2a55817 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -393,7 +393,7 @@ static const VMStateDescription vmstate_exynos4210_irq_gate = {
     .version_id = 2,
     .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, 0, n_in),
+        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, n_in),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index f036617..6c10ef7 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -471,9 +471,9 @@ static const VMStateDescription vmstate_ISAIPMIBTDevice = {
         VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
         VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
         VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
-        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL,
                                bt.outlen),
-        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL,
                                bt.inlen),
         VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
         VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 9a38f8a..b592d53 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -433,9 +433,9 @@ const VMStateDescription vmstate_ISAIPMIKCSDevice = {
         VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
         VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
         VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
-        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL,
                                kcs.outlen),
-        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL,
                                kcs.inlen),
         VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
         VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 90f6943..b4b71f3 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2396,7 +2396,7 @@ static const VMStateDescription vmxstate_vmxnet3_mcast_list = {
     .pre_load = vmxnet3_mcast_list_pre_load,
     .needed = vmxnet3_mc_list_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL,
             mcast_list_buff_size),
         VMSTATE_END_OF_LIST()
     }
diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index 24f6121..09676e2 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -83,7 +83,7 @@ static const VMStateDescription vmstate_macio_nvram = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, 0, size),
+        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, size),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 4de5f70..ccef3f9 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -218,7 +218,7 @@ static const VMStateDescription vmstate_spapr_nvram = {
     .post_load = spapr_nvram_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(size, sPAPRNVRAM),
-        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
+        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, size),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 01fbf22..c0d7de7 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1253,7 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
         VMSTATE_UINT16(data_count, SDHCIState),
         VMSTATE_UINT64(admasysaddr, SDHCIState),
         VMSTATE_UINT8(stopped_state, SDHCIState),
-        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz),
+        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, buf_maxsz),
         VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
         VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
         VMSTATE_END_OF_LIST()
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index e46ca88..40a9e18 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -634,7 +634,7 @@ static const VMStateDescription vmstate_m48t59 = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(lock, M48t59State),
         VMSTATE_UINT16(addr, M48t59State),
-        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, 0, size),
+        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, size),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1638ee5..26527ad 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -190,7 +190,6 @@ typedef struct {
     const char *name;
     size_t offset;
     size_t size;
-    size_t start;
     int num;
     size_t num_offset;
     size_t size_offset;
@@ -570,7 +569,7 @@ extern const VMStateInfo vmstate_info_bitmap;
     .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
 }
 
-#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
+#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _field_size, _multiply) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
@@ -579,10 +578,9 @@ extern const VMStateInfo vmstate_info_bitmap;
     .info         = &vmstate_info_buffer,                            \
     .flags        = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY,            \
     .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
 }
 
-#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
+#define VMSTATE_VBUFFER(_field, _state, _version, _test, _field_size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
@@ -590,10 +588,9 @@ extern const VMStateInfo vmstate_info_bitmap;
     .info         = &vmstate_info_buffer,                            \
     .flags        = VMS_VBUFFER|VMS_POINTER,                         \
     .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
 }
 
-#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
+#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _field_size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
@@ -601,10 +598,9 @@ extern const VMStateInfo vmstate_info_bitmap;
     .info         = &vmstate_info_buffer,                            \
     .flags        = VMS_VBUFFER|VMS_POINTER,                         \
     .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
 }
 
-#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _start, _field_size) { \
+#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _field_size) { \
     .name         = (stringify(_field)),                             \
     .version_id   = (_version),                                      \
     .field_exists = (_test),                                         \
@@ -612,7 +608,6 @@ extern const VMStateInfo vmstate_info_bitmap;
     .info         = &vmstate_info_buffer,                            \
     .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,               \
     .offset       = offsetof(_state, _field),                        \
-    .start        = (_start),                                        \
 }
 
 #define VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, _test, _version, _info, _size) { \
@@ -912,13 +907,10 @@ extern const VMStateInfo vmstate_info_bitmap;
     VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, 0)
 
 #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
-    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
+    VMSTATE_VBUFFER(_f, _s, 0, NULL, _size)
 
 #define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
-    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
-
-#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
-    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
+    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, _size)
 
 #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
     VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))
diff --git a/migration/savevm.c b/migration/savevm.c
index 33a2911..1df5c76 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -308,7 +308,7 @@ static const VMStateDescription vmstate_configuration = {
     .pre_save = configuration_pre_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(len, SaveState),
-        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, 0, len),
+        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
         VMSTATE_END_OF_LIST()
     },
 };
diff --git a/migration/vmstate.c b/migration/vmstate.c
index fc29acf..8767e40 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -66,10 +66,10 @@ static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
                 }
             }
             if (size) {
-                *((void **)base_addr + field->start) = g_malloc(size);
+                *(void **)base_addr = g_malloc(size);
             }
         }
-        base_addr = *(void **)base_addr + field->start;
+        base_addr = *(void **)base_addr;
     }
 
     return base_addr;
diff --git a/target-s390x/machine.c b/target-s390x/machine.c
index edc3a47..8503fa1 100644
--- a/target-s390x/machine.c
+++ b/target-s390x/machine.c
@@ -180,7 +180,7 @@ const VMStateDescription vmstate_s390_cpu = {
         VMSTATE_UINT8(env.cpu_state, S390CPU),
         VMSTATE_UINT8(env.sigp_order, S390CPU),
         VMSTATE_UINT32_V(irqstate_saved_size, S390CPU, 4),
-        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL, 0,
+        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
                                irqstate_saved_size),
         VMSTATE_END_OF_LIST()
     },
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 4ea64b7..aaa7143 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -506,9 +506,9 @@ static const VMStateDescription vmstate_vbuffer = {
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(u8_1, TestVBuffer),
-        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL, 0,
+        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL,
                         buffer_size),
-        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL, 0,
+        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL,
                         buffer_alloc_size),
         VMSTATE_UINT8(u8_2, TestVBuffer),
         VMSTATE_END_OF_LIST()
diff --git a/util/fifo8.c b/util/fifo8.c
index 5c64101..d38b3bd 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -118,7 +118,7 @@ const VMStateDescription vmstate_fifo8 = {
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, 0, capacity),
+        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, capacity),
         VMSTATE_UINT32(head, Fifo8),
         VMSTATE_UINT32(num, Fifo8),
         VMSTATE_END_OF_LIST()
-- 
2.8.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start
  2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
                   ` (3 preceding siblings ...)
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start Halil Pasic
@ 2016-10-18 11:24 ` no-reply
  4 siblings, 0 replies; 16+ messages in thread
From: no-reply @ 2016-10-18 11:24 UTC (permalink / raw)
  To: pasic; +Cc: famz, qemu-devel, amit.shah, hutzl, dgilbert

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20161018105724.26520-1-pasic@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1476782267-2602-1-git-send-email-ptoscano@redhat.com -> patchew/1476782267-2602-1-git-send-email-ptoscano@redhat.com
 - [tag update]      patchew/1476787062-27376-1-git-send-email-ptoscano@redhat.com -> patchew/1476787062-27376-1-git-send-email-ptoscano@redhat.com
Switched to a new branch 'test'
f8c250a migration: drop unused VMStateField.start
908def4 Revert "tests/test-vmstate.c: prove VMStateField.start broken"
10612d0 tests/test-vmstate.c: prove VMStateField.start broken
138f40f tests/test-vmstate.c: Add vBuffer test

=== OUTPUT BEGIN ===
Checking PATCH 1/4: tests/test-vmstate.c: Add vBuffer test...
Checking PATCH 2/4: tests/test-vmstate.c: prove VMStateField.start broken...
Checking PATCH 3/4: Revert "tests/test-vmstate.c: prove VMStateField.start broken"...
Checking PATCH 4/4: migration: drop unused VMStateField.start...
ERROR: line over 90 characters
#195: FILE: include/migration/vmstate.h:572:
+#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _field_size, _multiply) { \

WARNING: line over 80 characters
#231: FILE: include/migration/vmstate.h:603:
+#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _field_size) { \

total: 1 errors, 1 warnings, 223 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken Halil Pasic
@ 2016-10-18 13:27   ` Dr. David Alan Gilbert
  2016-10-18 13:43     ` Halil Pasic
  0 siblings, 1 reply; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-18 13:27 UTC (permalink / raw)
  To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Guenther Hutzl

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> The handling of VMStateField.start is currently quite broken if
> VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but
> fortunately also quite underutilized -- nobody is using .start != 0.
> 
> Let's prove with this patch that it's really broken (as a first
> step towards fixing things up).

You can't play this trick - patch series have to work and pass
tests after each test, otherwise it messes up people in the future
trying to do a git bisect.

Dave

> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> ---
> 
> The idea is to remove .start support and this patch should
> be reverted, as soon this happens, or even better just
> dropped. If however dropping the support for .start encounters
> resistance, this patch should prove useful in an unexpected
> way.
> ---
>  tests/test-vmstate.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 9a57aa0..a2ef4a8 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -588,6 +588,53 @@ static void test_complex_vbuffer(void)
>  #undef FIELD_EQUAL
>  #undef BUFFER_EQUAL
>  
> +typedef struct {
> +    uint32_t vbuff_size;
> +    uint8_t *vbuff;
> +    uint64_t stuff;
> +} TestVBufStart;
> +
> +static const VMStateDescription vmstate_vbuff_alloc_start = {
> +    .name = "test/vbuff_alloc_start",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(stuff, TestVBufStart),
> +        VMSTATE_UINT32(vbuff_size, TestVBufStart),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(vbuff, TestVBufStart, 1, 0, 1, vbuff_size),
> +        VMSTATE_END_OF_LIST()
> +    }
> +
> +};
> +
> +static void load_vmstate_one_obj(const VMStateDescription *vmsd, void *obj,
> +        int version_id)
> +{
> +        QEMUFile *fload = open_test_file(false);
> +
> +        SUCCESS(vmstate_load_state(fload, vmsd, obj, version_id));
> +        qemu_fclose(fload);
> +}
> +
> +static void test_vbuff_alloc_start(void)
> +{
> +    uint8_t my_vbuff1[] = {0, 1, 2, 3};
> +    uint8_t my_vbuff2[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +    TestVBufStart sample = {
> +        .stuff = 1,
> +        .vbuff_size = 3,
> +        .vbuff = my_vbuff1,
> +    };
> +    TestVBufStart load_obj = {
> +        .vbuff = my_vbuff2,
> +    };
> +
> +    save_vmstate(&vmstate_vbuff_alloc_start, &sample);
> +    load_vmstate_one_obj(&vmstate_vbuff_alloc_start, &load_obj, 1);
> +    g_assert_cmpint(load_obj.stuff, ==, 0);
> +    g_assert_cmpint((uint64_t) load_obj.vbuff, !=, (uint64_t) my_vbuff2);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      temp_fd = mkstemp(temp_file);
> @@ -603,8 +650,8 @@ int main(int argc, char **argv)
>      g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
>      g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
>      g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer);
> +    g_test_add_func("/vmstate/vbuff/alloc_start", test_vbuff_alloc_start);
>      g_test_run();
> -
>      close(temp_fd);
>      unlink(temp_file);
>  
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 13:27   ` Dr. David Alan Gilbert
@ 2016-10-18 13:43     ` Halil Pasic
  2016-10-18 13:54       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 13:43 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Amit Shah, Guenther Hutzl



On 10/18/2016 03:27 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> > The handling of VMStateField.start is currently quite broken if
>> > VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but
>> > fortunately also quite underutilized -- nobody is using .start != 0.
>> > 
>> > Let's prove with this patch that it's really broken (as a first
>> > step towards fixing things up).
> You can't play this trick - patch series have to work and pass
> tests after each test, otherwise it messes up people in the future
> trying to do a git bisect.
> 
> Dave
> 

I think I understand the motivation. Does that mean
you are not supposed to expose a bug via a test? I might
be able to demonstrate that something is wrong but unable
to fix the problem myself (time constraints).

How was I supposed to do this?

Cheers,
Halil

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 13:43     ` Halil Pasic
@ 2016-10-18 13:54       ` Dr. David Alan Gilbert
  2016-10-18 15:33         ` Halil Pasic
  0 siblings, 1 reply; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-18 13:54 UTC (permalink / raw)
  To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Guenther Hutzl

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/18/2016 03:27 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> > The handling of VMStateField.start is currently quite broken if
> >> > VMS_ALLOC is present (that is for VMSTATE_VBUFFER_ALLOC_UINT32) but
> >> > fortunately also quite underutilized -- nobody is using .start != 0.
> >> > 
> >> > Let's prove with this patch that it's really broken (as a first
> >> > step towards fixing things up).
> > You can't play this trick - patch series have to work and pass
> > tests after each test, otherwise it messes up people in the future
> > trying to do a git bisect.
> > 
> > Dave
> > 
> 
> I think I understand the motivation. Does that mean
> you are not supposed to expose a bug via a test? I might
> be able to demonstrate that something is wrong but unable
> to fix the problem myself (time constraints).
> 
> How was I supposed to do this?

You might add a test but leave it commented out, or just post
the test but not for merging so that it only gets merged
after someone fixes the bug.

Dave

> 
> Cheers,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 13:54       ` Dr. David Alan Gilbert
@ 2016-10-18 15:33         ` Halil Pasic
  2016-10-18 18:32           ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-18 15:33 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: Amit Shah, qemu-devel, Guenther Hutzl



On 10/18/2016 03:54 PM, Dr. David Alan Gilbert wrote:
>> > I think I understand the motivation. Does that mean
>> > you are not supposed to expose a bug via a test? I might
>> > be able to demonstrate that something is wrong but unable
>> > to fix the problem myself (time constraints).
>> > 
>> > How was I supposed to do this?
> You might add a test but leave it commented out, or just post
> the test but not for merging so that it only gets merged
> after someone fixes the bug.
> 
> Dave
> 

As stated by the accompanying message:

"The idea is to remove .start support and this patch should
be reverted, as soon this happens, or even better just
dropped. If however dropping the support for .start encounters
resistance, this patch should prove useful in an unexpected
way."

the patch is not intended for a merge. My preferred way of dealing
with this is to just pick (merge) the first and the last patch of the
series. The second patch is just to prove that we have a problem,
and it's effect is immediately reverted by the third patch as a
preparation for the forth one which removes the tested feature altogether.

In my opinion the inclusion of a commented out test makes even less
sense if the tested feature is intended to be removed by the next
patch in the series.

I think I was not clear enough when stating that this patch is
not intended for merging. Is there an established way to do
this?

Cheers,
Halil

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 15:33         ` Halil Pasic
@ 2016-10-18 18:32           ` Dr. David Alan Gilbert
  2016-10-19 11:04             ` Halil Pasic
  0 siblings, 1 reply; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-18 18:32 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Amit Shah, qemu-devel, Guenther Hutzl

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/18/2016 03:54 PM, Dr. David Alan Gilbert wrote:
> >> > I think I understand the motivation. Does that mean
> >> > you are not supposed to expose a bug via a test? I might
> >> > be able to demonstrate that something is wrong but unable
> >> > to fix the problem myself (time constraints).
> >> > 
> >> > How was I supposed to do this?
> > You might add a test but leave it commented out, or just post
> > the test but not for merging so that it only gets merged
> > after someone fixes the bug.
> > 
> > Dave
> > 
> 
> As stated by the accompanying message:
> 
> "The idea is to remove .start support and this patch should
> be reverted, as soon this happens, or even better just
> dropped. If however dropping the support for .start encounters
> resistance, this patch should prove useful in an unexpected
> way."
> 
> the patch is not intended for a merge. My preferred way of dealing
> with this is to just pick (merge) the first and the last patch of the
> series. The second patch is just to prove that we have a problem,
> and it's effect is immediately reverted by the third patch as a
> preparation for the forth one which removes the tested feature altogether.
> 
> In my opinion the inclusion of a commented out test makes even less
> sense if the tested feature is intended to be removed by the next
> patch in the series.
> 
> I think I was not clear enough when stating that this patch is
> not intended for merging. Is there an established way to do
> this?

I don't think there's any point in posting it like that as part
of a patch series; posting it as a separate test that fails or
something like that; but I don't think I've ever seen it done
like that inside a patch series where you expect some of it
to be picked up.

Dave

> 
> Cheers,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-18 18:32           ` Dr. David Alan Gilbert
@ 2016-10-19 11:04             ` Halil Pasic
  2016-10-20 12:00               ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 16+ messages in thread
From: Halil Pasic @ 2016-10-19 11:04 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: Amit Shah, qemu-devel, Guenther Hutzl



On 10/18/2016 08:32 PM, Dr. David Alan Gilbert wrote:
>> > "The idea is to remove .start support and this patch should
>> > be reverted, as soon this happens, or even better just
>> > dropped. If however dropping the support for .start encounters
>> > resistance, this patch should prove useful in an unexpected
>> > way."
>> > 
>> > the patch is not intended for a merge. My preferred way of dealing
>> > with this is to just pick (merge) the first and the last patch of the
>> > series. The second patch is just to prove that we have a problem,
>> > and it's effect is immediately reverted by the third patch as a
>> > preparation for the forth one which removes the tested feature altogether.
>> > 
>> > In my opinion the inclusion of a commented out test makes even less
>> > sense if the tested feature is intended to be removed by the next
>> > patch in the series.
>> > 
>> > I think I was not clear enough when stating that this patch is
>> > not intended for merging. Is there an established way to do
>> > this?
> I don't think there's any point in posting it like that as part
> of a patch series; posting it as a separate test that fails or
> something like that; but I don't think I've ever seen it done
> like that inside a patch series where you expect some of it
> to be picked up.
> 
> Dave
> 

I understand. I assumed cherry-picking the two relevant patches from the
series would not be a problem here. I was wrong.

Next time I will make sure to either do a separate failing test patch
and and cross reference in the cover letters, or to first do the fix and
then improve the test coverage so the bug does not come back.

Should I send a v2 with the two questionable patches (the failing test
and the revert of it) removed right away?

Regards,
Halil

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
@ 2016-10-20 11:52   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-20 11:52 UTC (permalink / raw)
  To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Guenther Hutzl, quintela

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> From: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> 
> The unit test test-vmstate.c is missing tests for some of the complex
> vmstate macros. This patch adds a new test for VMSTATE_VBUFFER
> and VMSTATE_VBUFFER_ALLOC_UINT32. The added test does not cover
> start != 0 because it's broken and unused so our intention is to
> remove start altogether.
> 
> Signed-off-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> 
> A proof for the brokenness of start is provided in a separate patch
> so it can be easily reverted or not picked (thus we separate what is
> intended to stay, and what is intended to go away).

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

> ---
>  tests/test-vmstate.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index d8da26f..9a57aa0 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -475,6 +475,119 @@ static void test_load_skip(void)
>      qemu_fclose(loading);
>  }
>  
> +/* vBuffer tests */
> +#define BUF_SIZE 10
> +
> +typedef struct TestVBuffer {
> +    uint8_t  u8_1;
> +    int32_t  buffer_size;
> +    uint8_t  *vBuffer_1;
> +    uint32_t buffer_alloc_size;
> +    uint8_t  *vBuffer_alloc_1;
> +    uint8_t  u8_2;
> +} TestVBuffer;
> +
> +/* buffers padded with 0xFE at the end to easier detect overflow */
> +uint8_t buf1[BUF_SIZE + 1] = { 1,  2,  3,  4,  5,  6,  7,  8,  9,  0, 0xfe};
> +uint8_t buf2[BUF_SIZE + 1] = { 1,  2,  3,  4,  5,  6,  7,  8,  9,  0, 0xfe};
> +
> +TestVBuffer obj_vbuffer = {
> +    .u8_1 = 100,
> +    .buffer_size = BUF_SIZE,
> +    .vBuffer_1 = buf1,
> +    .buffer_alloc_size = BUF_SIZE,
> +    .vBuffer_alloc_1 = buf2,
> +    .u8_2 = 200,
> +};
> +
> +static const VMStateDescription vmstate_vbuffer = {
> +    .name = "complex/vbuffer",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(u8_1, TestVBuffer),
> +        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL, 0,
> +                        buffer_size),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL, 0,
> +                        buffer_alloc_size),
> +        VMSTATE_UINT8(u8_2, TestVBuffer),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +uint8_t wire_vbuffer[] = {
> +    /* u8_1 */            0x64,
> +    /* vBuffer_1 */       0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
> +                          0x00,
> +    /* vBuffer_alloc_1 */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
> +                          0x00,
> +    /* u8_2 */            0xc8,
> +    QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
> +};
> +
> +static void obj_vbuffer_copy(void *target, void *source)
> +{
> +    /* this proc copies all struct TestVBuffer entries from source to target
> +       except the vBuffer pointers which should already point to the correct
> +       locations. The buffer contents are also copied */
> +    TestVBuffer *s = (TestVBuffer *)source;
> +    TestVBuffer *t = (TestVBuffer *)target;
> +
> +    t->u8_1 = s->u8_1;
> +    t->u8_2 = s->u8_2;
> +    t->buffer_size = s->buffer_size;
> +    t->buffer_alloc_size = s->buffer_alloc_size;
> +    if (t->vBuffer_1 && s->vBuffer_1) {
> +        memcpy(t->vBuffer_1, s->vBuffer_1, BUF_SIZE);
> +    }
> +    if (t->vBuffer_alloc_1 && s->vBuffer_alloc_1) {
> +        memcpy(t->vBuffer_alloc_1, s->vBuffer_alloc_1, BUF_SIZE);
> +    }
> +}
> +
> +static void test_complex_vbuffer(void)
> +{
> +    uint8_t buffer[BUF_SIZE];
> +    uint8_t buffer_clone[BUF_SIZE];
> +    TestVBuffer obj = {
> +        .u8_1 = 0,
> +        .buffer_size = BUF_SIZE,
> +        .vBuffer_1 = buffer,
> +        .buffer_alloc_size = BUF_SIZE,
> +        .vBuffer_alloc_1 = NULL,
> +        .u8_2 = 0,
> +    };
> +    TestVBuffer obj_clone = {
> +        .u8_1 = 0,
> +        .buffer_size = BUF_SIZE,
> +        .vBuffer_1 = buffer_clone,
> +        .buffer_alloc_size = BUF_SIZE,
> +        .vBuffer_alloc_1 = NULL,
> +        .u8_2 = 0,
> +    };
> +
> +    memset(buffer, 0, BUF_SIZE);
> +    memset(buffer_clone, 0, BUF_SIZE);
> +
> +    save_vmstate(&vmstate_vbuffer, &obj_vbuffer);
> +
> +    compare_vmstate(wire_vbuffer, sizeof(wire_vbuffer));
> +
> +    SUCCESS(load_vmstate(&vmstate_vbuffer, &obj, &obj_clone,
> +                         obj_vbuffer_copy, 1, wire_vbuffer,
> +                         sizeof(wire_vbuffer)));
> +
> +#define FIELD_EQUAL(name)  g_assert_cmpint(obj.name, ==, obj_vbuffer.name)
> +#define BUFFER_EQUAL(name) SUCCESS(memcmp(obj.name, obj_vbuffer.name, BUF_SIZE))
> +
> +    FIELD_EQUAL(u8_1);
> +    BUFFER_EQUAL(vBuffer_1);
> +    BUFFER_EQUAL(vBuffer_alloc_1);
> +    FIELD_EQUAL(u8_2);
> +}
> +#undef FIELD_EQUAL
> +#undef BUFFER_EQUAL
> +
>  int main(int argc, char **argv)
>  {
>      temp_fd = mkstemp(temp_file);
> @@ -489,6 +602,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/vmstate/field_exists/load/skip", test_load_skip);
>      g_test_add_func("/vmstate/field_exists/save/noskip", test_save_noskip);
>      g_test_add_func("/vmstate/field_exists/save/skip", test_save_skip);
> +    g_test_add_func("/vmstate/complex/vbuffer", test_complex_vbuffer);
>      g_test_run();
>  
>      close(temp_fd);
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start
  2016-10-18 10:57 ` [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start Halil Pasic
@ 2016-10-20 12:00   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-20 12:00 UTC (permalink / raw)
  To: Halil Pasic; +Cc: qemu-devel, Amit Shah, quintela@redhat.com Guenther Hutzl

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> The member VMStateField.start was solely used to implement the partial
> data migration for VBUFFER data (basically provide migration for a
> sub-buffer). However the implementation of this feature seems broken to
> me, but this goes unnoticed since the feature is not used at all.
> Instead of fixing it lets try simplify things by dropping it.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>

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

> ---
>  hw/char/exynos4210_uart.c   |  2 +-
>  hw/display/g364fb.c         |  2 +-
>  hw/dma/pl330.c              |  8 ++++----
>  hw/intc/exynos4210_gic.c    |  2 +-
>  hw/ipmi/isa_ipmi_bt.c       |  4 ++--
>  hw/ipmi/isa_ipmi_kcs.c      |  4 ++--
>  hw/net/vmxnet3.c            |  2 +-
>  hw/nvram/mac_nvram.c        |  2 +-
>  hw/nvram/spapr_nvram.c      |  2 +-
>  hw/sd/sdhci.c               |  2 +-
>  hw/timer/m48t59.c           |  2 +-
>  include/migration/vmstate.h | 20 ++++++--------------
>  migration/savevm.c          |  2 +-
>  migration/vmstate.c         |  4 ++--
>  target-s390x/machine.c      |  2 +-
>  tests/test-vmstate.c        |  4 ++--
>  util/fifo8.c                |  2 +-
>  17 files changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index 1107578..7b1b4b1 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -565,7 +565,7 @@ static const VMStateDescription vmstate_exynos4210_uart_fifo = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(sp, Exynos4210UartFIFO),
>          VMSTATE_UINT32(rp, Exynos4210UartFIFO),
> -        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, 0, size),
> +        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 70ef2c7..8cdc205 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -464,7 +464,7 @@ static const VMStateDescription vmstate_g364fb = {
>      .minimum_version_id = 1,
>      .post_load = g364fb_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, 0, vram_size),
> +        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
>          VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
>          VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
>          VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index c0bd9fe..32cf839 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -173,8 +173,8 @@ static const VMStateDescription vmstate_pl330_fifo = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, 0, buf_size),
> -        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, 0, buf_size),
> +        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, buf_size),
> +        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, buf_size),
>          VMSTATE_UINT32(head, PL330Fifo),
>          VMSTATE_UINT32(num, PL330Fifo),
>          VMSTATE_UINT32(buf_size, PL330Fifo),
> @@ -282,8 +282,8 @@ static const VMStateDescription vmstate_pl330 = {
>          VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
>          VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
>                                       vmstate_pl330_chan, PL330Chan),
> -        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, 0, num_chnls),
> -        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, 0, num_chnls),
> +        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
> +        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
>          VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
>          VMSTATE_STRUCT(read_queue, PL330State, 0, vmstate_pl330_queue,
>                         PL330Queue),
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index fd7a8f3..2a55817 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -393,7 +393,7 @@ static const VMStateDescription vmstate_exynos4210_irq_gate = {
>      .version_id = 2,
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, 0, n_in),
> +        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, n_in),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index f036617..6c10ef7 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -471,9 +471,9 @@ static const VMStateDescription vmstate_ISAIPMIBTDevice = {
>          VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
>          VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
>          VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
> -        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL,
>                                 bt.outlen),
> -        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL,
>                                 bt.inlen),
>          VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
>          VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index 9a38f8a..b592d53 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -433,9 +433,9 @@ const VMStateDescription vmstate_ISAIPMIKCSDevice = {
>          VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
>          VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
>          VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
> -        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL,
>                                 kcs.outlen),
> -        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL,
>                                 kcs.inlen),
>          VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
>          VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 90f6943..b4b71f3 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2396,7 +2396,7 @@ static const VMStateDescription vmxstate_vmxnet3_mcast_list = {
>      .pre_load = vmxnet3_mcast_list_pre_load,
>      .needed = vmxnet3_mc_list_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL,
>              mcast_list_buff_size),
>          VMSTATE_END_OF_LIST()
>      }
> diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
> index 24f6121..09676e2 100644
> --- a/hw/nvram/mac_nvram.c
> +++ b/hw/nvram/mac_nvram.c
> @@ -83,7 +83,7 @@ static const VMStateDescription vmstate_macio_nvram = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, 0, size),
> +        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 4de5f70..ccef3f9 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -218,7 +218,7 @@ static const VMStateDescription vmstate_spapr_nvram = {
>      .post_load = spapr_nvram_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(size, sPAPRNVRAM),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, size),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 01fbf22..c0d7de7 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1253,7 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
>          VMSTATE_UINT16(data_count, SDHCIState),
>          VMSTATE_UINT64(admasysaddr, SDHCIState),
>          VMSTATE_UINT8(stopped_state, SDHCIState),
> -        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz),
> +        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, buf_maxsz),
>          VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
>          VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
>          VMSTATE_END_OF_LIST()
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index e46ca88..40a9e18 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -634,7 +634,7 @@ static const VMStateDescription vmstate_m48t59 = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(lock, M48t59State),
>          VMSTATE_UINT16(addr, M48t59State),
> -        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, 0, size),
> +        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1638ee5..26527ad 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -190,7 +190,6 @@ typedef struct {
>      const char *name;
>      size_t offset;
>      size_t size;
> -    size_t start;
>      int num;
>      size_t num_offset;
>      size_t size_offset;
> @@ -570,7 +569,7 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
>  }
>  
> -#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
> +#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _field_size, _multiply) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -579,10 +578,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY,            \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
> -#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
> +#define VMSTATE_VBUFFER(_field, _state, _version, _test, _field_size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -590,10 +588,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
> -#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
> +#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _field_size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -601,10 +598,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
> -#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _start, _field_size) { \
> +#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _field_size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -612,7 +608,6 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,               \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
>  #define VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, _test, _version, _info, _size) { \
> @@ -912,13 +907,10 @@ extern const VMStateInfo vmstate_info_bitmap;
>      VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, 0)
>  
>  #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, _size)
>  
>  #define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> -
> -#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
> +    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, _size)
>  
>  #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 33a2911..1df5c76 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -308,7 +308,7 @@ static const VMStateDescription vmstate_configuration = {
>      .pre_save = configuration_pre_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(len, SaveState),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, 0, len),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fc29acf..8767e40 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -66,10 +66,10 @@ static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
>                  }
>              }
>              if (size) {
> -                *((void **)base_addr + field->start) = g_malloc(size);
> +                *(void **)base_addr = g_malloc(size);
>              }
>          }
> -        base_addr = *(void **)base_addr + field->start;
> +        base_addr = *(void **)base_addr;
>      }
>  
>      return base_addr;
> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
> index edc3a47..8503fa1 100644
> --- a/target-s390x/machine.c
> +++ b/target-s390x/machine.c
> @@ -180,7 +180,7 @@ const VMStateDescription vmstate_s390_cpu = {
>          VMSTATE_UINT8(env.cpu_state, S390CPU),
>          VMSTATE_UINT8(env.sigp_order, S390CPU),
>          VMSTATE_UINT32_V(irqstate_saved_size, S390CPU, 4),
> -        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
>                                 irqstate_saved_size),
>          VMSTATE_END_OF_LIST()
>      },
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 4ea64b7..aaa7143 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -506,9 +506,9 @@ static const VMStateDescription vmstate_vbuffer = {
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(u8_1, TestVBuffer),
> -        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL, 0,
> +        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL,
>                          buffer_size),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL, 0,
> +        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL,
>                          buffer_alloc_size),
>          VMSTATE_UINT8(u8_2, TestVBuffer),
>          VMSTATE_END_OF_LIST()
> diff --git a/util/fifo8.c b/util/fifo8.c
> index 5c64101..d38b3bd 100644
> --- a/util/fifo8.c
> +++ b/util/fifo8.c
> @@ -118,7 +118,7 @@ const VMStateDescription vmstate_fifo8 = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, 0, capacity),
> +        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, capacity),
>          VMSTATE_UINT32(head, Fifo8),
>          VMSTATE_UINT32(num, Fifo8),
>          VMSTATE_END_OF_LIST()
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-19 11:04             ` Halil Pasic
@ 2016-10-20 12:00               ` Dr. David Alan Gilbert
  2016-10-20 13:05                 ` Halil Pasic
  0 siblings, 1 reply; 16+ messages in thread
From: Dr. David Alan Gilbert @ 2016-10-20 12:00 UTC (permalink / raw)
  To: Halil Pasic; +Cc: Amit Shah, qemu-devel, quintela@redhat.com Guenther Hutzl

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 10/18/2016 08:32 PM, Dr. David Alan Gilbert wrote:
> >> > "The idea is to remove .start support and this patch should
> >> > be reverted, as soon this happens, or even better just
> >> > dropped. If however dropping the support for .start encounters
> >> > resistance, this patch should prove useful in an unexpected
> >> > way."
> >> > 
> >> > the patch is not intended for a merge. My preferred way of dealing
> >> > with this is to just pick (merge) the first and the last patch of the
> >> > series. The second patch is just to prove that we have a problem,
> >> > and it's effect is immediately reverted by the third patch as a
> >> > preparation for the forth one which removes the tested feature altogether.
> >> > 
> >> > In my opinion the inclusion of a commented out test makes even less
> >> > sense if the tested feature is intended to be removed by the next
> >> > patch in the series.
> >> > 
> >> > I think I was not clear enough when stating that this patch is
> >> > not intended for merging. Is there an established way to do
> >> > this?
> > I don't think there's any point in posting it like that as part
> > of a patch series; posting it as a separate test that fails or
> > something like that; but I don't think I've ever seen it done
> > like that inside a patch series where you expect some of it
> > to be picked up.
> > 
> > Dave
> > 
> 
> I understand. I assumed cherry-picking the two relevant patches from the
> series would not be a problem here. I was wrong.
> 
> Next time I will make sure to either do a separate failing test patch
> and and cross reference in the cover letters, or to first do the fix and
> then improve the test coverage so the bug does not come back.
> 
> Should I send a v2 with the two questionable patches (the failing test
> and the revert of it) removed right away?

Yes, probably the best way; you can add the Review-by's I've just
sent.

Dave

> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken
  2016-10-20 12:00               ` Dr. David Alan Gilbert
@ 2016-10-20 13:05                 ` Halil Pasic
  0 siblings, 0 replies; 16+ messages in thread
From: Halil Pasic @ 2016-10-20 13:05 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Amit Shah, qemu-devel, quintela@redhat.com Guenther Hutzl



On 10/20/2016 02:00 PM, Dr. David Alan Gilbert wrote:
>> Should I send a v2 with the two questionable patches (the failing test
>> > and the revert of it) removed right away?
> Yes, probably the best way; you can add the Review-by's I've just
> sent.
> 
> Dave
> 

Thanks a lot. Will do!

Halil

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-10-20 13:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
2016-10-20 11:52   ` Dr. David Alan Gilbert
2016-10-18 10:57 ` [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken Halil Pasic
2016-10-18 13:27   ` Dr. David Alan Gilbert
2016-10-18 13:43     ` Halil Pasic
2016-10-18 13:54       ` Dr. David Alan Gilbert
2016-10-18 15:33         ` Halil Pasic
2016-10-18 18:32           ` Dr. David Alan Gilbert
2016-10-19 11:04             ` Halil Pasic
2016-10-20 12:00               ` Dr. David Alan Gilbert
2016-10-20 13:05                 ` Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 3/4] Revert "tests/test-vmstate.c: prove VMStateField.start broken" Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start Halil Pasic
2016-10-20 12:00   ` Dr. David Alan Gilbert
2016-10-18 11:24 ` [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.