* [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start
@ 2016-10-20 13:25 Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 1/2] tests/test-vmstate.c: add vBuffer test Halil Pasic
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Halil Pasic @ 2016-10-20 13:25 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.
Let us add an unit test for VBUFFER and then remove the useless
VMStateFiled.start.
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 (1):
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 | 114 ++++++++++++++++++++++++++++++++++++++++++++
util/fifo8.c | 2 +-
17 files changed, 141 insertions(+), 35 deletions(-)
--
2.8.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] tests/test-vmstate.c: add vBuffer test
2016-10-20 13:25 [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start Halil Pasic
@ 2016-10-20 13:25 ` Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start Halil Pasic
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Halil Pasic @ 2016-10-20 13:25 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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
A proof for the brokenness (unit test) of .start is provided here:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04042.html
---
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] 9+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start
2016-10-20 13:25 [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 1/2] tests/test-vmstate.c: add vBuffer test Halil Pasic
@ 2016-10-20 13:25 ` Halil Pasic
2017-01-30 15:28 ` Halil Pasic
2016-10-20 13:48 ` [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start no-reply
2016-10-31 17:33 ` Halil Pasic
3 siblings, 1 reply; 9+ messages in thread
From: Halil Pasic @ 2016-10-20 13:25 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>
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 9a57aa0..1302c82 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] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start
2016-10-20 13:25 [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 1/2] tests/test-vmstate.c: add vBuffer test Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start Halil Pasic
@ 2016-10-20 13:48 ` no-reply
2016-10-31 17:33 ` Halil Pasic
3 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2016-10-20 13:48 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:
Subject: [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start
Type: series
Message-id: 20161020132556.67321-1-pasic@linux.vnet.ibm.com
=== 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
Switched to a new branch 'test'
b95fb4b migration: drop unused VMStateField.start
d4fd416 tests/test-vmstate.c: add vBuffer test
=== OUTPUT BEGIN ===
Checking PATCH 1/2: tests/test-vmstate.c: add vBuffer test...
Checking PATCH 2/2: migration: drop unused VMStateField.start...
ERROR: line over 90 characters
#196: FILE: include/migration/vmstate.h:572:
+#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _field_size, _multiply) { \
WARNING: line over 80 characters
#232: 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] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start
2016-10-20 13:25 [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start Halil Pasic
` (2 preceding siblings ...)
2016-10-20 13:48 ` [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start no-reply
@ 2016-10-31 17:33 ` Halil Pasic
3 siblings, 0 replies; 9+ messages in thread
From: Halil Pasic @ 2016-10-31 17:33 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Guenther Hutzl, Dr. David Alan Gilbert, Juan Quintela
ping
I would like to do further work based on this, and Dave's review
for v1 was quite favorable (IMHO). Any chance of this getting
merged soon (or receiving critical comments so I can move forward)?
Cheers,
Halil
On 10/20/2016 03:25 PM, Halil Pasic 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 is broken, but
> this goes unnoticed since the feature is not used at all.
>
> Let us add an unit test for VBUFFER and then remove the useless
> VMStateFiled.start.
>
> 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 (1):
> 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 | 114 ++++++++++++++++++++++++++++++++++++++++++++
> util/fifo8.c | 2 +-
> 17 files changed, 141 insertions(+), 35 deletions(-)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start Halil Pasic
@ 2017-01-30 15:28 ` Halil Pasic
2017-01-31 20:00 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 9+ messages in thread
From: Halil Pasic @ 2017-01-30 15:28 UTC (permalink / raw)
To: Amit Shah, Dr. David Alan Gilbert, Juan Quintela
Cc: qemu-devel, Guenther Hutzl
On 10/20/2016 03:25 PM, Halil Pasic wrote:
> 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;
Hi!
It is been a while, and IMHO this is still broken, and the
VMSTATE_VBUFFER* macros are still only used with the start argument
being zero.
What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
from Jan 19 we have code actually using VMStateDecription.start -- but
for something different (IMHO), as allocation is done by get_qtailq and
not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
Thus I would need to update the commit message and keep the start field
at least.
But before I do so, I would like to ask the maintainers if there is
interest in a change like this?
Regards,
Halil
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start
2017-01-30 15:28 ` Halil Pasic
@ 2017-01-31 20:00 ` Dr. David Alan Gilbert
2017-02-01 13:02 ` Halil Pasic
0 siblings, 1 reply; 9+ messages in thread
From: Dr. David Alan Gilbert @ 2017-01-31 20:00 UTC (permalink / raw)
To: Halil Pasic; +Cc: Amit Shah, Juan Quintela, qemu-devel, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 10/20/2016 03:25 PM, Halil Pasic wrote:
> > 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;
> Hi!
>
> It is been a while, and IMHO this is still broken, and the
> VMSTATE_VBUFFER* macros are still only used with the start argument
> being zero.
>
> What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
> from Jan 19 we have code actually using VMStateDecription.start -- but
> for something different (IMHO), as allocation is done by get_qtailq and
> not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
> Thus I would need to update the commit message and keep the start field
> at least.
>
> But before I do so, I would like to ask the maintainers if there is
> interest in a change like this?
I think it's certainly worth fixing VMSTATE_BUFFER_ALLOC*; if it can't
use the start field properly then it should have the start parameter
removed.
I'll admit I can't remember the details of why field->start as an offset
didn't work; are the other macros that take a start value correct?
If we can't remove the start variable then it's probably not removing
the start parameter everywhere.
That alloc case looks the most unusual in that vmstate_base_addr()
function; I *think* the non-alloc case makes sense
(i.e. follow a pointer and then use an offset from that pointer?)
even if no one currently uses it.
Dave
> Regards,
> Halil
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start
2017-01-31 20:00 ` Dr. David Alan Gilbert
@ 2017-02-01 13:02 ` Halil Pasic
2017-02-01 18:41 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 9+ messages in thread
From: Halil Pasic @ 2017-02-01 13:02 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Amit Shah, Juan Quintela, qemu-devel, Guenther Hutzl
On 01/31/2017 09:00 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 10/20/2016 03:25 PM, Halil Pasic wrote:
>>> 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;
>> Hi!
>>
>> It is been a while, and IMHO this is still broken, and the
>> VMSTATE_VBUFFER* macros are still only used with the start argument
>> being zero.
>>
>> What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
>> from Jan 19 we have code actually using VMStateDecription.start -- but
>> for something different (IMHO), as allocation is done by get_qtailq and
>> not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
>> Thus I would need to update the commit message and keep the start field
>> at least.
>>
>> But before I do so, I would like to ask the maintainers if there is
>> interest in a change like this?
>
> I think it's certainly worth fixing VMSTATE_BUFFER_ALLOC*; if it can't
> use the start field properly then it should have the start parameter
> removed.
Thanks for the reply! Let us try to figure out what to do together...
>
> I'll admit I can't remember the details of why field->start as an offset
> didn't work; are the other macros that take a start value correct?
I'm going to explain my view on field->start, but first let us have a look
which macros are using it:
$ pcregrep -Mi '#define VMSTATE_[^{]*{[^}]*\.start[^}]*}' include/migration/vmstate.h
#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
.name = (stringify(_field)), \
.version_id = (_version), \
.field_exists = (_test), \
.size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
.size = (_multiply), \
.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) { \
.name = (stringify(_field)), \
.version_id = (_version), \
.field_exists = (_test), \
.size_offset = vmstate_offset_value(_state, _field_size, int32_t),\
.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) { \
.name = (stringify(_field)), \
.version_id = (_version), \
.field_exists = (_test), \
.size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
.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) { \
.name = (stringify(_field)), \
.version_id = (_version), \
.field_exists = (_test), \
.size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
.info = &vmstate_info_buffer, \
.flags = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC, \
.offset = offsetof(_state, _field), \
.start = (_start), \
}
#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
{ \
.name = (stringify(_field)), \
.version_id = (_version), \
.vmsd = &(_vmsd), \
.size = sizeof(_type), \
.info = &vmstate_info_qtailq, \
.offset = offsetof(_state, _field), \
.start = offsetof(_type, _next), \
}
We both agree that VMSTATE_VBUFFER_ALLOC_UINT32 is wrong with start != 0,
and that VMSTATE_QTAILQ_V needs the data member. But I think
VMSTATE_QTAILQ_V is not using the start handling from vmstate_base_addr,
(because it has no VMS_POINTER flag set).
I think the not allocating versions of VMSTATE_VBUFFER are also fine as
is, but if someone had the bright idea to make an allocating version,
it's very straightforward to create something broken.
So my original train of thought was, because nobody uses VMSTATE_VBUFFER
with start != 0 let us get rid of the extra complexity, and eventually
reintroduce it when it's needed. This feature is around for quite some
time now, but nobody uses it, and I do not think it is very likely we
will need it in the future.
> If we can't remove the start variable then it's probably not removing
> the start parameter everywhere.
Yes we can. We just can't remove the start data member form VMStateField
because the QTAILQ stuff is now using it.
>
> That alloc case looks the most unusual in that vmstate_base_addr()
> function; I *think* the non-alloc case makes sense
> (i.e. follow a pointer and then use an offset from that pointer?)
> even if no one currently uses it.
I also think it makes sense, but as written above, it is never used.
I see 3 options regarding how to proceed:
1) Remove the start parameter from the VMSTATE_VBUFFER* macros, and
the start handling from vmstate_base_addr (basically the same I
did here, with the difference that VMStateField.start remains).
2) Proclaim that .start && (.flags & VMSTATE_ALLOC) == false is an
invariant of QEMU, and assert when we would allocate. Remove the parameter
form VMSTATRE_VBUFFER_ALLOC.
3) Make .start work properly for dynamically allocated buffers.
I prefer option 1). What is your preference?
Halil
>
> Dave
>
>> Regards,
>> Halil
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start
2017-02-01 13:02 ` Halil Pasic
@ 2017-02-01 18:41 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 9+ messages in thread
From: Dr. David Alan Gilbert @ 2017-02-01 18:41 UTC (permalink / raw)
To: Halil Pasic; +Cc: Amit Shah, Juan Quintela, qemu-devel, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 01/31/2017 09:00 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 10/20/2016 03:25 PM, Halil Pasic wrote:
> >>> 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;
> >> Hi!
> >>
> >> It is been a while, and IMHO this is still broken, and the
> >> VMSTATE_VBUFFER* macros are still only used with the start argument
> >> being zero.
> >>
> >> What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
> >> from Jan 19 we have code actually using VMStateDecription.start -- but
> >> for something different (IMHO), as allocation is done by get_qtailq and
> >> not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
> >> Thus I would need to update the commit message and keep the start field
> >> at least.
> >>
> >> But before I do so, I would like to ask the maintainers if there is
> >> interest in a change like this?
> >
> > I think it's certainly worth fixing VMSTATE_BUFFER_ALLOC*; if it can't
> > use the start field properly then it should have the start parameter
> > removed.
>
> Thanks for the reply! Let us try to figure out what to do together...
> >
> > I'll admit I can't remember the details of why field->start as an offset
> > didn't work; are the other macros that take a start value correct?
>
> I'm going to explain my view on field->start, but first let us have a look
> which macros are using it:
>
> $ pcregrep -Mi '#define VMSTATE_[^{]*{[^}]*\.start[^}]*}' include/migration/vmstate.h
nice :-)
> #define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .size = (_multiply), \
> .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) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, int32_t),\
> .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) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .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) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .info = &vmstate_info_buffer, \
> .flags = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC, \
> .offset = offsetof(_state, _field), \
> .start = (_start), \
> }
> #define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
> { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .vmsd = &(_vmsd), \
> .size = sizeof(_type), \
> .info = &vmstate_info_qtailq, \
> .offset = offsetof(_state, _field), \
> .start = offsetof(_type, _next), \
> }
>
> We both agree that VMSTATE_VBUFFER_ALLOC_UINT32 is wrong with start != 0,
> and that VMSTATE_QTAILQ_V needs the data member. But I think
> VMSTATE_QTAILQ_V is not using the start handling from vmstate_base_addr,
> (because it has no VMS_POINTER flag set).
Yes, I think I agree.
> I think the not allocating versions of VMSTATE_VBUFFER are also fine as
> is, but if someone had the bright idea to make an allocating version,
> it's very straightforward to create something broken.
Yes.
> So my original train of thought was, because nobody uses VMSTATE_VBUFFER
> with start != 0 let us get rid of the extra complexity, and eventually
> reintroduce it when it's needed. This feature is around for quite some
> time now, but nobody uses it, and I do not think it is very likely we
> will need it in the future.
>
>
> > If we can't remove the start variable then it's probably not removing
> > the start parameter everywhere.
>
> Yes we can. We just can't remove the start data member form VMStateField
> because the QTAILQ stuff is now using it.
> >
> > That alloc case looks the most unusual in that vmstate_base_addr()
> > function; I *think* the non-alloc case makes sense
> > (i.e. follow a pointer and then use an offset from that pointer?)
> > even if no one currently uses it.
>
> I also think it makes sense, but as written above, it is never used.
>
> I see 3 options regarding how to proceed:
>
> 1) Remove the start parameter from the VMSTATE_VBUFFER* macros, and
> the start handling from vmstate_base_addr (basically the same I
> did here, with the difference that VMStateField.start remains).
> 2) Proclaim that .start && (.flags & VMSTATE_ALLOC) == false is an
> invariant of QEMU, and assert when we would allocate. Remove the parameter
> form VMSTATRE_VBUFFER_ALLOC.
> 3) Make .start work properly for dynamically allocated buffers.
>
> I prefer option 1). What is your preference?
Yes OK; that probably makes sense; I slightly preferred (2) but I think
you convinced me :-)
Dave
> Halil
>
> >
> > Dave
> >
> >> Regards,
> >> Halil
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-01 18:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-20 13:25 [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 1/2] tests/test-vmstate.c: add vBuffer test Halil Pasic
2016-10-20 13:25 ` [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start Halil Pasic
2017-01-30 15:28 ` Halil Pasic
2017-01-31 20:00 ` Dr. David Alan Gilbert
2017-02-01 13:02 ` Halil Pasic
2017-02-01 18:41 ` Dr. David Alan Gilbert
2016-10-20 13:48 ` [Qemu-devel] [PATCH v2 0/2] remove unused VMSTateField.start no-reply
2016-10-31 17:33 ` Halil Pasic
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.