* [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers
@ 2016-11-08 9:55 Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 1/8] tests/test-vmstate.c: add vBuffer test Halil Pasic
` (8 more replies)
0 siblings, 9 replies; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:55 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Make VMS_ARRAY_OF_POINTER cope with null pointers. Currently the reward
for trying to migrate an array with some null pointers in it is an
illegal memory access, that is a swift and painless death of the
process. Let's make vmstate cope with this scenario at least for
pointers to structs.
We need this functionality for the migration of the channel subsystem
(hw/s390x/css.c).
The first 2 patches (1-2) are basically from a different series. Both
received favorable reviews and no criticism yet. Since things are
progressing slow there (understandably, its rather a cleanup than the
killer feature) but things are nicer with these I decided to include
them in this series to ease review.
Then 3 more cleanup patches (3,5,6) and a test coverage for the existing
functionality in patch 4 follow. Patches 3 and 4 are already included by
Juan but still not in master I'm including them here as well (for
reference see
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg00335.html
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg00329.html
).
I ended up with an RFC again because of these cleanup patches 5 and 6.
Last time they were one patch with the new functionality patch which
made things messy, and I did not receive enough feedback regarding if
these are welcomed by the community or should be dismissed as not worth
it. Here I want to point out that IMHO "split up vmstate_base_addr"
also fixes a latent bug so its not pure cleanup.
The new functionality is introduced by patch 7 and test coverage for it
in patch 8.
v1 --> v2:
* Added Reviewed-by tags
* Fixed marker for null pointer as suggested by Dave
* Split out the cleanup patches as suggested by Dave
Guenther Hutzl (1):
tests/test-vmstate.c: add vBuffer test
Halil Pasic (7):
migration: drop unused VMStateField.start
tests/test-vmstate.c: add save_buffer util func
tests/test-vmstate.c: add array of pointer to struct
migration/vmstate: renames in (load|save)_state
migration/vmstate: split up vmstate_base_addr
migration/vmstate: fix array of pointers to struct
tests/test-vmstate.c: add array of pointers to struct with NULL
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 | 27 +++--
migration/savevm.c | 2 +-
migration/vmstate.c | 91 ++++++++++------
target-s390x/machine.c | 2 +-
tests/test-vmstate.c | 250 +++++++++++++++++++++++++++++++++++++++++---
util/fifo8.c | 2 +-
17 files changed, 327 insertions(+), 79 deletions(-)
--
2.8.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 1/8] tests/test-vmstate.c: add vBuffer test
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
@ 2016-11-08 9:55 ` Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 2/8] migration: drop unused VMStateField.start Halil Pasic
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:55 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, 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>
---
_No review needed!_ Different series. See:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04791.html
---
tests/test-vmstate.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 41fd841..c6fba0d 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -471,6 +471,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);
@@ -485,6 +598,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] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 2/8] migration: drop unused VMStateField.start
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 1/8] tests/test-vmstate.c: add vBuffer test Halil Pasic
@ 2016-11-08 9:55 ` Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 3/8] tests/test-vmstate.c: add save_buffer util func Halil Pasic
` (6 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:55 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, 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>
---
_No review needed!_ Different series. See:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04792.html
---
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 | 22 ++++++++--------------
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, 31 insertions(+), 37 deletions(-)
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 885ecc0..f6a4187 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -563,7 +563,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..5940db0 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,8 @@ 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 +579,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 +589,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 +599,10 @@ 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 +610,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 +909,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 c6fba0d..78067b7 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -502,9 +502,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] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 3/8] tests/test-vmstate.c: add save_buffer util func
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 1/8] tests/test-vmstate.c: add vBuffer test Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 2/8] migration: drop unused VMStateField.start Halil Pasic
@ 2016-11-08 9:55 ` Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 4/8] tests/test-vmstate.c: add array of pointer to struct Halil Pasic
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:55 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Let us deduplicate some code by introducing an utility function for
saving a chunk of bytes (used when testing load based on wire).
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
_No review needed!_ Already included but not in master yet. See:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg00335.html
---
tests/test-vmstate.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 78067b7..9062d6b 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -79,6 +79,13 @@ static void save_vmstate(const VMStateDescription *desc, void *obj)
qemu_fclose(f);
}
+static void save_buffer(const uint8_t *buf, size_t buf_size)
+{
+ QEMUFile *fsave = open_test_file(true);
+ qemu_put_buffer(fsave, buf, buf_size);
+ qemu_fclose(fsave);
+}
+
static void compare_vmstate(uint8_t *wire, size_t size)
{
QEMUFile *f = open_test_file(false);
@@ -305,15 +312,13 @@ static const VMStateDescription vmstate_versioned = {
static void test_load_v1(void)
{
- QEMUFile *fsave = open_test_file(true);
uint8_t buf[] = {
0, 0, 0, 10, /* a */
0, 0, 0, 30, /* c */
0, 0, 0, 0, 0, 0, 0, 40, /* d */
QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
};
- qemu_put_buffer(fsave, buf, sizeof(buf));
- qemu_fclose(fsave);
+ save_buffer(buf, sizeof(buf));
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .b = 200, .e = 500, .f = 600 };
@@ -330,7 +335,6 @@ static void test_load_v1(void)
static void test_load_v2(void)
{
- QEMUFile *fsave = open_test_file(true);
uint8_t buf[] = {
0, 0, 0, 10, /* a */
0, 0, 0, 20, /* b */
@@ -340,8 +344,7 @@ static void test_load_v2(void)
0, 0, 0, 0, 0, 0, 0, 60, /* f */
QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
};
- qemu_put_buffer(fsave, buf, sizeof(buf));
- qemu_fclose(fsave);
+ save_buffer(buf, sizeof(buf));
QEMUFile *loading = open_test_file(false);
TestStruct obj;
@@ -419,7 +422,6 @@ static void test_save_skip(void)
static void test_load_noskip(void)
{
- QEMUFile *fsave = open_test_file(true);
uint8_t buf[] = {
0, 0, 0, 10, /* a */
0, 0, 0, 20, /* b */
@@ -429,8 +431,7 @@ static void test_load_noskip(void)
0, 0, 0, 0, 0, 0, 0, 60, /* f */
QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
};
- qemu_put_buffer(fsave, buf, sizeof(buf));
- qemu_fclose(fsave);
+ save_buffer(buf, sizeof(buf));
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .skip_c_e = false };
@@ -447,7 +448,6 @@ static void test_load_noskip(void)
static void test_load_skip(void)
{
- QEMUFile *fsave = open_test_file(true);
uint8_t buf[] = {
0, 0, 0, 10, /* a */
0, 0, 0, 20, /* b */
@@ -455,8 +455,7 @@ static void test_load_skip(void)
0, 0, 0, 0, 0, 0, 0, 60, /* f */
QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */
};
- qemu_put_buffer(fsave, buf, sizeof(buf));
- qemu_fclose(fsave);
+ save_buffer(buf, sizeof(buf));
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 4/8] tests/test-vmstate.c: add array of pointer to struct
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (2 preceding siblings ...)
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 3/8] tests/test-vmstate.c: add save_buffer util func Halil Pasic
@ 2016-11-08 9:55 ` Halil Pasic
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state Halil Pasic
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:55 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Increase test coverage by adding tests for the macro
VMSTATE_ARRAY_OF_POINTER_TO_STRUCT.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
_No review needed!_ Already included but not in master yet. See:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg00335.html
---
tests/test-vmstate.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index 9062d6b..aa1ccf1 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -510,6 +510,20 @@ static const VMStateDescription vmstate_vbuffer = {
}
};
+typedef struct {
+ int32_t i;
+} TestStructTriv;
+
+const VMStateDescription vmsd_tst = {
+ .name = "test/tst",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_INT32(i, TestStructTriv),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
uint8_t wire_vbuffer[] = {
/* u8_1 */ 0x64,
/* vBuffer_1 */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
@@ -582,6 +596,60 @@ static void test_complex_vbuffer(void)
}
#undef FIELD_EQUAL
#undef BUFFER_EQUAL
+#define AR_SIZE 4
+
+typedef struct {
+ TestStructTriv *ar[AR_SIZE];
+} TestArrayOfPtrToStuct;
+
+const VMStateDescription vmsd_arps = {
+ .name = "test/arps",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(ar, TestArrayOfPtrToStuct,
+ AR_SIZE, 0, vmsd_tst, TestStructTriv),
+ VMSTATE_END_OF_LIST()
+ }
+};
+static void test_arr_ptr_str_no0_save(void)
+{
+ TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+ TestArrayOfPtrToStuct sample = {.ar = {&ar[0], &ar[1], &ar[2], &ar[3]} };
+ uint8_t wire_sample[] = {
+ 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x01,
+ 0x00, 0x00, 0x00, 0x02,
+ 0x00, 0x00, 0x00, 0x03,
+ QEMU_VM_EOF
+ };
+
+ save_vmstate(&vmsd_arps, &sample);
+ compare_vmstate(wire_sample, sizeof(wire_sample));
+}
+
+static void test_arr_ptr_str_no0_load(void)
+{
+ TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+ TestStructTriv ar[AR_SIZE] = {};
+ TestArrayOfPtrToStuct obj = {.ar = {&ar[0], &ar[1], &ar[2], &ar[3]} };
+ int idx;
+ uint8_t wire_sample[] = {
+ 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x01,
+ 0x00, 0x00, 0x00, 0x02,
+ 0x00, 0x00, 0x00, 0x03,
+ QEMU_VM_EOF
+ };
+
+ save_buffer(wire_sample, sizeof(wire_sample));
+ SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+ wire_sample, sizeof(wire_sample)));
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ /* compare the target array ar with the ground truth array ar_gt */
+ g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
+ }
+}
int main(int argc, char **argv)
{
@@ -598,6 +666,10 @@ 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/array/ptr/str/no0/save",
+ test_arr_ptr_str_no0_save);
+ g_test_add_func("/vmstate/array/ptr/str/no0/load",
+ test_arr_ptr_str_no0_load);
g_test_run();
close(temp_fd);
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (3 preceding siblings ...)
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 4/8] tests/test-vmstate.c: add array of pointer to struct Halil Pasic
@ 2016-11-08 9:56 ` Halil Pasic
2016-12-15 11:55 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr Halil Pasic
` (3 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
The vmstate_(load|save)_state start out with an a void *opaque pointing
to some struct, and manipulate one or more elements of one field within
that struct.
First the field within the struct is pinpointed as opaque + offset, then
if this is a pointer the pointer is dereferenced to obtain a pointer to
the first element of the vmstate field. Pointers to further elements if
any are calculated as first_element + i * element_size (where i is the
zero based index of the element in question).
Currently base_addr and addr is used as a variable name for the pointer
to the first element and the pointer to the current element being
processed. This is suboptimal because base_addr is somewhat
counter-intuitive (because obtained as base + offset) and both base_addr
and addr not very descriptive (that we have a pointer should be clear
from the fact that it is declared as a pointer).
Let make things easier to understand by renaming base_addr to first_elem
and addr to curr_elem. This has the additional benefit of harmonizing
with other names within the scope (n_elems, vmstate_n_elems).
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
--
This patch roots in the difficulties I faced when trying to figure out
the code in question. In the meanwhile I'm quite fine with the current
names because I have it already figured out (after a couple of months
not looking on this code however, who knows). Thus my main motivation
is making things easier for the next new guy, but if the old guys say
not worth it I can very well accept that too.
---
migration/vmstate.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 8767e40..a86fb7e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -108,21 +108,21 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
field->field_exists(opaque, version_id)) ||
(!field->field_exists &&
field->version_id <= version_id)) {
- void *base_addr = vmstate_base_addr(opaque, field, true);
+ void *first_elem = vmstate_base_addr(opaque, field, true);
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
for (i = 0; i < n_elems; i++) {
- void *addr = base_addr + size * i;
+ void *curr_elem = first_elem + size * i;
if (field->flags & VMS_ARRAY_OF_POINTER) {
- addr = *(void **)addr;
+ curr_elem = *(void **)curr_elem;
}
if (field->flags & VMS_STRUCT) {
- ret = vmstate_load_state(f, field->vmsd, addr,
+ ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->vmsd->version_id);
} else {
- ret = field->info->get(f, addr, size);
+ ret = field->info->get(f, curr_elem, size);
}
if (ret >= 0) {
@@ -310,25 +310,25 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
while (field->name) {
if (!field->field_exists ||
field->field_exists(opaque, vmsd->version_id)) {
- void *base_addr = vmstate_base_addr(opaque, field, false);
+ void *first_elem = vmstate_base_addr(opaque, field, false);
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
int64_t old_offset, written_bytes;
QJSON *vmdesc_loop = vmdesc;
for (i = 0; i < n_elems; i++) {
- void *addr = base_addr + size * i;
+ void *curr_elem = first_elem + size * i;
vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
old_offset = qemu_ftell_fast(f);
-
if (field->flags & VMS_ARRAY_OF_POINTER) {
- addr = *(void **)addr;
+ assert(curr_elem);
+ curr_elem = *(void **)curr_elem;
}
if (field->flags & VMS_STRUCT) {
- vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
+ vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
} else {
- field->info->put(f, addr, size);
+ field->info->put(f, curr_elem, size);
}
written_bytes = qemu_ftell_fast(f) - old_offset;
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (4 preceding siblings ...)
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state Halil Pasic
@ 2016-11-08 9:56 ` Halil Pasic
2016-12-15 13:29 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct Halil Pasic
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Currently vmstate_base_addr does several things: it pinpoints the field
within the struct, possibly allocates memory and possibly does the first
pointer dereference. Obviously allocation is needed only for load.
Let us split up the functionality in vmstate_base_addr and move the
address manipulations (that is everything but the allocation logic) to
load and save so it becomes more obvious what is actually going on. Like
this all the address calculations (and the handling of the flags
controlling these) is in one place and the sequence is more obvious.
The newly introduced function vmstate_handle_alloc also fixes the
allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is
substantially simpler than the original vmstate_base_addr.
In load and save some asserts are added so it's easier to debug
situations where we would end up with a null pointer dereference.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
migration/vmstate.c | 41 ++++++++++++++++++-----------------------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index a86fb7e..10a7645 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -50,29 +50,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
return size;
}
-static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
-{
- void *base_addr = opaque + field->offset;
-
- if (field->flags & VMS_POINTER) {
- if (alloc && (field->flags & VMS_ALLOC)) {
- gsize size = 0;
- if (field->flags & VMS_VBUFFER) {
- size = vmstate_size(opaque, field);
- } else {
- int n_elems = vmstate_n_elems(opaque, field);
- if (n_elems) {
- size = n_elems * field->size;
- }
- }
- if (size) {
- *(void **)base_addr = g_malloc(size);
- }
+static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)
+{
+ if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
+ gsize size = vmstate_size(opaque, field);
+ size *= vmstate_n_elems(opaque, field);
+ if (size) {
+ *(void **)ptr = g_malloc(size);
}
- base_addr = *(void **)base_addr;
}
-
- return base_addr;
}
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
@@ -108,10 +94,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
field->field_exists(opaque, version_id)) ||
(!field->field_exists &&
field->version_id <= version_id)) {
- void *first_elem = vmstate_base_addr(opaque, field, true);
+ void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
+ vmstate_handle_alloc(first_elem, field, opaque);
+ if (field->flags & VMS_POINTER) {
+ first_elem = *(void **)first_elem;
+ assert(first_elem);
+ }
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
@@ -310,12 +301,16 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
while (field->name) {
if (!field->field_exists ||
field->field_exists(opaque, vmsd->version_id)) {
- void *first_elem = vmstate_base_addr(opaque, field, false);
+ void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
int64_t old_offset, written_bytes;
QJSON *vmdesc_loop = vmdesc;
+ if (field->flags & VMS_POINTER) {
+ first_elem = *(void **)first_elem;
+ assert(first_elem);
+ }
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (5 preceding siblings ...)
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr Halil Pasic
@ 2016-11-08 9:56 ` Halil Pasic
2016-12-15 12:33 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL Halil Pasic
2016-12-15 13:31 ` [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Dr. David Alan Gilbert
8 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward
for trying to migrate an array with some null pointers in it was an
illegal memory access, that is a swift and painless death of the process.
Let's make vmstate cope with this scenario at least for pointers to
structs. The general approach is when we encounter a null pointer
(element) instead of following the pointer to save/load the data behind
it we save/load a placeholder. This way we can detect if we expected a
null pointer at the load side but not null data was saved instead. Sadly
all other error scenarios are not detected by this scheme (and would
require the usage of the JSON meta data).
Limitations: Does not work for pointers to primitives.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by:
Guenther Hutzl <hutzl@linux.vnet.ibm.com> ---
We will need this to load/save some on demand created state from within
the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
example).
---
include/migration/vmstate.h | 5 +++++
migration/vmstate.c | 32 ++++++++++++++++++++++++++++++--
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 5940db0..86d4aca 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
extern const VMStateInfo vmstate_info_uint32;
extern const VMStateInfo vmstate_info_uint64;
+/** Put this in the stream when migrating a null pointer.*/
+#define VMS_NULLPTR_MARKER ((int8_t) -1)
+extern const VMStateInfo vmstate_info_nullptr;
+
extern const VMStateInfo vmstate_info_float64;
extern const VMStateInfo vmstate_info_cpudouble;
@@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
.size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
.offset = vmstate_offset_array(_s, _f, _type*, _n), \
+ .info = &vmstate_info_nullptr, \
}
#define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 10a7645..ce3490a 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (field->flags & VMS_ARRAY_OF_POINTER) {
curr_elem = *(void **)curr_elem;
}
- if (field->flags & VMS_STRUCT) {
+ if (!curr_elem) {
+ /* if null pointer check placeholder and do not follow */
+ assert(field->flags & VMS_ARRAY_OF_POINTER);
+ vmstate_info_nullptr.get(f, curr_elem, size);
+ } else if (field->flags & VMS_STRUCT) {
ret = vmstate_load_state(f, field->vmsd, curr_elem,
field->vmsd->version_id);
} else {
@@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
assert(curr_elem);
curr_elem = *(void **)curr_elem;
}
- if (field->flags & VMS_STRUCT) {
+ if (!curr_elem) {
+ /* if null pointer write placeholder and do not follow */
+ assert(field->flags & VMS_ARRAY_OF_POINTER);
+ vmstate_info_nullptr.put(f, curr_elem, size);
+ } else if (field->flags & VMS_STRUCT) {
vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
} else {
field->info->put(f, curr_elem, size);
@@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
.put = put_uint64,
};
+static int get_nullptr(QEMUFile *f, void *pv, size_t size)
+{
+ int8_t tmp;
+ qemu_get_s8s(f, &tmp);
+ return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
+}
+
+static void put_nullptr(QEMUFile *f, void *pv, size_t size)
+{
+ int8_t tmp = VMS_NULLPTR_MARKER;
+ assert(pv == NULL);
+ qemu_put_s8s(f, &tmp);
+}
+
+const VMStateInfo vmstate_info_nullptr = {
+ .name = "uint64",
+ .get = get_nullptr,
+ .put = put_nullptr,
+};
+
/* 64 bit unsigned int. See that the received value is the same than the one
in the field */
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (6 preceding siblings ...)
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct Halil Pasic
@ 2016-11-08 9:56 ` Halil Pasic
2016-12-15 12:52 ` Dr. David Alan Gilbert
2016-12-15 13:31 ` [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Dr. David Alan Gilbert
8 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2016-11-08 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: Amit Shah, Juan Quintela, Guenther Hutzl, Dr. David Alan Gilbert,
Halil Pasic
Increase coverage by testing VMSTATE_ARRAY_OF_POINTER_TO_STRUCT
with an array containing some NULL pointer.
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
---
tests/test-vmstate.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index aa1ccf1..5394119 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -651,6 +651,44 @@ static void test_arr_ptr_str_no0_load(void)
}
}
+static void test_arr_ptr_str_0_save(void)
+{
+ TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+ TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+
+ save_vmstate(&vmsd_arps, &sample); /* fails with SEGFAULT with master */
+}
+
+static void test_arr_ptr_str_0_load(void)
+{
+ TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
+ TestStructTriv ar[AR_SIZE] = {};
+ TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
+ int idx;
+ uint8_t wire_sample[] = {
+ 0x00, 0x00, 0x00, 0x00,
+ VMS_NULLPTR_MARKER,
+ 0x00, 0x00, 0x00, 0x02,
+ 0x00, 0x00, 0x00, 0x03,
+ QEMU_VM_EOF
+ };
+
+ save_buffer(wire_sample, sizeof(wire_sample));
+ SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
+ wire_sample, sizeof(wire_sample)));
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ /* compare the target array ar with the ground truth array ar_gt */
+ g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
+ }
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ if (idx == 1) {
+ g_assert_cmpint((uint64_t)(obj.ar[idx]), ==, 0);
+ } else {
+ g_assert_cmpint((uint64_t)(obj.ar[idx]), !=, 0);
+ }
+ }
+}
+
int main(int argc, char **argv)
{
temp_fd = mkstemp(temp_file);
@@ -670,6 +708,9 @@ int main(int argc, char **argv)
test_arr_ptr_str_no0_save);
g_test_add_func("/vmstate/array/ptr/str/no0/load",
test_arr_ptr_str_no0_load);
+ g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
+ g_test_add_func("/vmstate/array/ptr/str/0/load",
+ test_arr_ptr_str_0_load);
g_test_run();
close(temp_fd);
--
2.8.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state Halil Pasic
@ 2016-12-15 11:55 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-15 11:55 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> The vmstate_(load|save)_state start out with an a void *opaque pointing
> to some struct, and manipulate one or more elements of one field within
> that struct.
>
> First the field within the struct is pinpointed as opaque + offset, then
> if this is a pointer the pointer is dereferenced to obtain a pointer to
> the first element of the vmstate field. Pointers to further elements if
> any are calculated as first_element + i * element_size (where i is the
> zero based index of the element in question).
>
> Currently base_addr and addr is used as a variable name for the pointer
> to the first element and the pointer to the current element being
> processed. This is suboptimal because base_addr is somewhat
> counter-intuitive (because obtained as base + offset) and both base_addr
> and addr not very descriptive (that we have a pointer should be clear
> from the fact that it is declared as a pointer).
>
> Let make things easier to understand by renaming base_addr to first_elem
> and addr to curr_elem. This has the additional benefit of harmonizing
> with other names within the scope (n_elems, vmstate_n_elems).
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> --
>
> This patch roots in the difficulties I faced when trying to figure out
> the code in question. In the meanwhile I'm quite fine with the current
> names because I have it already figured out (after a couple of months
> not looking on this code however, who knows). Thus my main motivation
> is making things easier for the next new guy, but if the old guys say
> not worth it I can very well accept that too.
> ---
> migration/vmstate.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 8767e40..a86fb7e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -108,21 +108,21 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> field->field_exists(opaque, version_id)) ||
> (!field->field_exists &&
> field->version_id <= version_id)) {
> - void *base_addr = vmstate_base_addr(opaque, field, true);
> + void *first_elem = vmstate_base_addr(opaque, field, true);
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
>
> for (i = 0; i < n_elems; i++) {
> - void *addr = base_addr + size * i;
> + void *curr_elem = first_elem + size * i;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - addr = *(void **)addr;
> + curr_elem = *(void **)curr_elem;
> }
> if (field->flags & VMS_STRUCT) {
> - ret = vmstate_load_state(f, field->vmsd, addr,
> + ret = vmstate_load_state(f, field->vmsd, curr_elem,
> field->vmsd->version_id);
> } else {
> - ret = field->info->get(f, addr, size);
> + ret = field->info->get(f, curr_elem, size);
>
> }
> if (ret >= 0) {
> @@ -310,25 +310,25 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> while (field->name) {
> if (!field->field_exists ||
> field->field_exists(opaque, vmsd->version_id)) {
> - void *base_addr = vmstate_base_addr(opaque, field, false);
> + void *first_elem = vmstate_base_addr(opaque, field, false);
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> int64_t old_offset, written_bytes;
> QJSON *vmdesc_loop = vmdesc;
>
> for (i = 0; i < n_elems; i++) {
> - void *addr = base_addr + size * i;
> + void *curr_elem = first_elem + size * i;
>
> vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
> old_offset = qemu_ftell_fast(f);
> -
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - addr = *(void **)addr;
> + assert(curr_elem);
> + curr_elem = *(void **)curr_elem;
> }
> if (field->flags & VMS_STRUCT) {
> - vmstate_save_state(f, field->vmsd, addr, vmdesc_loop);
> + vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> } else {
> - field->info->put(f, addr, size);
> + field->info->put(f, curr_elem, size);
> }
>
> written_bytes = qemu_ftell_fast(f) - old_offset;
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct Halil Pasic
@ 2016-12-15 12:33 ` Dr. David Alan Gilbert
2017-01-31 15:17 ` Halil Pasic
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-15 12:33 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward
> for trying to migrate an array with some null pointers in it was an
> illegal memory access, that is a swift and painless death of the process.
> Let's make vmstate cope with this scenario at least for pointers to
> structs. The general approach is when we encounter a null pointer
> (element) instead of following the pointer to save/load the data behind
> it we save/load a placeholder. This way we can detect if we expected a
> null pointer at the load side but not null data was saved instead. Sadly
> all other error scenarios are not detected by this scheme (and would
> require the usage of the JSON meta data).
>
> Limitations: Does not work for pointers to primitives.
Please document that limitation in a comment in the vmstate.h near the
macros that use it.
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by:
> Guenther Hutzl <hutzl@linux.vnet.ibm.com> ---
>
> We will need this to load/save some on demand created state from within
> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
> example).
> ---
> include/migration/vmstate.h | 5 +++++
> migration/vmstate.c | 32 ++++++++++++++++++++++++++++++--
> 2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 5940db0..86d4aca 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
> extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
>
> +/** Put this in the stream when migrating a null pointer.*/
> +#define VMS_NULLPTR_MARKER ((int8_t) -1)
You seem to be making things harder by using a signed int everywhere
when you just want a byte; I suggest using '0'
> +extern const VMStateInfo vmstate_info_nullptr;
> +
> extern const VMStateInfo vmstate_info_float64;
> extern const VMStateInfo vmstate_info_cpudouble;
>
> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
> .size = sizeof(_type *), \
> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> + .info = &vmstate_info_nullptr, \
What's this change for?
> }
>
> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 10a7645..ce3490a 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> curr_elem = *(void **)curr_elem;
> }
> - if (field->flags & VMS_STRUCT) {
> + if (!curr_elem) {
> + /* if null pointer check placeholder and do not follow */
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + vmstate_info_nullptr.get(f, curr_elem, size);
> + } else if (field->flags & VMS_STRUCT) {
> ret = vmstate_load_state(f, field->vmsd, curr_elem,
> field->vmsd->version_id);
> } else {
> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> }
> - if (field->flags & VMS_STRUCT) {
> + if (!curr_elem) {
> + /* if null pointer write placeholder and do not follow */
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + vmstate_info_nullptr.put(f, curr_elem, size);
> + } else if (field->flags & VMS_STRUCT) {
> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> } else {
> field->info->put(f, curr_elem, size);
> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
> .put = put_uint64,
> };
>
> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
> +{
> + int8_t tmp;
> + qemu_get_s8s(f, &tmp);
Now that I've suggested using just a character you can turn that
into:
return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> +}
> +
> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
> +{
> + int8_t tmp = VMS_NULLPTR_MARKER;
> + assert(pv == NULL);
> + qemu_put_s8s(f, &tmp);
and similarly put_byte.
Dave
> +}
> +
> +const VMStateInfo vmstate_info_nullptr = {
> + .name = "uint64",
> + .get = get_nullptr,
> + .put = put_nullptr,
> +};
> +
> /* 64 bit unsigned int. See that the received value is the same than the one
> in the field */
>
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL Halil Pasic
@ 2016-12-15 12:52 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-15 12:52 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Increase coverage by testing VMSTATE_ARRAY_OF_POINTER_TO_STRUCT
> with an array containing some NULL pointer.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> tests/test-vmstate.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index aa1ccf1..5394119 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -651,6 +651,44 @@ static void test_arr_ptr_str_no0_load(void)
> }
> }
>
> +static void test_arr_ptr_str_0_save(void)
> +{
> + TestStructTriv ar[AR_SIZE] = {{.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
> + TestArrayOfPtrToStuct sample = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> +
> + save_vmstate(&vmsd_arps, &sample); /* fails with SEGFAULT with master */
Should probably remove that comment now; but other than that.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> +}
> +
> +static void test_arr_ptr_str_0_load(void)
> +{
> + TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
> + TestStructTriv ar[AR_SIZE] = {};
> + TestArrayOfPtrToStuct obj = {.ar = {&ar[0], NULL, &ar[2], &ar[3]} };
> + int idx;
> + uint8_t wire_sample[] = {
> + 0x00, 0x00, 0x00, 0x00,
> + VMS_NULLPTR_MARKER,
> + 0x00, 0x00, 0x00, 0x02,
> + 0x00, 0x00, 0x00, 0x03,
> + QEMU_VM_EOF
> + };
> +
> + save_buffer(wire_sample, sizeof(wire_sample));
> + SUCCESS(load_vmstate_one(&vmsd_arps, &obj, 1,
> + wire_sample, sizeof(wire_sample)));
> + for (idx = 0; idx < AR_SIZE; ++idx) {
> + /* compare the target array ar with the ground truth array ar_gt */
> + g_assert_cmpint(ar_gt[idx].i, ==, ar[idx].i);
> + }
> + for (idx = 0; idx < AR_SIZE; ++idx) {
> + if (idx == 1) {
> + g_assert_cmpint((uint64_t)(obj.ar[idx]), ==, 0);
> + } else {
> + g_assert_cmpint((uint64_t)(obj.ar[idx]), !=, 0);
> + }
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> temp_fd = mkstemp(temp_file);
> @@ -670,6 +708,9 @@ int main(int argc, char **argv)
> test_arr_ptr_str_no0_save);
> g_test_add_func("/vmstate/array/ptr/str/no0/load",
> test_arr_ptr_str_no0_load);
> + g_test_add_func("/vmstate/array/ptr/str/0/save", test_arr_ptr_str_0_save);
> + g_test_add_func("/vmstate/array/ptr/str/0/load",
> + test_arr_ptr_str_0_load);
> g_test_run();
>
> close(temp_fd);
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr Halil Pasic
@ 2016-12-15 13:29 ` Dr. David Alan Gilbert
2016-12-16 15:57 ` Halil Pasic
0 siblings, 1 reply; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-15 13:29 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Currently vmstate_base_addr does several things: it pinpoints the field
> within the struct, possibly allocates memory and possibly does the first
> pointer dereference. Obviously allocation is needed only for load.
>
> Let us split up the functionality in vmstate_base_addr and move the
> address manipulations (that is everything but the allocation logic) to
> load and save so it becomes more obvious what is actually going on. Like
> this all the address calculations (and the handling of the flags
> controlling these) is in one place and the sequence is more obvious.
>
> The newly introduced function vmstate_handle_alloc also fixes the
> allocation for the unused VMS_VBUFFER| VMS_MULTIPLY scenario and is
> substantially simpler than the original vmstate_base_addr.
>
> In load and save some asserts are added so it's easier to debug
> situations where we would end up with a null pointer dereference.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> migration/vmstate.c | 41 ++++++++++++++++++-----------------------
> 1 file changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index a86fb7e..10a7645 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -50,29 +50,15 @@ static int vmstate_size(void *opaque, VMStateField *field)
> return size;
> }
>
> -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc)
> -{
> - void *base_addr = opaque + field->offset;
> -
> - if (field->flags & VMS_POINTER) {
> - if (alloc && (field->flags & VMS_ALLOC)) {
> - gsize size = 0;
> - if (field->flags & VMS_VBUFFER) {
OK, that test isn't really needed because vmstate_size already has it.
> - size = vmstate_size(opaque, field);
> - } else {
> - int n_elems = vmstate_n_elems(opaque, field);
> - if (n_elems) {
> - size = n_elems * field->size;
> - }
> - }
> - if (size) {
> - *(void **)base_addr = g_malloc(size);
> - }
> +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque)
> +{
> + if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) {
> + gsize size = vmstate_size(opaque, field);
> + size *= vmstate_n_elems(opaque, field);
> + if (size) {
> + *(void **)ptr = g_malloc(size);
> }
> - base_addr = *(void **)base_addr;
> }
> -
> - return base_addr;
> }
>
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> @@ -108,10 +94,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> field->field_exists(opaque, version_id)) ||
> (!field->field_exists &&
> field->version_id <= version_id)) {
> - void *first_elem = vmstate_base_addr(opaque, field, true);
> + void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
>
> + vmstate_handle_alloc(first_elem, field, opaque);
> + if (field->flags & VMS_POINTER) {
> + first_elem = *(void **)first_elem;
> + assert(first_elem);
> + }
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
>
> @@ -310,12 +301,16 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> while (field->name) {
> if (!field->field_exists ||
> field->field_exists(opaque, vmsd->version_id)) {
> - void *first_elem = vmstate_base_addr(opaque, field, false);
> + void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> int64_t old_offset, written_bytes;
> QJSON *vmdesc_loop = vmdesc;
>
> + if (field->flags & VMS_POINTER) {
> + first_elem = *(void **)first_elem;
> + assert(first_elem);
Can you make that assert(first_elem || !n_elems) please.
and same above.
Dave
> + }
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
>
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
` (7 preceding siblings ...)
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL Halil Pasic
@ 2016-12-15 13:31 ` Dr. David Alan Gilbert
8 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-15 13:31 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> Make VMS_ARRAY_OF_POINTER cope with null pointers. Currently the reward
> for trying to migrate an array with some null pointers in it is an
> illegal memory access, that is a swift and painless death of the
> process. Let's make vmstate cope with this scenario at least for
> pointers to structs.
>
> Halil Pasic (7):
> migration: drop unused VMStateField.start
Note that Jianjun uses .start in the QTAILQ migration series.
Dave
> tests/test-vmstate.c: add save_buffer util func
> tests/test-vmstate.c: add array of pointer to struct
> migration/vmstate: renames in (load|save)_state
> migration/vmstate: split up vmstate_base_addr
> migration/vmstate: fix array of pointers to struct
> tests/test-vmstate.c: add array of pointers to struct with NULL
>
> 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 | 27 +++--
> migration/savevm.c | 2 +-
> migration/vmstate.c | 91 ++++++++++------
> target-s390x/machine.c | 2 +-
> tests/test-vmstate.c | 250 +++++++++++++++++++++++++++++++++++++++++---
> util/fifo8.c | 2 +-
> 17 files changed, 327 insertions(+), 79 deletions(-)
>
> --
> 2.8.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr
2016-12-15 13:29 ` Dr. David Alan Gilbert
@ 2016-12-16 15:57 ` Halil Pasic
2016-12-16 19:47 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2016-12-16 15:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
On 12/15/2016 02:29 PM, Dr. David Alan Gilbert wrote:
>> + vmstate_handle_alloc(first_elem, field, opaque);
>> + if (field->flags & VMS_POINTER) {
>> + first_elem = *(void **)first_elem;
>> + assert(first_elem);
>> + }
>> for (i = 0; i < n_elems; i++) {
>> void *curr_elem = first_elem + size * i;
>>
>> @@ -310,12 +301,16 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>> while (field->name) {
>> if (!field->field_exists ||
>> field->field_exists(opaque, vmsd->version_id)) {
>> - void *first_elem = vmstate_base_addr(opaque, field, false);
>> + void *first_elem = opaque + field->offset;
>> int i, n_elems = vmstate_n_elems(opaque, field);
>> int size = vmstate_size(opaque, field);
>> int64_t old_offset, written_bytes;
>> QJSON *vmdesc_loop = vmdesc;
>>
>> + if (field->flags & VMS_POINTER) {
>> + first_elem = *(void **)first_elem;
>> + assert(first_elem);
> Can you make that assert(first_elem || !n_elems) please.
> and same above.
>
> Dave
Good catch! This could indeed be some dynamic length
thing with 0 elements.
I'm not sure if I'm going to respin this year though.
Halil
>
>> + }
>> for (i = 0; i < n_elems; i++) {
>> void *curr_elem = first_elem + size * i;
>>
>> --
>> 2.8.4
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr
2016-12-16 15:57 ` Halil Pasic
@ 2016-12-16 19:47 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2016-12-16 19:47 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 12/15/2016 02:29 PM, Dr. David Alan Gilbert wrote:
> >> + vmstate_handle_alloc(first_elem, field, opaque);
> >> + if (field->flags & VMS_POINTER) {
> >> + first_elem = *(void **)first_elem;
> >> + assert(first_elem);
> >> + }
> >> for (i = 0; i < n_elems; i++) {
> >> void *curr_elem = first_elem + size * i;
> >>
> >> @@ -310,12 +301,16 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> while (field->name) {
> >> if (!field->field_exists ||
> >> field->field_exists(opaque, vmsd->version_id)) {
> >> - void *first_elem = vmstate_base_addr(opaque, field, false);
> >> + void *first_elem = opaque + field->offset;
> >> int i, n_elems = vmstate_n_elems(opaque, field);
> >> int size = vmstate_size(opaque, field);
> >> int64_t old_offset, written_bytes;
> >> QJSON *vmdesc_loop = vmdesc;
> >>
> >> + if (field->flags & VMS_POINTER) {
> >> + first_elem = *(void **)first_elem;
> >> + assert(first_elem);
> > Can you make that assert(first_elem || !n_elems) please.
> > and same above.
> >
> > Dave
>
> Good catch! This could indeed be some dynamic length
> thing with 0 elements.
>
> I'm not sure if I'm going to respin this year though.
Well I'm not going to review it this year if you do :-)
So have a good new year and lets see to it then!
Dave
> Halil
>
> >
> >> + }
> >> for (i = 0; i < n_elems; i++) {
> >> void *curr_elem = first_elem + size * i;
> >>
> >> --
> >> 2.8.4
> >>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct
2016-12-15 12:33 ` Dr. David Alan Gilbert
@ 2017-01-31 15:17 ` Halil Pasic
2017-02-01 18:18 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2017-01-31 15:17 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
On 12/15/2016 01:33 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward
>> for trying to migrate an array with some null pointers in it was an
>> illegal memory access, that is a swift and painless death of the process.
>> Let's make vmstate cope with this scenario at least for pointers to
>> structs. The general approach is when we encounter a null pointer
>> (element) instead of following the pointer to save/load the data behind
>> it we save/load a placeholder. This way we can detect if we expected a
>> null pointer at the load side but not null data was saved instead. Sadly
>> all other error scenarios are not detected by this scheme (and would
>> require the usage of the JSON meta data).
>>
>> Limitations: Does not work for pointers to primitives.
>
> Please document that limitation in a comment in the vmstate.h near the
> macros that use it.
>
After looking at this again I do not know what was my intention whit the
limitations statement. It was probably about arrays of pointers to
primitives, but then the statement is wrong. The same scheme does work
for primitives too. It just that I had a previous version which had this
limitation.
It could also be about just pointer (not array of pointers), but the
commit message does not mention this case at all.
I will just drop the limitations statement.
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by:
>> Guenther Hutzl <hutzl@linux.vnet.ibm.com> ---
>>
>> We will need this to load/save some on demand created state from within
>> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
>> example).
>> ---
>> include/migration/vmstate.h | 5 +++++
>> migration/vmstate.c | 32 ++++++++++++++++++++++++++++++--
>> 2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 5940db0..86d4aca 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
>> extern const VMStateInfo vmstate_info_uint32;
>> extern const VMStateInfo vmstate_info_uint64;
>>
>> +/** Put this in the stream when migrating a null pointer.*/
>> +#define VMS_NULLPTR_MARKER ((int8_t) -1)
>
> You seem to be making things harder by using a signed int everywhere
> when you just want a byte; I suggest using '0'
>
I'm fine with byte 0x30, but I have an irrational bad feeling about
using a character literal here. I know, in practice should
not matter, but the C11 standard says nothing about how members of the
basic execution character set are represented, and it's also not among
the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining
the constant/macro as Ox30U instead of '0'?
>> +extern const VMStateInfo vmstate_info_nullptr;
>> +
>> extern const VMStateInfo vmstate_info_float64;
>> extern const VMStateInfo vmstate_info_cpudouble;
>>
>> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
>> .size = sizeof(_type *), \
>> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
>> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
>> + .info = &vmstate_info_nullptr, \
>
> What's this change for?
It's garbage. A remainder of a previous version I mentioned above. Will
remove. Good catch!
>
>> }
>>
>> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 10a7645..ce3490a 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>> if (field->flags & VMS_ARRAY_OF_POINTER) {
>> curr_elem = *(void **)curr_elem;
>> }
>> - if (field->flags & VMS_STRUCT) {
>> + if (!curr_elem) {
>> + /* if null pointer check placeholder and do not follow */
>> + assert(field->flags & VMS_ARRAY_OF_POINTER);
>> + vmstate_info_nullptr.get(f, curr_elem, size);
>> + } else if (field->flags & VMS_STRUCT) {
>> ret = vmstate_load_state(f, field->vmsd, curr_elem,
>> field->vmsd->version_id);
>> } else {
>> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>> assert(curr_elem);
>> curr_elem = *(void **)curr_elem;
>> }
>> - if (field->flags & VMS_STRUCT) {
>> + if (!curr_elem) {
>> + /* if null pointer write placeholder and do not follow */
>> + assert(field->flags & VMS_ARRAY_OF_POINTER);
>> + vmstate_info_nullptr.put(f, curr_elem, size);
>> + } else if (field->flags & VMS_STRUCT) {
>> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
>> } else {
>> field->info->put(f, curr_elem, size);
>> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
>> .put = put_uint64,
>> };
>>
>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
>> +{
>> + int8_t tmp;
>> + qemu_get_s8s(f, &tmp);
>
> Now that I've suggested using just a character you can turn that
> into:
>
> return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>
You are right! I was over-complicating. Will do.
>> + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>> +}
>> +
>> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
>> +{
>> + int8_t tmp = VMS_NULLPTR_MARKER;
>> + assert(pv == NULL);
>> + qemu_put_s8s(f, &tmp);
>
> and similarly put_byte.
>
Will do.
> Dave
>
Thanks for the review. I have already adopted mostly your proposals,
but I will wait a bit in the hope that the fate of this patch borrowed
form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused
VMStateField.start" gets resolved.
By the way you could answer my question regarding that yourself, since
you became co-maintainer for migration starting 2017-01-24. Congrats :).
Regards,
Halil
>> +}
>> +
>> +const VMStateInfo vmstate_info_nullptr = {
>> + .name = "uint64",
>> + .get = get_nullptr,
>> + .put = put_nullptr,
>> +};
>> +
>> /* 64 bit unsigned int. See that the received value is the same than the one
>> in the field */
>>
>> --
>> 2.8.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct
2017-01-31 15:17 ` Halil Pasic
@ 2017-02-01 18:18 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 18+ messages in thread
From: Dr. David Alan Gilbert @ 2017-02-01 18:18 UTC (permalink / raw)
To: Halil Pasic; +Cc: qemu-devel, Amit Shah, Juan Quintela, Guenther Hutzl
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>
>
> On 12/15/2016 01:33 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward
> >> for trying to migrate an array with some null pointers in it was an
> >> illegal memory access, that is a swift and painless death of the process.
> >> Let's make vmstate cope with this scenario at least for pointers to
> >> structs. The general approach is when we encounter a null pointer
> >> (element) instead of following the pointer to save/load the data behind
> >> it we save/load a placeholder. This way we can detect if we expected a
> >> null pointer at the load side but not null data was saved instead. Sadly
> >> all other error scenarios are not detected by this scheme (and would
> >> require the usage of the JSON meta data).
> >>
> >> Limitations: Does not work for pointers to primitives.
> >
> > Please document that limitation in a comment in the vmstate.h near the
> > macros that use it.
> >
>
> After looking at this again I do not know what was my intention whit the
> limitations statement. It was probably about arrays of pointers to
> primitives, but then the statement is wrong. The same scheme does work
> for primitives too. It just that I had a previous version which had this
> limitation.
>
> It could also be about just pointer (not array of pointers), but the
> commit message does not mention this case at all.
>
> I will just drop the limitations statement.
>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reviewed-by:
> >> Guenther Hutzl <hutzl@linux.vnet.ibm.com> ---
> >>
> >> We will need this to load/save some on demand created state from within
> >> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
> >> example).
> >> ---
> >> include/migration/vmstate.h | 5 +++++
> >> migration/vmstate.c | 32 ++++++++++++++++++++++++++++++--
> >> 2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 5940db0..86d4aca 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -236,6 +236,10 @@ extern const VMStateInfo vmstate_info_uint16;
> >> extern const VMStateInfo vmstate_info_uint32;
> >> extern const VMStateInfo vmstate_info_uint64;
> >>
> >> +/** Put this in the stream when migrating a null pointer.*/
> >> +#define VMS_NULLPTR_MARKER ((int8_t) -1)
> >
> > You seem to be making things harder by using a signed int everywhere
> > when you just want a byte; I suggest using '0'
> >
>
> I'm fine with byte 0x30, but I have an irrational bad feeling about
> using a character literal here. I know, in practice should
> not matter, but the C11 standard says nothing about how members of the
> basic execution character set are represented, and it's also not among
> the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining
> the constant/macro as Ox30U instead of '0'?
0x30 is fine if you prefer (although I'd be pretty surprised if we worked
on a non-ASCII system!); My only requirements are it's just a plain normal
unsigned char and it's not the type of value that's common (like 0x00).
> >> +extern const VMStateInfo vmstate_info_nullptr;
> >> +
> >> extern const VMStateInfo vmstate_info_float64;
> >> extern const VMStateInfo vmstate_info_cpudouble;
> >>
> >> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
> >> .size = sizeof(_type *), \
> >> .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
> >> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> >> + .info = &vmstate_info_nullptr, \
> >
> > What's this change for?
>
> It's garbage. A remainder of a previous version I mentioned above. Will
> remove. Good catch!
>
> >
> >> }
> >>
> >> #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 10a7645..ce3490a 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -109,7 +109,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> if (field->flags & VMS_ARRAY_OF_POINTER) {
> >> curr_elem = *(void **)curr_elem;
> >> }
> >> - if (field->flags & VMS_STRUCT) {
> >> + if (!curr_elem) {
> >> + /* if null pointer check placeholder and do not follow */
> >> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> >> + vmstate_info_nullptr.get(f, curr_elem, size);
> >> + } else if (field->flags & VMS_STRUCT) {
> >> ret = vmstate_load_state(f, field->vmsd, curr_elem,
> >> field->vmsd->version_id);
> >> } else {
> >> @@ -320,7 +324,11 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> assert(curr_elem);
> >> curr_elem = *(void **)curr_elem;
> >> }
> >> - if (field->flags & VMS_STRUCT) {
> >> + if (!curr_elem) {
> >> + /* if null pointer write placeholder and do not follow */
> >> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> >> + vmstate_info_nullptr.put(f, curr_elem, size);
> >> + } else if (field->flags & VMS_STRUCT) {
> >> vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop);
> >> } else {
> >> field->info->put(f, curr_elem, size);
> >> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
> >> .put = put_uint64,
> >> };
> >>
> >> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> + int8_t tmp;
> >> + qemu_get_s8s(f, &tmp);
> >
> > Now that I've suggested using just a character you can turn that
> > into:
> >
> > return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> >
>
> You are right! I was over-complicating. Will do.
>
> >> + return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> >> +}
> >> +
> >> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
> >> +{
> >> + int8_t tmp = VMS_NULLPTR_MARKER;
> >> + assert(pv == NULL);
> >> + qemu_put_s8s(f, &tmp);
> >
> > and similarly put_byte.
> >
>
> Will do.
>
> > Dave
> >
>
> Thanks for the review. I have already adopted mostly your proposals,
> but I will wait a bit in the hope that the fate of this patch borrowed
> form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused
> VMStateField.start" gets resolved.
Yep, I'll look at that in a minute.
Dave
> By the way you could answer my question regarding that yourself, since
> you became co-maintainer for migration starting 2017-01-24. Congrats :).
>
>
> Regards,
> Halil
>
> >> +}
> >> +
> >> +const VMStateInfo vmstate_info_nullptr = {
> >> + .name = "uint64",
> >> + .get = get_nullptr,
> >> + .put = put_nullptr,
> >> +};
> >> +
> >> /* 64 bit unsigned int. See that the received value is the same than the one
> >> in the field */
> >>
> >> --
> >> 2.8.4
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-02-01 18:19 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-08 9:55 [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 1/8] tests/test-vmstate.c: add vBuffer test Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 2/8] migration: drop unused VMStateField.start Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 3/8] tests/test-vmstate.c: add save_buffer util func Halil Pasic
2016-11-08 9:55 ` [Qemu-devel] [RFC PATCH v2 4/8] tests/test-vmstate.c: add array of pointer to struct Halil Pasic
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 5/8] migration/vmstate: renames in (load|save)_state Halil Pasic
2016-12-15 11:55 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 6/8] migration/vmstate: split up vmstate_base_addr Halil Pasic
2016-12-15 13:29 ` Dr. David Alan Gilbert
2016-12-16 15:57 ` Halil Pasic
2016-12-16 19:47 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct Halil Pasic
2016-12-15 12:33 ` Dr. David Alan Gilbert
2017-01-31 15:17 ` Halil Pasic
2017-02-01 18:18 ` Dr. David Alan Gilbert
2016-11-08 9:56 ` [Qemu-devel] [RFC PATCH v2 8/8] tests/test-vmstate.c: add array of pointers to struct with NULL Halil Pasic
2016-12-15 12:52 ` Dr. David Alan Gilbert
2016-12-15 13:31 ` [Qemu-devel] [RFC PATCH v2 0/8] VMS_ARRAY_OF_POINTER with null pointers Dr. David Alan Gilbert
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.