* [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:19 ` Fabiano Rosas
2026-04-01 13:29 ` Philippe Mathieu-Daudé
2026-03-26 21:05 ` [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
` (9 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
Passing in a pointer almost never helps. Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
---
include/migration/vmstate.h | 6 +++---
tests/unit/test-vmstate.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index d4a39aa794..8992fba57d 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -547,9 +547,9 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num = (_num), \
.info = &(_info), \
- .size = sizeof(_type), \
+ .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
- .offset = vmstate_offset_array(_state, _field, _type, _num), \
+ .offset = vmstate_offset_array(_state, _field, _type*, _num), \
}
#define VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(_f, _s, _n, _v, _vmsd, _type) { \
@@ -1093,7 +1093,7 @@ extern const VMStateInfo vmstate_info_qlist;
VMSTATE_TIMER_PTR_V(_f, _s, 0)
#define VMSTATE_TIMER_PTR_ARRAY(_f, _s, _n) \
- VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer *)
+ VMSTATE_ARRAY_OF_POINTER(_f, _s, _n, 0, vmstate_info_timer, QEMUTimer)
#define VMSTATE_TIMER_TEST(_f, _s, _test) \
VMSTATE_SINGLE_TEST(_f, _s, _test, 0, vmstate_info_timer, QEMUTimer)
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index cadbab3c5e..6a42cc1a4e 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -668,7 +668,7 @@ const VMStateDescription vmsd_arpp = {
.minimum_version_id = 1,
.fields = (const VMStateField[]) {
VMSTATE_ARRAY_OF_POINTER(ar, TestArrayOfPtrToInt,
- AR_SIZE, 0, vmstate_info_int32, int32_t*),
+ AR_SIZE, 0, vmstate_info_int32, int32_t),
VMSTATE_END_OF_LIST()
}
};
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
2026-03-26 21:05 ` [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
@ 2026-03-27 12:19 ` Fabiano Rosas
2026-04-01 13:29 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:19 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin,
Alexander Mikhalitsyn
Peter Xu <peterx@redhat.com> writes:
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER
2026-03-26 21:05 ` [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
2026-03-27 12:19 ` Fabiano Rosas
@ 2026-04-01 13:29 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-01 13:29 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
On 26/3/26 22:05, Peter Xu wrote:
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 6 +++---
> tests/unit/test-vmstate.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index d4a39aa794..8992fba57d 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -547,9 +547,9 @@ extern const VMStateInfo vmstate_info_qlist;
> .version_id = (_version), \
> .num = (_num), \
> .info = &(_info), \
> - .size = sizeof(_type), \
> + .size = sizeof(_type *), \
> .flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
> - .offset = vmstate_offset_array(_state, _field, _type, _num), \
> + .offset = vmstate_offset_array(_state, _field, _type*, _num), \
For consistency, "_type *" with a space separator (like in following
patch), otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-26 21:05 ` [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:19 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
2026-03-26 21:05 ` [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
` (8 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
Passing in a pointer almost never helps. Convert it to pass in struct for
further refactoring on VMS_ARRAY_OF_POINTER.
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
---
include/hw/intc/riscv_aclint.h | 6 +++---
include/migration/vmstate.h | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/hw/intc/riscv_aclint.h b/include/hw/intc/riscv_aclint.h
index 5310615cbf..0e0b98acb0 100644
--- a/include/hw/intc/riscv_aclint.h
+++ b/include/hw/intc/riscv_aclint.h
@@ -80,8 +80,8 @@ enum {
RISCV_ACLINT_SWI_SIZE = 0x4000
};
-#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
-VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
- QEMUTimer *)
+#define VMSTATE_TIMER_PTR_VARRAY(_f, _s, _f_n) \
+ VMSTATE_VARRAY_OF_POINTER_UINT32(_f, _s, _f_n, 0, vmstate_info_timer, \
+ QEMUTimer)
#endif
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 8992fba57d..68fd954411 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -567,9 +567,9 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
.info = &(_info), \
- .size = sizeof(_type), \
+ .size = sizeof(_type *), \
.flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
- .offset = vmstate_offset_pointer(_state, _field, _type), \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
}
#define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
2026-03-26 21:05 ` [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
@ 2026-03-27 12:19 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:19 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin,
Alexander Mikhalitsyn
Peter Xu <peterx@redhat.com> writes:
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32
2026-03-26 21:05 ` [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
2026-03-27 12:19 ` Fabiano Rosas
@ 2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-01 13:30 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
On 26/3/26 22:05, Peter Xu wrote:
> Passing in a pointer almost never helps. Convert it to pass in struct for
> further refactoring on VMS_ARRAY_OF_POINTER.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/hw/intc/riscv_aclint.h | 6 +++---
> include/migration/vmstate.h | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-26 21:05 ` [PATCH RFC v2 01/11] vmstate: Pass in struct itself for VMSTATE_ARRAY_OF_POINTER Peter Xu
2026-03-26 21:05 ` [PATCH RFC v2 02/11] vmstate: Pass in struct itself for VMSTATE_VARRAY_OF_POINTER_UINT32 Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:20 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
2026-03-26 21:05 ` [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once Peter Xu
` (7 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
When VMS_ARRAY_OF_POINTER is specified, it means the vmstate field is an
array of pointers.
The size of the element is not relevant to whatever it is stored inside: it
is always the host pointer size.
Let's reserve the "size" field in this case for future use, update
vmstate_size() so as to make it still work for array of pointers properly.
When at this, provide rich documentation on how size / size_offset works in
vmstate.
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
---
include/migration/vmstate.h | 20 ++++++++++++++++----
migration/savevm.c | 3 +++
migration/vmstate.c | 10 +++++++++-
3 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 68fd954411..b4bc69486d 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -183,11 +183,26 @@ typedef enum {
struct VMStateField {
const char *name;
size_t offset;
+
+ /*
+ * @size or @size_offset specifies the size of the element embeded in
+ * the field. Only one of them should be present never both. When
+ * @size_offset is used together with VMS_VBUFFER, it means the size is
+ * dynamic calculated instead of a constant.
+ *
+ * When the field is an array of any type, this stores the size of one
+ * element of the array.
+ *
+ * NOTE: even if VMS_POINTER or VMS_ARRAY_OF_POINTER may be specified,
+ * this parameter always reflects the real size of the objects that a
+ * pointer point to.
+ */
size_t size;
+ size_t size_offset;
+
size_t start;
int num;
size_t num_offset;
- size_t size_offset;
const VMStateInfo *info;
enum VMStateFlags flags;
const VMStateDescription *vmsd;
@@ -547,7 +562,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num = (_num), \
.info = &(_info), \
- .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_ARRAY_OF_POINTER, \
.offset = vmstate_offset_array(_state, _field, _type*, _num), \
}
@@ -557,7 +571,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_v), \
.num = (_n), \
.vmsd = &(_vmsd), \
- .size = sizeof(_type *), \
.flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \
.offset = vmstate_offset_array(_s, _f, _type*, _n), \
}
@@ -567,7 +580,6 @@ extern const VMStateInfo vmstate_info_qlist;
.version_id = (_version), \
.num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
.info = &(_info), \
- .size = sizeof(_type *), \
.flags = VMS_VARRAY_UINT32 | VMS_ARRAY_OF_POINTER | VMS_POINTER, \
.offset = vmstate_offset_pointer(_state, _field, _type *), \
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 8115203b51..f5a6fd0c66 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -868,6 +868,9 @@ static void vmstate_check(const VMStateDescription *vmsd)
if (field) {
while (field->name) {
+ if (field->flags & VMS_ARRAY_OF_POINTER) {
+ assert(field->size == 0);
+ }
if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
/* Recurse to sub structures */
vmstate_check(field->vmsd);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e98b5f5346..e29a8c3f49 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -110,13 +110,21 @@ static int vmstate_n_elems(void *opaque, const VMStateField *field)
static int vmstate_size(void *opaque, const VMStateField *field)
{
- int size = field->size;
+ int size;
if (field->flags & VMS_VBUFFER) {
size = *(int32_t *)(opaque + field->size_offset);
if (field->flags & VMS_MULTIPLY) {
size *= field->size;
}
+ } else if (field->flags & VMS_ARRAY_OF_POINTER) {
+ /*
+ * For an array of pointer, the each element is always size of a
+ * host pointer.
+ */
+ size = sizeof(void *);
+ } else {
+ size = field->size;
}
return size;
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER
2026-03-26 21:05 ` [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
@ 2026-03-27 12:20 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:20 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin,
Alexander Mikhalitsyn
Peter Xu <peterx@redhat.com> writes:
> When VMS_ARRAY_OF_POINTER is specified, it means the vmstate field is an
> array of pointers.
>
> The size of the element is not relevant to whatever it is stored inside: it
> is always the host pointer size.
>
> Let's reserve the "size" field in this case for future use, update
> vmstate_size() so as to make it still work for array of pointers properly.
>
> When at this, provide rich documentation on how size / size_offset works in
> vmstate.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER
2026-03-26 21:05 ` [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
2026-03-27 12:20 ` Fabiano Rosas
@ 2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-01 13:30 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
On 26/3/26 22:05, Peter Xu wrote:
> When VMS_ARRAY_OF_POINTER is specified, it means the vmstate field is an
> array of pointers.
>
> The size of the element is not relevant to whatever it is stored inside: it
> is always the host pointer size.
>
> Let's reserve the "size" field in this case for future use, update
> vmstate_size() so as to make it still work for array of pointers properly.
>
> When at this, provide rich documentation on how size / size_offset works in
> vmstate.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 20 ++++++++++++++++----
> migration/savevm.c | 3 +++
> migration/vmstate.c | 10 +++++++++-
> 3 files changed, 28 insertions(+), 5 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (2 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 03/11] vmstate: Do not set size for VMS_ARRAY_OF_POINTER Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:26 ` Fabiano Rosas
2026-04-01 13:17 ` Alexander Mikhalitsyn
2026-03-26 21:05 ` [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
` (6 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin
QEMU has a trick in vmstate_save_vmsd_v(), where it will try to compress
multiple JSON entries into one with a count to avoid duplicated entries.
That only applies to the cases where vmsd_can_compress() should return
true. For example, vmsd_desc_field_start() later (who will take the
updated max_elems as the last parameter) will ignore the value passed in
when vmsd_can_compress() returns false.
Do that check once at the start of loop, and use it to update max_elems, so
that max_elems keeps 1 for uncompressable VMSD fields, which is more
straightforward.
This also paves way to make this counter work for ptr marker VMSD fields
too.
No functional change intended in this patch alone.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index e29a8c3f49..05badef42f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -556,7 +556,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
bool is_null;
- int max_elems = n_elems - i;
+ /* maximum number of elements to compress in the JSON blob */
+ int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
old_offset = qemu_file_transferred(f);
if (field->flags & VMS_ARRAY_OF_POINTER) {
@@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
* vs. nullptr). Search ahead for the next null/non-null element
* and start a new compressed array if found.
*/
- if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
+ if (vmdesc && max_elems > 1 &&
+ (field->flags & VMS_ARRAY_OF_POINTER) &&
is_null != is_prev_null) {
is_prev_null = is_null;
@@ -626,7 +628,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
/* Compressed arrays only care about the first element */
- if (vmdesc_loop && vmsd_can_compress(field)) {
+ if (vmdesc_loop && max_elems > 1) {
vmdesc_loop = NULL;
}
}
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once
2026-03-26 21:05 ` [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once Peter Xu
@ 2026-03-27 12:26 ` Fabiano Rosas
2026-04-01 13:17 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:26 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> QEMU has a trick in vmstate_save_vmsd_v(), where it will try to compress
> multiple JSON entries into one with a count to avoid duplicated entries.
>
> That only applies to the cases where vmsd_can_compress() should return
> true. For example, vmsd_desc_field_start() later (who will take the
> updated max_elems as the last parameter) will ignore the value passed in
> when vmsd_can_compress() returns false.
>
> Do that check once at the start of loop, and use it to update max_elems, so
> that max_elems keeps 1 for uncompressable VMSD fields, which is more
> straightforward.
>
> This also paves way to make this counter work for ptr marker VMSD fields
> too.
>
> No functional change intended in this patch alone.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once
2026-03-26 21:05 ` [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once Peter Xu
2026-03-27 12:26 ` Fabiano Rosas
@ 2026-04-01 13:17 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Alexander Mikhalitsyn @ 2026-04-01 13:17 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin
Am Do., 26. März 2026 um 22:05 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> QEMU has a trick in vmstate_save_vmsd_v(), where it will try to compress
> multiple JSON entries into one with a count to avoid duplicated entries.
>
> That only applies to the cases where vmsd_can_compress() should return
> true. For example, vmsd_desc_field_start() later (who will take the
> updated max_elems as the last parameter) will ignore the value passed in
> when vmsd_can_compress() returns false.
>
> Do that check once at the start of loop, and use it to update max_elems, so
> that max_elems keeps 1 for uncompressable VMSD fields, which is more
> straightforward.
>
> This also paves way to make this counter work for ptr marker VMSD fields
> too.
>
> No functional change intended in this patch alone.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index e29a8c3f49..05badef42f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -556,7 +556,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> bool is_null;
> - int max_elems = n_elems - i;
> + /* maximum number of elements to compress in the JSON blob */
> + int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
>
> old_offset = qemu_file_transferred(f);
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> @@ -587,7 +588,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> * vs. nullptr). Search ahead for the next null/non-null element
> * and start a new compressed array if found.
> */
> - if (vmdesc && (field->flags & VMS_ARRAY_OF_POINTER) &&
> + if (vmdesc && max_elems > 1 &&
> + (field->flags & VMS_ARRAY_OF_POINTER) &&
> is_null != is_prev_null) {
>
> is_prev_null = is_null;
> @@ -626,7 +628,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> /* Compressed arrays only care about the first element */
> - if (vmdesc_loop && vmsd_can_compress(field)) {
> + if (vmdesc_loop && max_elems > 1) {
> vmdesc_loop = NULL;
> }
> }
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (3 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 04/11] vmstate: Update max_elems early and check field compressable once Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:27 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
2026-03-26 21:05 ` [PATCH RFC v2 06/11] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
` (5 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
Prepare for a new MARKER for non-NULL pointer.
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
---
include/migration/vmstate.h | 2 +-
migration/vmstate-types.c | 4 ++--
tests/unit/test-vmstate.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index b4bc69486d..092e8f7e9a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -283,7 +283,7 @@ extern const VMStateInfo vmstate_info_uint64;
extern const VMStateInfo vmstate_info_fd;
/** Put this in the stream when migrating a null pointer.*/
-#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
+#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
extern const VMStateInfo vmstate_info_nullptr;
extern const VMStateInfo vmstate_info_cpudouble;
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 23f3433696..7622cf8f01 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -363,7 +363,7 @@ static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
const VMStateField *field, Error **errp)
{
- if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
+ if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
return true;
}
@@ -377,7 +377,7 @@ static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
{
if (pv == NULL) {
- qemu_put_byte(f, VMS_NULLPTR_MARKER);
+ qemu_put_byte(f, VMS_MARKER_PTR_NULL);
return true;
}
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index 6a42cc1a4e..dae15786aa 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -620,7 +620,7 @@ static void test_arr_ptr_str_no0_load(void)
static uint8_t wire_arr_ptr_0[] = {
0x00, 0x00, 0x00, 0x00,
- VMS_NULLPTR_MARKER,
+ VMS_MARKER_PTR_NULL,
0x00, 0x00, 0x00, 0x02,
0x00, 0x00, 0x00, 0x03,
QEMU_VM_EOF
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
2026-03-26 21:05 ` [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
@ 2026-03-27 12:27 ` Fabiano Rosas
2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:27 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin,
Alexander Mikhalitsyn
Peter Xu <peterx@redhat.com> writes:
> Prepare for a new MARKER for non-NULL pointer.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 2 +-
> migration/vmstate-types.c | 4 ++--
> tests/unit/test-vmstate.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index b4bc69486d..092e8f7e9a 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -283,7 +283,7 @@ extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> /** Put this in the stream when migrating a null pointer.*/
> -#define VMS_NULLPTR_MARKER (0x30U) /* '0' */
> +#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> extern const VMStateInfo vmstate_info_nullptr;
>
> extern const VMStateInfo vmstate_info_cpudouble;
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 23f3433696..7622cf8f01 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,7 +363,7 @@ static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
> + if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> return true;
> }
>
> @@ -377,7 +377,7 @@ static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
>
> {
> if (pv == NULL) {
> - qemu_put_byte(f, VMS_NULLPTR_MARKER);
> + qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> return true;
> }
>
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index 6a42cc1a4e..dae15786aa 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -620,7 +620,7 @@ static void test_arr_ptr_str_no0_load(void)
>
> static uint8_t wire_arr_ptr_0[] = {
> 0x00, 0x00, 0x00, 0x00,
> - VMS_NULLPTR_MARKER,
> + VMS_MARKER_PTR_NULL,
> 0x00, 0x00, 0x00, 0x02,
> 0x00, 0x00, 0x00, 0x03,
> QEMU_VM_EOF
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL
2026-03-26 21:05 ` [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
2026-03-27 12:27 ` Fabiano Rosas
@ 2026-04-01 13:30 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 32+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-01 13:30 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Alexander Mikhalitsyn, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
On 26/3/26 22:05, Peter Xu wrote:
> Prepare for a new MARKER for non-NULL pointer.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 2 +-
> migration/vmstate-types.c | 4 ++--
> tests/unit/test-vmstate.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 06/11] vmstate: Introduce vmstate_save_field_with_vmdesc()
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (4 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 05/11] vmstate: Rename VMS_NULLPTR_MARKER to VMS_MARKER_PTR_NULL Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:27 ` Fabiano Rosas
2026-03-26 21:05 ` [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
` (4 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
Introduce a helper to do both the JSON blob generations and save vmstate.
This further shrinks the function a bit. More importantly, we'll need to
save two fields in one loop very soon in the future with the JSON blob.
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 05badef42f..b274204e66 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -514,6 +514,35 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
return true;
}
+/*
+ * Save a whole VMSD field, including its JSON blob separately when @vmdesc
+ * is specified.
+ */
+static inline bool
+vmstate_save_field_with_vmdesc(QEMUFile *f, void *pv, size_t size,
+ const VMStateDescription *vmsd,
+ const VMStateField *field, JSONWriter *vmdesc,
+ int i, int max, Error **errp)
+{
+ uint64_t old_offset, written_bytes;
+ bool ok;
+
+ vmsd_desc_field_start(vmsd, vmdesc, field, i, max);
+
+ old_offset = qemu_file_transferred(f);
+ ok = vmstate_save_field(f, pv, size, field, vmdesc, errp);
+ written_bytes = qemu_file_transferred(f) - old_offset;
+
+ vmsd_desc_field_end(vmsd, vmdesc, field, written_bytes);
+
+ if (!ok) {
+ error_prepend(errp, "Save of field %s/%s failed: ",
+ vmsd->name, field->name);
+ }
+
+ return ok;
+}
+
static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp)
@@ -542,7 +571,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
- uint64_t old_offset, written_bytes;
JSONWriter *vmdesc_loop = vmdesc;
bool is_prev_null = false;
@@ -559,7 +587,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
/* maximum number of elements to compress in the JSON blob */
int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
- old_offset = qemu_file_transferred(f);
if (field->flags & VMS_ARRAY_OF_POINTER) {
assert(curr_elem);
curr_elem = *(void **)curr_elem;
@@ -606,15 +633,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
}
- vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
- i, max_elems);
-
- ok = vmstate_save_field(f, curr_elem, size, inner_field,
- vmdesc_loop, errp);
-
- written_bytes = qemu_file_transferred(f) - old_offset;
- vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
- written_bytes);
+ ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
+ inner_field, vmdesc_loop,
+ i, max_elems, errp);
/* If we used a fake temp field.. free it now */
if (is_null) {
@@ -622,8 +643,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
if (!ok) {
- error_prepend(errp, "Save of field %s/%s failed: ",
- vmsd->name, field->name);
goto out;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (5 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 06/11] vmstate: Introduce vmstate_save_field_with_vmdesc() Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:29 ` Fabiano Rosas
2026-04-01 13:19 ` Alexander Mikhalitsyn
2026-03-26 21:05 ` [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core Peter Xu
` (3 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin
We used to have one vmstate called "nullptr" which is only used to generate
one-byte hint to say one pointer is NULL.
Let's extend its use so that it will generate another byte to say the
pointer is non-NULL.
With that, the name of the info struct (or functions) do not apply anymore.
Update correspondingly.
Update analyze-migration.py to work with the new layout.
No functional change intended yet.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 9 +++++++--
migration/vmstate-types.c | 34 ++++++++++++++++------------------
migration/vmstate.c | 25 +++++++++++++------------
scripts/analyze-migration.py | 22 ++++++++++++----------
4 files changed, 48 insertions(+), 42 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 092e8f7e9a..2e51b5ea04 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
extern const VMStateInfo vmstate_info_uint64;
extern const VMStateInfo vmstate_info_fd;
-/** Put this in the stream when migrating a null pointer.*/
+/*
+ * Put this in the stream when migrating a pointer to reflect either a NULL
+ * or valid pointer.
+ */
#define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
-extern const VMStateInfo vmstate_info_nullptr;
+#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
+
+extern const VMStateInfo vmstate_info_ptr_marker;
extern const VMStateInfo vmstate_info_cpudouble;
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 7622cf8f01..b31689fc3c 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
.save = save_fd,
};
-static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, Error **errp)
+static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
- if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
+ int byte = qemu_get_byte(f);
+
+ if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
+ /* TODO: process PTR_VALID case */
return true;
}
- error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
+ error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
return false;
}
-static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc,
- Error **errp)
+static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
- if (pv == NULL) {
- qemu_put_byte(f, VMS_MARKER_PTR_NULL);
- return true;
- }
-
- error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
- return false;
+ qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
+ return true;
}
-const VMStateInfo vmstate_info_nullptr = {
- .name = "nullptr",
- .load = load_nullptr,
- .save = save_nullptr,
+const VMStateInfo vmstate_info_ptr_marker = {
+ .name = "ptr-marker",
+ .load = load_ptr_marker,
+ .save = save_ptr_marker,
};
/* 64 bit unsigned int. See that the received value is the same than the one
diff --git a/migration/vmstate.c b/migration/vmstate.c
index b274204e66..b333aa1744 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
}
/*
- * Create a fake nullptr field when there's a NULL pointer detected in the
+ * Create a ptr marker field when there's a NULL pointer detected in the
* array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
* can't dereference the NULL pointer.
*/
static const VMStateField *
-vmsd_create_fake_nullptr_field(const VMStateField *field)
+vmsd_create_ptr_marker_field(const VMStateField *field)
{
VMStateField *fake = g_new0(VMStateField, 1);
@@ -71,12 +71,12 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
fake->name = field->name;
fake->version_id = field->version_id;
- /* Do not need "field_exists" check as it always exists (which is null) */
+ /* Do not need "field_exists" check as it always exists */
fake->field_exists = NULL;
- /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
+ /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
fake->size = 1;
- fake->info = &vmstate_info_nullptr;
+ fake->info = &vmstate_info_ptr_marker;
fake->flags = VMS_SINGLE;
/* All the rest fields shouldn't matter.. */
@@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
* an array of pointers), use null placeholder and do
* not follow.
*/
- inner_field = vmsd_create_fake_nullptr_field(field);
+ inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
}
@@ -583,26 +583,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
- bool is_null;
/* maximum number of elements to compress in the JSON blob */
int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
+ bool use_marker_field, is_null;
if (field->flags & VMS_ARRAY_OF_POINTER) {
assert(curr_elem);
curr_elem = *(void **)curr_elem;
}
- if (!curr_elem && size) {
+ is_null = !curr_elem && size;
+ use_marker_field = is_null;
+
+ if (use_marker_field) {
/*
* If null pointer found (which should only happen in
* an array of pointers), use null placeholder and do
* not follow.
*/
- inner_field = vmsd_create_fake_nullptr_field(field);
- is_null = true;
+ inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
- is_null = false;
}
/*
@@ -638,7 +639,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
i, max_elems, errp);
/* If we used a fake temp field.. free it now */
- if (is_null) {
+ if (use_marker_field) {
g_clear_pointer((gpointer *)&inner_field, g_free);
}
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index e81deab8f9..1771ff781b 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -469,26 +469,26 @@ def __init__(self, desc, file):
super(VMSDFieldIntLE, self).__init__(desc, file)
self.dtype = '<i%d' % self.size
-class VMSDFieldNull(VMSDFieldGeneric):
+class VMSDFieldPtrMarker(VMSDFieldGeneric):
NULL_PTR_MARKER = b'0'
+ VALID_PTR_MARKER = b'1'
def __init__(self, desc, file):
- super(VMSDFieldNull, self).__init__(desc, file)
+ super(VMSDFieldPtrMarker, self).__init__(desc, file)
def __repr__(self):
- # A NULL pointer is encoded in the stream as a '0' to
- # disambiguate from a mere 0x0 value and avoid consumers
- # trying to follow the NULL pointer. Displaying '0', 0x30 or
- # 0x0 when analyzing the JSON debug stream could become
+ # A NULL / non-NULL pointer may be encoded in the stream as a
+ # '0'/'1' to represent the status of the pointer. Displaying '0',
+ # 0x30 or 0x0 when analyzing the JSON debug stream could become
# confusing, so use an explicit term instead.
- return "nullptr"
+ return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
def __str__(self):
return self.__repr__()
def read(self):
- super(VMSDFieldNull, self).read()
- assert(self.data == self.NULL_PTR_MARKER)
+ super(VMSDFieldPtrMarker, self).read()
+ assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
return self.data
class VMSDFieldBool(VMSDFieldGeneric):
@@ -642,7 +642,9 @@ def getDict(self):
"bitmap" : VMSDFieldGeneric,
"struct" : VMSDFieldStruct,
"capability": VMSDFieldCap,
- "nullptr": VMSDFieldNull,
+ # Keep the old nullptr for old binaries
+ "nullptr": VMSDFieldPtrMarker,
+ "ptr-marker": VMSDFieldPtrMarker,
"unknown" : VMSDFieldGeneric,
}
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-26 21:05 ` [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
@ 2026-03-27 12:29 ` Fabiano Rosas
2026-04-01 13:19 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:29 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> Update analyze-migration.py to work with the new layout.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/migration/vmstate.h | 9 +++++++--
> migration/vmstate-types.c | 34 ++++++++++++++++------------------
> migration/vmstate.c | 25 +++++++++++++------------
> scripts/analyze-migration.py | 22 ++++++++++++----------
> 4 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
> #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> .save = save_fd,
> };
>
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> + /* TODO: process PTR_VALID case */
> return true;
> }
>
> - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> return false;
> }
>
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc,
> - Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
>
> {
> - if (pv == NULL) {
> - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> - return true;
> - }
> -
> - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> - return false;
> + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> + return true;
> }
>
> -const VMStateInfo vmstate_info_nullptr = {
> - .name = "nullptr",
> - .load = load_nullptr,
> - .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> + .name = "ptr-marker",
> + .load = load_ptr_marker,
> + .save = save_ptr_marker,
> };
>
> /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b274204e66..b333aa1744 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> }
>
> /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
> * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> * can't dereference the NULL pointer.
> */
> static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
> {
> VMStateField *fake = g_new0(VMStateField, 1);
>
> @@ -71,12 +71,12 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
> fake->name = field->name;
> fake->version_id = field->version_id;
>
> - /* Do not need "field_exists" check as it always exists (which is null) */
> + /* Do not need "field_exists" check as it always exists */
> fake->field_exists = NULL;
>
> - /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
> + /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
> fake->size = 1;
> - fake->info = &vmstate_info_nullptr;
> + fake->info = &vmstate_info_ptr_marker;
> fake->flags = VMS_SINGLE;
>
> /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> }
> @@ -583,26 +583,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> - bool is_null;
> /* maximum number of elements to compress in the JSON blob */
> int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
> + bool use_marker_field, is_null;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> }
>
> - if (!curr_elem && size) {
> + is_null = !curr_elem && size;
> + use_marker_field = is_null;
> +
> + if (use_marker_field) {
> /*
> * If null pointer found (which should only happen in
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> - is_null = true;
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> - is_null = false;
> }
>
> /*
> @@ -638,7 +639,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> - if (is_null) {
> + if (use_marker_field) {
> g_clear_pointer((gpointer *)&inner_field, g_free);
> }
>
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index e81deab8f9..1771ff781b 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -469,26 +469,26 @@ def __init__(self, desc, file):
> super(VMSDFieldIntLE, self).__init__(desc, file)
> self.dtype = '<i%d' % self.size
>
> -class VMSDFieldNull(VMSDFieldGeneric):
> +class VMSDFieldPtrMarker(VMSDFieldGeneric):
> NULL_PTR_MARKER = b'0'
> + VALID_PTR_MARKER = b'1'
>
> def __init__(self, desc, file):
> - super(VMSDFieldNull, self).__init__(desc, file)
> + super(VMSDFieldPtrMarker, self).__init__(desc, file)
>
> def __repr__(self):
> - # A NULL pointer is encoded in the stream as a '0' to
> - # disambiguate from a mere 0x0 value and avoid consumers
> - # trying to follow the NULL pointer. Displaying '0', 0x30 or
> - # 0x0 when analyzing the JSON debug stream could become
> + # A NULL / non-NULL pointer may be encoded in the stream as a
> + # '0'/'1' to represent the status of the pointer. Displaying '0',
> + # 0x30 or 0x0 when analyzing the JSON debug stream could become
> # confusing, so use an explicit term instead.
> - return "nullptr"
> + return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
>
> def __str__(self):
> return self.__repr__()
>
> def read(self):
> - super(VMSDFieldNull, self).read()
> - assert(self.data == self.NULL_PTR_MARKER)
> + super(VMSDFieldPtrMarker, self).read()
> + assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
> return self.data
>
> class VMSDFieldBool(VMSDFieldGeneric):
> @@ -642,7 +642,9 @@ def getDict(self):
> "bitmap" : VMSDFieldGeneric,
> "struct" : VMSDFieldStruct,
> "capability": VMSDFieldCap,
> - "nullptr": VMSDFieldNull,
> + # Keep the old nullptr for old binaries
> + "nullptr": VMSDFieldPtrMarker,
> + "ptr-marker": VMSDFieldPtrMarker,
> "unknown" : VMSDFieldGeneric,
> }
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers
2026-03-26 21:05 ` [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
2026-03-27 12:29 ` Fabiano Rosas
@ 2026-04-01 13:19 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Alexander Mikhalitsyn @ 2026-04-01 13:19 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin
Am Do., 26. März 2026 um 22:05 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> Update analyze-migration.py to work with the new layout.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> include/migration/vmstate.h | 9 +++++++--
> migration/vmstate-types.c | 34 ++++++++++++++++------------------
> migration/vmstate.c | 25 +++++++++++++------------
> scripts/analyze-migration.py | 22 ++++++++++++----------
> 4 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
> #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> .save = save_fd,
> };
>
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> + /* TODO: process PTR_VALID case */
> return true;
> }
>
> - error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> return false;
> }
>
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc,
> - Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
>
> {
> - if (pv == NULL) {
> - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> - return true;
> - }
> -
> - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> - return false;
> + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> + return true;
> }
>
> -const VMStateInfo vmstate_info_nullptr = {
> - .name = "nullptr",
> - .load = load_nullptr,
> - .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> + .name = "ptr-marker",
> + .load = load_ptr_marker,
> + .save = save_ptr_marker,
> };
>
> /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b274204e66..b333aa1744 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> }
>
> /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
> * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> * can't dereference the NULL pointer.
> */
> static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
> {
> VMStateField *fake = g_new0(VMStateField, 1);
>
> @@ -71,12 +71,12 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
> fake->name = field->name;
> fake->version_id = field->version_id;
>
> - /* Do not need "field_exists" check as it always exists (which is null) */
> + /* Do not need "field_exists" check as it always exists */
> fake->field_exists = NULL;
>
> - /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
> + /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
> fake->size = 1;
> - fake->info = &vmstate_info_nullptr;
> + fake->info = &vmstate_info_ptr_marker;
> fake->flags = VMS_SINGLE;
>
> /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> }
> @@ -583,26 +583,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> - bool is_null;
> /* maximum number of elements to compress in the JSON blob */
> int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
> + bool use_marker_field, is_null;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> }
>
> - if (!curr_elem && size) {
> + is_null = !curr_elem && size;
> + use_marker_field = is_null;
> +
> + if (use_marker_field) {
> /*
> * If null pointer found (which should only happen in
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> - is_null = true;
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> - is_null = false;
> }
>
> /*
> @@ -638,7 +639,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> - if (is_null) {
> + if (use_marker_field) {
> g_clear_pointer((gpointer *)&inner_field, g_free);
> }
>
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index e81deab8f9..1771ff781b 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -469,26 +469,26 @@ def __init__(self, desc, file):
> super(VMSDFieldIntLE, self).__init__(desc, file)
> self.dtype = '<i%d' % self.size
>
> -class VMSDFieldNull(VMSDFieldGeneric):
> +class VMSDFieldPtrMarker(VMSDFieldGeneric):
> NULL_PTR_MARKER = b'0'
> + VALID_PTR_MARKER = b'1'
>
> def __init__(self, desc, file):
> - super(VMSDFieldNull, self).__init__(desc, file)
> + super(VMSDFieldPtrMarker, self).__init__(desc, file)
>
> def __repr__(self):
> - # A NULL pointer is encoded in the stream as a '0' to
> - # disambiguate from a mere 0x0 value and avoid consumers
> - # trying to follow the NULL pointer. Displaying '0', 0x30 or
> - # 0x0 when analyzing the JSON debug stream could become
> + # A NULL / non-NULL pointer may be encoded in the stream as a
> + # '0'/'1' to represent the status of the pointer. Displaying '0',
> + # 0x30 or 0x0 when analyzing the JSON debug stream could become
> # confusing, so use an explicit term instead.
> - return "nullptr"
> + return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
>
> def __str__(self):
> return self.__repr__()
>
> def read(self):
> - super(VMSDFieldNull, self).read()
> - assert(self.data == self.NULL_PTR_MARKER)
> + super(VMSDFieldPtrMarker, self).read()
> + assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
> return self.data
>
> class VMSDFieldBool(VMSDFieldGeneric):
> @@ -642,7 +642,9 @@ def getDict(self):
> "bitmap" : VMSDFieldGeneric,
> "struct" : VMSDFieldStruct,
> "capability": VMSDFieldCap,
> - "nullptr": VMSDFieldNull,
> + # Keep the old nullptr for old binaries
> + "nullptr": VMSDFieldPtrMarker,
> + "ptr-marker": VMSDFieldPtrMarker,
> "unknown" : VMSDFieldGeneric,
> }
>
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (6 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 12:30 ` Fabiano Rosas
2026-04-01 13:21 ` Alexander Mikhalitsyn
2026-03-26 21:05 ` [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (2 subsequent siblings)
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin
The loader side of ptr marker is pretty straightforward, instead of playing
the inner_field trick, just do the load manually assuming the marker layout
is a stable ABI (which it is true already).
This will remove some logic while loading VMSD, and hopefully it makes it
slightly easier to read. Unfortunately, we still need to keep the sender
side because of the JSON blob we're maintaining..
This paves way for future processing of non-NULL markers as well.
When at it, not check "size" anymore for existing NULL markers, and move it
under the same VMS_ARRAY_OF_POINTER section because that's the only place
that NULL marker can happen (which guarantess size==host ptr size, which is
non-zero).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate-types.c | 12 ++++------
migration/vmstate.c | 46 ++++++++++++++++++++++++---------------
2 files changed, 32 insertions(+), 26 deletions(-)
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index b31689fc3c..ae465c5c2c 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
const VMStateField *field, Error **errp)
{
- int byte = qemu_get_byte(f);
-
- if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
- /* TODO: process PTR_VALID case */
- return true;
- }
-
- error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
+ /*
+ * Load is done in vmstate core, see vmstate_ptr_marker_load().
+ */
+ g_assert_not_reached();
return false;
}
diff --git a/migration/vmstate.c b/migration/vmstate.c
index b333aa1744..47812eb882 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
}
}
+static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
+ Error **errp)
+{
+ int byte = qemu_get_byte(f);
+
+ if (byte == VMS_MARKER_PTR_NULL) {
+ /* When it's a null ptr marker, do not continue the load */
+ *load_field = false;
+ return true;
+ }
+
+ error_setg(errp, "Unexpected ptr marker: %d", byte);
+ return false;
+}
+
static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
Error **errp)
{
@@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
}
for (i = 0; i < n_elems; i++) {
- bool ok;
+ /* If we will process the load of field? */
+ bool load_field = true;
+ bool ok = true;
void *curr_elem = first_elem + size * i;
- const VMStateField *inner_field;
if (field->flags & VMS_ARRAY_OF_POINTER) {
curr_elem = *(void **)curr_elem;
+ if (!curr_elem) {
+ /* Read the marker instead of VMSD itself */
+ if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
+ trace_vmstate_load_field_error(field->name,
+ -EINVAL);
+ return false;
+ }
+ }
}
- if (!curr_elem && size) {
- /*
- * If null pointer found (which should only happen in
- * an array of pointers), use null placeholder and do
- * not follow.
- */
- inner_field = vmsd_create_ptr_marker_field(field);
- } else {
- inner_field = field;
- }
-
- ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
-
- /* If we used a fake temp field.. free it now */
- if (inner_field != field) {
- g_clear_pointer((gpointer *)&inner_field, g_free);
+ if (load_field) {
+ ok = vmstate_load_field(f, curr_elem, size, field, errp);
}
if (ok) {
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core
2026-03-26 21:05 ` [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core Peter Xu
@ 2026-03-27 12:30 ` Fabiano Rosas
2026-04-01 13:21 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 12:30 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> The loader side of ptr marker is pretty straightforward, instead of playing
> the inner_field trick, just do the load manually assuming the marker layout
> is a stable ABI (which it is true already).
>
> This will remove some logic while loading VMSD, and hopefully it makes it
> slightly easier to read. Unfortunately, we still need to keep the sender
> side because of the JSON blob we're maintaining..
>
> This paves way for future processing of non-NULL markers as well.
>
> When at it, not check "size" anymore for existing NULL markers, and move it
> under the same VMS_ARRAY_OF_POINTER section because that's the only place
> that NULL marker can happen (which guarantess size==host ptr size, which is
> non-zero).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/vmstate-types.c | 12 ++++------
> migration/vmstate.c | 46 ++++++++++++++++++++++++---------------
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index b31689fc3c..ae465c5c2c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - int byte = qemu_get_byte(f);
> -
> - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> - /* TODO: process PTR_VALID case */
> - return true;
> - }
> -
> - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> + /*
> + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> + */
> + g_assert_not_reached();
> return false;
> }
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b333aa1744..47812eb882 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> }
> }
>
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> + Error **errp)
> +{
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL) {
> + /* When it's a null ptr marker, do not continue the load */
> + *load_field = false;
> + return true;
> + }
> +
> + error_setg(errp, "Unexpected ptr marker: %d", byte);
> + return false;
> +}
> +
> static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> Error **errp)
> {
> @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - bool ok;
> + /* If we will process the load of field? */
> + bool load_field = true;
> + bool ok = true;
> void *curr_elem = first_elem + size * i;
> - const VMStateField *inner_field;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> curr_elem = *(void **)curr_elem;
> + if (!curr_elem) {
> + /* Read the marker instead of VMSD itself */
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> + trace_vmstate_load_field_error(field->name,
> + -EINVAL);
> + return false;
> + }
> + }
> }
>
> - if (!curr_elem && size) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> - inner_field = vmsd_create_ptr_marker_field(field);
> - } else {
> - inner_field = field;
> - }
> -
> - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> -
> - /* If we used a fake temp field.. free it now */
> - if (inner_field != field) {
> - g_clear_pointer((gpointer *)&inner_field, g_free);
> + if (load_field) {
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> }
>
> if (ok) {
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core
2026-03-26 21:05 ` [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core Peter Xu
2026-03-27 12:30 ` Fabiano Rosas
@ 2026-04-01 13:21 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Alexander Mikhalitsyn @ 2026-04-01 13:21 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin
Am Do., 26. März 2026 um 22:05 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> The loader side of ptr marker is pretty straightforward, instead of playing
> the inner_field trick, just do the load manually assuming the marker layout
> is a stable ABI (which it is true already).
>
> This will remove some logic while loading VMSD, and hopefully it makes it
> slightly easier to read. Unfortunately, we still need to keep the sender
> side because of the JSON blob we're maintaining..
>
> This paves way for future processing of non-NULL markers as well.
>
> When at it, not check "size" anymore for existing NULL markers, and move it
> under the same VMS_ARRAY_OF_POINTER section because that's the only place
> that NULL marker can happen (which guarantess size==host ptr size, which is
> non-zero).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate-types.c | 12 ++++------
> migration/vmstate.c | 46 ++++++++++++++++++++++++---------------
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index b31689fc3c..ae465c5c2c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> const VMStateField *field, Error **errp)
>
> {
> - int byte = qemu_get_byte(f);
> -
> - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> - /* TODO: process PTR_VALID case */
> - return true;
> - }
> -
> - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> + /*
> + * Load is done in vmstate core, see vmstate_ptr_marker_load().
> + */
> + g_assert_not_reached();
> return false;
> }
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b333aa1744..47812eb882 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> }
> }
>
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> + Error **errp)
> +{
> + int byte = qemu_get_byte(f);
> +
> + if (byte == VMS_MARKER_PTR_NULL) {
> + /* When it's a null ptr marker, do not continue the load */
> + *load_field = false;
> + return true;
> + }
> +
> + error_setg(errp, "Unexpected ptr marker: %d", byte);
> + return false;
> +}
> +
> static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
> Error **errp)
> {
> @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - bool ok;
> + /* If we will process the load of field? */
> + bool load_field = true;
> + bool ok = true;
> void *curr_elem = first_elem + size * i;
> - const VMStateField *inner_field;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> curr_elem = *(void **)curr_elem;
> + if (!curr_elem) {
> + /* Read the marker instead of VMSD itself */
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> + trace_vmstate_load_field_error(field->name,
> + -EINVAL);
> + return false;
> + }
> + }
> }
>
> - if (!curr_elem && size) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> - inner_field = vmsd_create_ptr_marker_field(field);
> - } else {
> - inner_field = field;
> - }
> -
> - ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> -
> - /* If we used a fake temp field.. free it now */
> - if (inner_field != field) {
> - g_clear_pointer((gpointer *)&inner_field, g_free);
> + if (load_field) {
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> }
>
> if (ok) {
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (7 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 08/11] vmstate: Implement load of ptr marker in vmstate core Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-03-27 13:12 ` Fabiano Rosas
2026-04-01 13:28 ` Alexander Mikhalitsyn
2026-03-26 21:05 ` [PATCH RFC v2 10/11] vmstate: Stop checking size for nullptr compression Peter Xu
2026-03-26 21:05 ` [PATCH RFC v2 11/11] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
10 siblings, 2 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin
Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
must be used together with VMS_ARRAY_OF_POINTER.
It can be used to allow migration of an array of pointers where the
pointers may point to NULLs.
Note that we used to allow migration of a NULL pointer within an array that
is being migrated. That corresponds to the code around vmstate_info_nullptr
where we may get/put one byte showing that the element of an array is NULL.
That usage is fine but very limited, it's because even if it will migrate a
NULL pointer with a marker, it still works in a way that both src and dest
QEMUs must know exactly which elements of the array are non-NULL, so
instead of dynamically loading an array (which can have NULL pointers), it
actually only verifies the known NULL pointers are still NULL pointers
after migration.
Also, in that case since dest QEMU knows exactly which element is NULL,
which is not NULL, dest QEMU's device code will manage all allocations for
the elements before invoking vmstate_load_vmsd().
That's not enough per evolving needs of new device states that may want to
provide real dynamic array of pointers, like what Alexander proposed here
with the NVMe device migration:
https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
This patch is an alternative approach to address the problem.
Along with the flag, introduce two new macros:
VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
Which will be used very soon in the NVMe series.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/migration/vmstate.h | 51 +++++++++++++-
migration/savevm.c | 27 ++++++-
migration/vmstate.c | 136 ++++++++++++++++++++++++++++++------
3 files changed, 190 insertions(+), 24 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 2e51b5ea04..d844b46e63 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -161,8 +161,21 @@ enum VMStateFlags {
* structure we are referencing to use. */
VMS_VSTRUCT = 0x8000,
+ /*
+ * This is a sub-flag for VMS_ARRAY_OF_POINTER. When this flag is set,
+ * VMS_ARRAY_OF_POINTER must also be set. When set, it means array
+ * elements can contain either valid or NULL pointers, vmstate core
+ * will be responsible for synchronizing the pointer status, providing
+ * proper memory allocations on the pointer when it is populated on the
+ * source QEMU. It also means the user of the field must make sure all
+ * the elements in the array are NULL pointers before loading. This
+ * should also work with VMS_ALLOC when the array itself also needs to
+ * be allocated.
+ */
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
+
/* Marker for end of list */
- VMS_END = 0x10000
+ VMS_END = 0x20000,
};
typedef enum {
@@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
.offset = vmstate_offset_array(_s, _f, _type*, _n), \
}
+/*
+ * For migrating a dynamically allocated uint{8,32}-indexed array of
+ * pointers to structures (with NULL entries and with auto memory
+ * allocation).
+ *
+ * _type: type of structure pointed to
+ * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
+ * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
+ * start: size of (_type) pointed to (for auto memory allocation)
+ */
+#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
+ _field, _state, _field_num, _version, _vmsd, _type) { \
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
+ .vmsd = &(_vmsd), \
+ .size = sizeof(_type), \
+ .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
+ VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
+}
+
+#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
+ _field, _state, _field_num, _version, _vmsd, _type) { \
+ .name = (stringify(_field)), \
+ .version_id = (_version), \
+ .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
+ .vmsd = &(_vmsd), \
+ .size = sizeof(_type), \
+ .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
+ VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
+ VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
+ .offset = vmstate_offset_pointer(_state, _field, _type *), \
+}
+
#define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
.name = (stringify(_field)), \
.version_id = (_version), \
diff --git a/migration/savevm.c b/migration/savevm.c
index f5a6fd0c66..765df8ce2d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
if (field) {
while (field->name) {
if (field->flags & VMS_ARRAY_OF_POINTER) {
- assert(field->size == 0);
+ if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ /*
+ * Size must be provided because dest QEMU needs that
+ * info to know what to allocate
+ */
+ assert(field->size || field->size_offset);
+ } else {
+ /*
+ * Otherwise size info isn't useful (because it's
+ * always the size of host pointer), detect accidental
+ * setup of sizes in this case.
+ */
+ assert(field->size == 0 && field->size_offset == 0);
+ }
+ /*
+ * VMS_ARRAY_OF_POINTER must be used only together with one
+ * of VMS_(V)ARRAY* flags.
+ */
+ assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
+ VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
+ VMS_VARRAY_UINT32));
}
+
+ if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ assert(field->flags & VMS_ARRAY_OF_POINTER);
+ }
+
if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
/* Recurse to sub structures */
vmstate_check(field->vmsd);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 47812eb882..9cd0a88ce9 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
return true;
}
+ if (byte == VMS_MARKER_PTR_VALID) {
+ /* We need to load the field right after the marker */
+ *load_field = true;
+ return true;
+ }
+
error_setg(errp, "Unexpected ptr marker: %d", byte);
return false;
}
@@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
return true;
}
+/*
+ * Try to prepare loading the next element, the object pointer to be put
+ * into @next_elem. When @next_elem is NULL, it means we should skip
+ * loading this element.
+ *
+ * Returns false for errors, in which case *errp will be set, migration
+ * must be aborted.
+ */
+static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
+ void *first_elem, void **next_elem,
+ int size, int i, Error **errp)
+{
+ bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
+ void *ptr = first_elem + size * i, **pptr;
+ bool load_field;
+
+ if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
+ /* Simplest case, no pointer involved */
+ *next_elem = ptr;
+ return true;
+ }
+
+ /*
+ * We're loading an array of pointers, switch to use pptr to make it
+ * easier to read later
+ */
+ pptr = (void **)ptr;
+
+ /*
+ * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
+ * ptr marker will always exist, or (2) the element on destination is
+ * NULL, which expects the src to send a NULL-only marker.
+ */
+ if (auto_alloc || !*pptr) {
+ if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
+ trace_vmstate_load_field_error(field->name, -EINVAL);
+ return false;
+ }
+
+ if (load_field) {
+ /*
+ * When reaching here, it means we received a non-NULL ptr
+ * marker, so we need to populate the field before loading it.
+ *
+ * NOTE: do not use vmstate_size() here, because we need the
+ * object size, not entry size of the array.
+ */
+ assert(auto_alloc);
+ *pptr = g_malloc0(field->size);
+ } else {
+ /* Clear the pointer to imply a skip */
+ *next_elem = NULL;
+ return true;
+ }
+ }
+
+ /* Move the cursor to the next element for loading */
+ *next_elem = *pptr;
+ return true;
+}
+
bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
@@ -279,27 +346,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
}
for (i = 0; i < n_elems; i++) {
- /* If we will process the load of field? */
- bool load_field = true;
- bool ok = true;
- void *curr_elem = first_elem + size * i;
+ void *curr_elem;
+ bool ok;
- if (field->flags & VMS_ARRAY_OF_POINTER) {
- curr_elem = *(void **)curr_elem;
- if (!curr_elem) {
- /* Read the marker instead of VMSD itself */
- if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
- trace_vmstate_load_field_error(field->name,
- -EINVAL);
- return false;
- }
- }
+ ok = vmstate_load_next(f, field, first_elem, &curr_elem,
+ size, i, errp);
+ if (!ok) {
+ return false;
}
- if (load_field) {
- ok = vmstate_load_field(f, curr_elem, size, field, errp);
+ if (!curr_elem) {
+ /* Implies a skip */
+ continue;
}
+ ok = vmstate_load_field(f, curr_elem, size, field, errp);
+
if (ok) {
int ret = qemu_file_get_error(f);
if (ret < 0) {
@@ -397,6 +459,16 @@ static bool vmsd_can_compress(const VMStateField *field)
return false;
}
+ if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
+ /*
+ * This may involve two VMSD fields to be saved, one for the
+ * marker to show if the pointer is NULL, followed by the real
+ * vmstate object. To make it simple at least for now, skip
+ * compression for this one.
+ */
+ return false;
+ }
+
if (field->flags & VMS_STRUCT) {
const VMStateField *sfield = field->vmsd->fields;
while (sfield->name) {
@@ -583,6 +655,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
int size = vmstate_size(opaque, field);
JSONWriter *vmdesc_loop = vmdesc;
bool is_prev_null = false;
+ /*
+ * When this is enabled, it means we will always push a ptr
+ * marker first for each element saying if it's populated.
+ */
+ bool use_dynamic_array =
+ field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
@@ -603,14 +681,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
}
is_null = !curr_elem && size;
- use_marker_field = is_null;
+ use_marker_field = use_dynamic_array || is_null;
if (use_marker_field) {
- /*
- * If null pointer found (which should only happen in
- * an array of pointers), use null placeholder and do
- * not follow.
- */
inner_field = vmsd_create_ptr_marker_field(field);
} else {
inner_field = field;
@@ -657,6 +730,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
goto out;
}
+ /*
+ * If we're using dynamic array and the element is
+ * populated, save the real object right after the marker.
+ */
+ if (use_dynamic_array && curr_elem) {
+ /*
+ * NOTE: do not use vmstate_size() here because we want
+ * to save the real VMSD object now.
+ */
+ ok = vmstate_save_field_with_vmdesc(f, curr_elem,
+ field->size, vmsd,
+ field, vmdesc_loop,
+ i, max_elems, errp);
+
+ if (!ok) {
+ goto out;
+ }
+ }
+
/* Compressed arrays only care about the first element */
if (vmdesc_loop && max_elems > 1) {
vmdesc_loop = NULL;
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-26 21:05 ` [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-27 13:12 ` Fabiano Rosas
2026-03-27 14:15 ` Peter Xu
2026-04-01 13:28 ` Alexander Mikhalitsyn
1 sibling, 1 reply; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 13:12 UTC (permalink / raw)
To: Peter Xu, qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> must be used together with VMS_ARRAY_OF_POINTER.
>
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
> VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> include/migration/vmstate.h | 51 +++++++++++++-
> migration/savevm.c | 27 ++++++-
> migration/vmstate.c | 136 ++++++++++++++++++++++++++++++------
> 3 files changed, 190 insertions(+), 24 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..d844b46e63 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,21 @@ enum VMStateFlags {
> * structure we are referencing to use. */
> VMS_VSTRUCT = 0x8000,
>
> + /*
> + * This is a sub-flag for VMS_ARRAY_OF_POINTER. When this flag is set,
> + * VMS_ARRAY_OF_POINTER must also be set. When set, it means array
> + * elements can contain either valid or NULL pointers, vmstate core
> + * will be responsible for synchronizing the pointer status, providing
> + * proper memory allocations on the pointer when it is populated on the
> + * source QEMU. It also means the user of the field must make sure all
> + * the elements in the array are NULL pointers before loading. This
> + * should also work with VMS_ALLOC when the array itself also needs to
> + * be allocated.
> + */
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
> /* Marker for end of list */
> - VMS_END = 0x10000
> + VMS_END = 0x20000,
> };
>
> typedef enum {
> @@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
>
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> + * start: size of (_type) pointed to (for auto memory allocation)
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..765df8ce2d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
> if (field) {
> while (field->name) {
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - assert(field->size == 0);
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * Size must be provided because dest QEMU needs that
> + * info to know what to allocate
> + */
> + assert(field->size || field->size_offset);
> + } else {
> + /*
> + * Otherwise size info isn't useful (because it's
> + * always the size of host pointer), detect accidental
> + * setup of sizes in this case.
> + */
> + assert(field->size == 0 && field->size_offset == 0);
> + }
> + /*
> + * VMS_ARRAY_OF_POINTER must be used only together with one
> + * of VMS_(V)ARRAY* flags.
> + */
> + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> + VMS_VARRAY_UINT32));
> }
> +
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + }
> +
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 47812eb882..9cd0a88ce9 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> return true;
> }
>
> + if (byte == VMS_MARKER_PTR_VALID) {
> + /* We need to load the field right after the marker */
> + *load_field = true;
> + return true;
> + }
> +
> error_setg(errp, "Unexpected ptr marker: %d", byte);
just checking: is this error always the right thing to do? IOW, an array
of pointers member should never be NULL unless wrapped by PTR_NULL or
PTR_VALID.
> return false;
> }
> @@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> return true;
> }
>
> +/*
> + * Try to prepare loading the next element, the object pointer to be put
> + * into @next_elem. When @next_elem is NULL, it means we should skip
> + * loading this element.
> + *
> + * Returns false for errors, in which case *errp will be set, migration
> + * must be aborted.
> + */
> +static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
> + void *first_elem, void **next_elem,
> + int size, int i, Error **errp)
> +{
> + bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> + void *ptr = first_elem + size * i, **pptr;
> + bool load_field;
> +
> + if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
> + /* Simplest case, no pointer involved */
> + *next_elem = ptr;
> + return true;
> + }
> +
> + /*
> + * We're loading an array of pointers, switch to use pptr to make it
> + * easier to read later
> + */
> + pptr = (void **)ptr;
> +
> + /*
> + * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
> + * ptr marker will always exist, or (2) the element on destination is
> + * NULL, which expects the src to send a NULL-only marker.
> + */
> + if (auto_alloc || !*pptr) {
If auto_alloc && load_field, then !*pptr must be NULL. And if [1]
!load_field, then *pptr must also be NULL. So this auto_alloc check is
not needed.
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
If !*pptr && !auto_alloc, we'll load the marker, which must be [2]
VMS_MARKER_PTR_NULL, but the vmstate_ptr_marker_load function will also
happilly accept VMS_MARKER_PTR_VALID. I guess that's what the assert
down below is for.
> + trace_vmstate_load_field_error(field->name, -EINVAL);
> + return false;
> + }
> +
> + if (load_field) {
> + /*
> + * When reaching here, it means we received a non-NULL ptr
> + * marker, so we need to populate the field before loading it.
> + *
> + * NOTE: do not use vmstate_size() here, because we need the
> + * object size, not entry size of the array.
> + */
> + assert(auto_alloc);
> + *pptr = g_malloc0(field->size);
> + } else {
> + /* Clear the pointer to imply a skip */
> + *next_elem = NULL;
nit: if !*pptr then there's no need to set *next_elem to NULL. [3]
> + return true;
> + }
> + }
What about this version:
[1] if (!*pptr) {
int byte = qemu_get_byte(f);
if (byte == VMS_MARKER_PTR_NULL) {
/* When it's a null ptr marker, do not continue the load */
[3] goto out;
}
[2] if (auto_alloc && byte == VMS_MARKER_PTR_VALID) {
/*
* When reaching here, it means we received a non-NULL ptr
* marker, so we need to populate the field before loading it.
*
* NOTE: do not use vmstate_size() here, because we need the
* object size, not entry size of the array.
*/
*pptr = g_malloc0(field->size);
} else {
error_setg(errp, "Unexpected ptr marker: %d", byte);
return false;
}
}
out:
/* Move the cursor to the next element for loading */
*next_elem = *pptr;
return true;
> +
> + /* Move the cursor to the next element for loading */
> + *next_elem = *pptr;
> + return true;
> +}
> +
> bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp)
> {
> @@ -279,27 +346,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - /* If we will process the load of field? */
> - bool load_field = true;
> - bool ok = true;
> - void *curr_elem = first_elem + size * i;
> + void *curr_elem;
> + bool ok;
>
> - if (field->flags & VMS_ARRAY_OF_POINTER) {
> - curr_elem = *(void **)curr_elem;
> - if (!curr_elem) {
> - /* Read the marker instead of VMSD itself */
> - if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> - trace_vmstate_load_field_error(field->name,
> - -EINVAL);
> - return false;
> - }
> - }
> + ok = vmstate_load_next(f, field, first_elem, &curr_elem,
> + size, i, errp);
> + if (!ok) {
> + return false;
> }
>
> - if (load_field) {
> - ok = vmstate_load_field(f, curr_elem, size, field, errp);
> + if (!curr_elem) {
> + /* Implies a skip */
> + continue;
> }
>
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> +
> if (ok) {
> int ret = qemu_file_get_error(f);
> if (ret < 0) {
> @@ -397,6 +459,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> return false;
> }
>
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * This may involve two VMSD fields to be saved, one for the
> + * marker to show if the pointer is NULL, followed by the real
> + * vmstate object. To make it simple at least for now, skip
> + * compression for this one.
> + */
> + return false;
> + }
> +
> if (field->flags & VMS_STRUCT) {
> const VMStateField *sfield = field->vmsd->fields;
> while (sfield->name) {
> @@ -583,6 +655,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> bool is_prev_null = false;
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -603,14 +681,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> is_null = !curr_elem && size;
> - use_marker_field = is_null;
> + use_marker_field = use_dynamic_array || is_null;
>
> if (use_marker_field) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> @@ -657,6 +730,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> goto out;
> }
>
> + /*
> + * If we're using dynamic array and the element is
> + * populated, save the real object right after the marker.
> + */
> + if (use_dynamic_array && curr_elem) {
> + /*
> + * NOTE: do not use vmstate_size() here because we want
> + * to save the real VMSD object now.
> + */
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> + field->size, vmsd,
> + field, vmdesc_loop,
> + i, max_elems, errp);
> +
> + if (!ok) {
> + goto out;
> + }
> + }
> +
> /* Compressed arrays only care about the first element */
> if (vmdesc_loop && max_elems > 1) {
> vmdesc_loop = NULL;
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-27 13:12 ` Fabiano Rosas
@ 2026-03-27 14:15 ` Peter Xu
2026-03-27 14:18 ` Fabiano Rosas
0 siblings, 1 reply; 32+ messages in thread
From: Peter Xu @ 2026-03-27 14:15 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin
On Fri, Mar 27, 2026 at 10:12:01AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> > must be used together with VMS_ARRAY_OF_POINTER.
> >
> > It can be used to allow migration of an array of pointers where the
> > pointers may point to NULLs.
> >
> > Note that we used to allow migration of a NULL pointer within an array that
> > is being migrated. That corresponds to the code around vmstate_info_nullptr
> > where we may get/put one byte showing that the element of an array is NULL.
> >
> > That usage is fine but very limited, it's because even if it will migrate a
> > NULL pointer with a marker, it still works in a way that both src and dest
> > QEMUs must know exactly which elements of the array are non-NULL, so
> > instead of dynamically loading an array (which can have NULL pointers), it
> > actually only verifies the known NULL pointers are still NULL pointers
> > after migration.
> >
> > Also, in that case since dest QEMU knows exactly which element is NULL,
> > which is not NULL, dest QEMU's device code will manage all allocations for
> > the elements before invoking vmstate_load_vmsd().
> >
> > That's not enough per evolving needs of new device states that may want to
> > provide real dynamic array of pointers, like what Alexander proposed here
> > with the NVMe device migration:
> >
> > https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
> >
> > This patch is an alternative approach to address the problem.
> >
> > Along with the flag, introduce two new macros:
> >
> > VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
> >
> > Which will be used very soon in the NVMe series.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > include/migration/vmstate.h | 51 +++++++++++++-
> > migration/savevm.c | 27 ++++++-
> > migration/vmstate.c | 136 ++++++++++++++++++++++++++++++------
> > 3 files changed, 190 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 2e51b5ea04..d844b46e63 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -161,8 +161,21 @@ enum VMStateFlags {
> > * structure we are referencing to use. */
> > VMS_VSTRUCT = 0x8000,
> >
> > + /*
> > + * This is a sub-flag for VMS_ARRAY_OF_POINTER. When this flag is set,
> > + * VMS_ARRAY_OF_POINTER must also be set. When set, it means array
> > + * elements can contain either valid or NULL pointers, vmstate core
> > + * will be responsible for synchronizing the pointer status, providing
> > + * proper memory allocations on the pointer when it is populated on the
> > + * source QEMU. It also means the user of the field must make sure all
> > + * the elements in the array are NULL pointers before loading. This
> > + * should also work with VMS_ALLOC when the array itself also needs to
> > + * be allocated.
> > + */
> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> > +
> > /* Marker for end of list */
> > - VMS_END = 0x10000
> > + VMS_END = 0x20000,
> > };
> >
> > typedef enum {
> > @@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
> > .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> > }
> >
> > +/*
> > + * For migrating a dynamically allocated uint{8,32}-indexed array of
> > + * pointers to structures (with NULL entries and with auto memory
> > + * allocation).
> > + *
> > + * _type: type of structure pointed to
> > + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> > + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> > + * start: size of (_type) pointed to (for auto memory allocation)
> > + */
> > +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> > + _field, _state, _field_num, _version, _vmsd, _type) { \
> > + .name = (stringify(_field)), \
> > + .version_id = (_version), \
> > + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> > + .vmsd = &(_vmsd), \
> > + .size = sizeof(_type), \
> > + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> > + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> > + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> > +}
> > +
> > +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> > + _field, _state, _field_num, _version, _vmsd, _type) { \
> > + .name = (stringify(_field)), \
> > + .version_id = (_version), \
> > + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> > + .vmsd = &(_vmsd), \
> > + .size = sizeof(_type), \
> > + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> > + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> > + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> > +}
> > +
> > #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> > .name = (stringify(_field)), \
> > .version_id = (_version), \
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index f5a6fd0c66..765df8ce2d 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
> > if (field) {
> > while (field->name) {
> > if (field->flags & VMS_ARRAY_OF_POINTER) {
> > - assert(field->size == 0);
> > + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> > + /*
> > + * Size must be provided because dest QEMU needs that
> > + * info to know what to allocate
> > + */
> > + assert(field->size || field->size_offset);
> > + } else {
> > + /*
> > + * Otherwise size info isn't useful (because it's
> > + * always the size of host pointer), detect accidental
> > + * setup of sizes in this case.
> > + */
> > + assert(field->size == 0 && field->size_offset == 0);
> > + }
> > + /*
> > + * VMS_ARRAY_OF_POINTER must be used only together with one
> > + * of VMS_(V)ARRAY* flags.
> > + */
> > + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> > + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> > + VMS_VARRAY_UINT32));
> > }
> > +
> > + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> > + assert(field->flags & VMS_ARRAY_OF_POINTER);
> > + }
> > +
> > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> > /* Recurse to sub structures */
> > vmstate_check(field->vmsd);
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 47812eb882..9cd0a88ce9 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> > return true;
> > }
> >
> > + if (byte == VMS_MARKER_PTR_VALID) {
> > + /* We need to load the field right after the marker */
> > + *load_field = true;
> > + return true;
> > + }
> > +
> > error_setg(errp, "Unexpected ptr marker: %d", byte);
>
> just checking: is this error always the right thing to do? IOW, an array
> of pointers member should never be NULL unless wrapped by PTR_NULL or
> PTR_VALID.
One thing to note is, we invoke vmstate_ptr_marker_load() only if we are
100% sure a marker is expected in the current stream we're loading.
We used to only allow that to happen if there're NULL ptrs within
VMS_ARRAY_OF_POINTER, now we extended that with _AUTO_ALLOC.
So I think this error is always the right thing to do, because it means we
expect a marker to present but when it's neither 0x30 nor 0x31 it means the
marker is definitely missing.
>
> > return false;
> > }
> > @@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> > return true;
> > }
> >
> > +/*
> > + * Try to prepare loading the next element, the object pointer to be put
> > + * into @next_elem. When @next_elem is NULL, it means we should skip
> > + * loading this element.
> > + *
> > + * Returns false for errors, in which case *errp will be set, migration
> > + * must be aborted.
> > + */
> > +static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
> > + void *first_elem, void **next_elem,
> > + int size, int i, Error **errp)
> > +{
> > + bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> > + void *ptr = first_elem + size * i, **pptr;
> > + bool load_field;
> > +
> > + if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
> > + /* Simplest case, no pointer involved */
> > + *next_elem = ptr;
> > + return true;
> > + }
> > +
> > + /*
> > + * We're loading an array of pointers, switch to use pptr to make it
> > + * easier to read later
> > + */
> > + pptr = (void **)ptr;
> > +
> > + /*
> > + * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
> > + * ptr marker will always exist, or (2) the element on destination is
> > + * NULL, which expects the src to send a NULL-only marker.
> > + */
> > + if (auto_alloc || !*pptr) {
>
> If auto_alloc && load_field, then !*pptr must be NULL. And if [1]
> !load_field, then *pptr must also be NULL. So this auto_alloc check is
> not needed.
Good point, I can simply this check and perhaps add a prior assertion
instead:
/*
* If auto_alloc is on, making sure the user provided an array of NULL
* pointers to start with
*/
assert(!auto_alloc || *pptr == NULL);
I used to have similar assertion in the old version, but I lost that when
addressing the rfcv1 comments.
>
> > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
>
> If !*pptr && !auto_alloc, we'll load the marker, which must be [2]
> VMS_MARKER_PTR_NULL, but the vmstate_ptr_marker_load function will also
> happilly accept VMS_MARKER_PTR_VALID. I guess that's what the assert
> down below is for.
Yes.
>
> > + trace_vmstate_load_field_error(field->name, -EINVAL);
> > + return false;
> > + }
> > +
> > + if (load_field) {
> > + /*
> > + * When reaching here, it means we received a non-NULL ptr
> > + * marker, so we need to populate the field before loading it.
> > + *
> > + * NOTE: do not use vmstate_size() here, because we need the
> > + * object size, not entry size of the array.
> > + */
> > + assert(auto_alloc);
> > + *pptr = g_malloc0(field->size);
> > + } else {
> > + /* Clear the pointer to imply a skip */
> > + *next_elem = NULL;
>
> nit: if !*pptr then there's no need to set *next_elem to NULL. [3]
Indeed; maybe this will make the code slightly harder to follow who will
read it the first time.. but it's no problem, I can add a comment while
removing this branch.
>
> > + return true;
> > + }
> > + }
>
> What about this version:
>
> [1] if (!*pptr) {
This one I agree.
> int byte = qemu_get_byte(f);
>
> if (byte == VMS_MARKER_PTR_NULL) {
> /* When it's a null ptr marker, do not continue the load */
> [3] goto out;
> }
>
> [2] if (auto_alloc && byte == VMS_MARKER_PTR_VALID) {
> /*
> * When reaching here, it means we received a non-NULL ptr
> * marker, so we need to populate the field before loading it.
> *
> * NOTE: do not use vmstate_size() here, because we need the
> * object size, not entry size of the array.
> */
> *pptr = g_malloc0(field->size);
> } else {
> error_setg(errp, "Unexpected ptr marker: %d", byte);
We'll need to attach the trace_*() to not lose capturing all errors in
tracepoints.
> return false;
> }
> }
Let's do apples-to-apples compare, removing comments.
This is the current version after I address 1+3:
if (!*pptr) {
if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
trace_vmstate_load_field_error(field->name, -EINVAL);
return false;
}
if (load_field) {
assert(auto_alloc);
*pptr = g_malloc0(field->size);
}
}
This is the suggested version (added tracepoint back):
if (!*pptr) {
int byte = qemu_get_byte(f);
if (byte == VMS_MARKER_PTR_NULL) {
goto out;
}
if (auto_alloc && byte == VMS_MARKER_PTR_VALID) {
*pptr = g_malloc0(field->size);
} else {
error_setg(errp, "Unexpected ptr marker: %d", byte);
trace_vmstate_load_field_error(field->name, -EINVAL);
return false;
}
}
I slightly prefer the amended version in a few things:
(1) vmstate_ptr_marker_load() small helper provides standalone logic on
marker loads, slightly more readable. Meanwhile, this function invoked
at the top says "there must be a marker present", so it is hopefully
more readable too on understading the expected stream format (rather
than reading a byte and further process, which seems to mean it may or
may not be a marker).
(2) when auto_alloc is specified but marker is missing, it will assert
instead of setting error. I think it makes more sense because it is a
programming error and it should never be triggerable from a valid user.
In general, I slightly prefer seperations of "loading the marker" process,
versus "processing the marker" process.
>
> out:
> /* Move the cursor to the next element for loading */
> *next_elem = *pptr;
> return true;
>
> > +
> > + /* Move the cursor to the next element for loading */
> > + *next_elem = *pptr;
> > + return true;
> > +}
> > +
> > bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> > void *opaque, int version_id, Error **errp)
> > {
> > @@ -279,27 +346,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> > }
> >
> > for (i = 0; i < n_elems; i++) {
> > - /* If we will process the load of field? */
> > - bool load_field = true;
> > - bool ok = true;
> > - void *curr_elem = first_elem + size * i;
> > + void *curr_elem;
> > + bool ok;
> >
> > - if (field->flags & VMS_ARRAY_OF_POINTER) {
> > - curr_elem = *(void **)curr_elem;
> > - if (!curr_elem) {
> > - /* Read the marker instead of VMSD itself */
> > - if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> > - trace_vmstate_load_field_error(field->name,
> > - -EINVAL);
> > - return false;
> > - }
> > - }
> > + ok = vmstate_load_next(f, field, first_elem, &curr_elem,
> > + size, i, errp);
> > + if (!ok) {
> > + return false;
> > }
> >
> > - if (load_field) {
> > - ok = vmstate_load_field(f, curr_elem, size, field, errp);
> > + if (!curr_elem) {
> > + /* Implies a skip */
> > + continue;
> > }
> >
> > + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> > +
> > if (ok) {
> > int ret = qemu_file_get_error(f);
> > if (ret < 0) {
> > @@ -397,6 +459,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> > return false;
> > }
> >
> > + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> > + /*
> > + * This may involve two VMSD fields to be saved, one for the
> > + * marker to show if the pointer is NULL, followed by the real
> > + * vmstate object. To make it simple at least for now, skip
> > + * compression for this one.
> > + */
> > + return false;
> > + }
> > +
> > if (field->flags & VMS_STRUCT) {
> > const VMStateField *sfield = field->vmsd->fields;
> > while (sfield->name) {
> > @@ -583,6 +655,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> > int size = vmstate_size(opaque, field);
> > JSONWriter *vmdesc_loop = vmdesc;
> > bool is_prev_null = false;
> > + /*
> > + * When this is enabled, it means we will always push a ptr
> > + * marker first for each element saying if it's populated.
> > + */
> > + bool use_dynamic_array =
> > + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> >
> > trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> > if (field->flags & VMS_POINTER) {
> > @@ -603,14 +681,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> > }
> >
> > is_null = !curr_elem && size;
> > - use_marker_field = is_null;
> > + use_marker_field = use_dynamic_array || is_null;
> >
> > if (use_marker_field) {
> > - /*
> > - * If null pointer found (which should only happen in
> > - * an array of pointers), use null placeholder and do
> > - * not follow.
> > - */
> > inner_field = vmsd_create_ptr_marker_field(field);
> > } else {
> > inner_field = field;
> > @@ -657,6 +730,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> > goto out;
> > }
> >
> > + /*
> > + * If we're using dynamic array and the element is
> > + * populated, save the real object right after the marker.
> > + */
> > + if (use_dynamic_array && curr_elem) {
> > + /*
> > + * NOTE: do not use vmstate_size() here because we want
> > + * to save the real VMSD object now.
> > + */
> > + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> > + field->size, vmsd,
> > + field, vmdesc_loop,
> > + i, max_elems, errp);
> > +
> > + if (!ok) {
> > + goto out;
> > + }
> > + }
> > +
> > /* Compressed arrays only care about the first element */
> > if (vmdesc_loop && max_elems > 1) {
> > vmdesc_loop = NULL;
>
--
Peter Xu
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-27 14:15 ` Peter Xu
@ 2026-03-27 14:18 ` Fabiano Rosas
0 siblings, 0 replies; 32+ messages in thread
From: Fabiano Rosas @ 2026-03-27 14:18 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Alexander Mikhalitsyn, Juraj Marcin
Peter Xu <peterx@redhat.com> writes:
> On Fri, Mar 27, 2026 at 10:12:01AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>> > Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
>> > must be used together with VMS_ARRAY_OF_POINTER.
>> >
>> > It can be used to allow migration of an array of pointers where the
>> > pointers may point to NULLs.
>> >
>> > Note that we used to allow migration of a NULL pointer within an array that
>> > is being migrated. That corresponds to the code around vmstate_info_nullptr
>> > where we may get/put one byte showing that the element of an array is NULL.
>> >
>> > That usage is fine but very limited, it's because even if it will migrate a
>> > NULL pointer with a marker, it still works in a way that both src and dest
>> > QEMUs must know exactly which elements of the array are non-NULL, so
>> > instead of dynamically loading an array (which can have NULL pointers), it
>> > actually only verifies the known NULL pointers are still NULL pointers
>> > after migration.
>> >
>> > Also, in that case since dest QEMU knows exactly which element is NULL,
>> > which is not NULL, dest QEMU's device code will manage all allocations for
>> > the elements before invoking vmstate_load_vmsd().
>> >
>> > That's not enough per evolving needs of new device states that may want to
>> > provide real dynamic array of pointers, like what Alexander proposed here
>> > with the NVMe device migration:
>> >
>> > https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>> >
>> > This patch is an alternative approach to address the problem.
>> >
>> > Along with the flag, introduce two new macros:
>> >
>> > VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>> >
>> > Which will be used very soon in the NVMe series.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> > include/migration/vmstate.h | 51 +++++++++++++-
>> > migration/savevm.c | 27 ++++++-
>> > migration/vmstate.c | 136 ++++++++++++++++++++++++++++++------
>> > 3 files changed, 190 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> > index 2e51b5ea04..d844b46e63 100644
>> > --- a/include/migration/vmstate.h
>> > +++ b/include/migration/vmstate.h
>> > @@ -161,8 +161,21 @@ enum VMStateFlags {
>> > * structure we are referencing to use. */
>> > VMS_VSTRUCT = 0x8000,
>> >
>> > + /*
>> > + * This is a sub-flag for VMS_ARRAY_OF_POINTER. When this flag is set,
>> > + * VMS_ARRAY_OF_POINTER must also be set. When set, it means array
>> > + * elements can contain either valid or NULL pointers, vmstate core
>> > + * will be responsible for synchronizing the pointer status, providing
>> > + * proper memory allocations on the pointer when it is populated on the
>> > + * source QEMU. It also means the user of the field must make sure all
>> > + * the elements in the array are NULL pointers before loading. This
>> > + * should also work with VMS_ALLOC when the array itself also needs to
>> > + * be allocated.
>> > + */
>> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
>> > +
>> > /* Marker for end of list */
>> > - VMS_END = 0x10000
>> > + VMS_END = 0x20000,
>> > };
>> >
>> > typedef enum {
>> > @@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
>> > .offset = vmstate_offset_array(_s, _f, _type*, _n), \
>> > }
>> >
>> > +/*
>> > + * For migrating a dynamically allocated uint{8,32}-indexed array of
>> > + * pointers to structures (with NULL entries and with auto memory
>> > + * allocation).
>> > + *
>> > + * _type: type of structure pointed to
>> > + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
>> > + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
>> > + * start: size of (_type) pointed to (for auto memory allocation)
>> > + */
>> > +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
>> > + _field, _state, _field_num, _version, _vmsd, _type) { \
>> > + .name = (stringify(_field)), \
>> > + .version_id = (_version), \
>> > + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
>> > + .vmsd = &(_vmsd), \
>> > + .size = sizeof(_type), \
>> > + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
>> > + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
>> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
>> > + .offset = vmstate_offset_pointer(_state, _field, _type *), \
>> > +}
>> > +
>> > +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
>> > + _field, _state, _field_num, _version, _vmsd, _type) { \
>> > + .name = (stringify(_field)), \
>> > + .version_id = (_version), \
>> > + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
>> > + .vmsd = &(_vmsd), \
>> > + .size = sizeof(_type), \
>> > + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
>> > + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
>> > + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
>> > + .offset = vmstate_offset_pointer(_state, _field, _type *), \
>> > +}
>> > +
>> > #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
>> > .name = (stringify(_field)), \
>> > .version_id = (_version), \
>> > diff --git a/migration/savevm.c b/migration/savevm.c
>> > index f5a6fd0c66..765df8ce2d 100644
>> > --- a/migration/savevm.c
>> > +++ b/migration/savevm.c
>> > @@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
>> > if (field) {
>> > while (field->name) {
>> > if (field->flags & VMS_ARRAY_OF_POINTER) {
>> > - assert(field->size == 0);
>> > + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
>> > + /*
>> > + * Size must be provided because dest QEMU needs that
>> > + * info to know what to allocate
>> > + */
>> > + assert(field->size || field->size_offset);
>> > + } else {
>> > + /*
>> > + * Otherwise size info isn't useful (because it's
>> > + * always the size of host pointer), detect accidental
>> > + * setup of sizes in this case.
>> > + */
>> > + assert(field->size == 0 && field->size_offset == 0);
>> > + }
>> > + /*
>> > + * VMS_ARRAY_OF_POINTER must be used only together with one
>> > + * of VMS_(V)ARRAY* flags.
>> > + */
>> > + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
>> > + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
>> > + VMS_VARRAY_UINT32));
>> > }
>> > +
>> > + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
>> > + assert(field->flags & VMS_ARRAY_OF_POINTER);
>> > + }
>> > +
>> > if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
>> > /* Recurse to sub structures */
>> > vmstate_check(field->vmsd);
>> > diff --git a/migration/vmstate.c b/migration/vmstate.c
>> > index 47812eb882..9cd0a88ce9 100644
>> > --- a/migration/vmstate.c
>> > +++ b/migration/vmstate.c
>> > @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
>> > return true;
>> > }
>> >
>> > + if (byte == VMS_MARKER_PTR_VALID) {
>> > + /* We need to load the field right after the marker */
>> > + *load_field = true;
>> > + return true;
>> > + }
>> > +
>> > error_setg(errp, "Unexpected ptr marker: %d", byte);
>>
>> just checking: is this error always the right thing to do? IOW, an array
>> of pointers member should never be NULL unless wrapped by PTR_NULL or
>> PTR_VALID.
>
> One thing to note is, we invoke vmstate_ptr_marker_load() only if we are
> 100% sure a marker is expected in the current stream we're loading.
>
> We used to only allow that to happen if there're NULL ptrs within
> VMS_ARRAY_OF_POINTER, now we extended that with _AUTO_ALLOC.
>
> So I think this error is always the right thing to do, because it means we
> expect a marker to present but when it's neither 0x30 nor 0x31 it means the
> marker is definitely missing.
>
>>
>> > return false;
>> > }
>> > @@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
>> > return true;
>> > }
>> >
>> > +/*
>> > + * Try to prepare loading the next element, the object pointer to be put
>> > + * into @next_elem. When @next_elem is NULL, it means we should skip
>> > + * loading this element.
>> > + *
>> > + * Returns false for errors, in which case *errp will be set, migration
>> > + * must be aborted.
>> > + */
>> > +static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
>> > + void *first_elem, void **next_elem,
>> > + int size, int i, Error **errp)
>> > +{
>> > + bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>> > + void *ptr = first_elem + size * i, **pptr;
>> > + bool load_field;
>> > +
>> > + if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
>> > + /* Simplest case, no pointer involved */
>> > + *next_elem = ptr;
>> > + return true;
>> > + }
>> > +
>> > + /*
>> > + * We're loading an array of pointers, switch to use pptr to make it
>> > + * easier to read later
>> > + */
>> > + pptr = (void **)ptr;
>> > +
>> > + /*
>> > + * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
>> > + * ptr marker will always exist, or (2) the element on destination is
>> > + * NULL, which expects the src to send a NULL-only marker.
>> > + */
>> > + if (auto_alloc || !*pptr) {
>>
>> If auto_alloc && load_field, then !*pptr must be NULL. And if [1]
>> !load_field, then *pptr must also be NULL. So this auto_alloc check is
>> not needed.
>
> Good point, I can simply this check and perhaps add a prior assertion
> instead:
>
> /*
> * If auto_alloc is on, making sure the user provided an array of NULL
> * pointers to start with
> */
> assert(!auto_alloc || *pptr == NULL);
>
> I used to have similar assertion in the old version, but I lost that when
> addressing the rfcv1 comments.
>
>>
>> > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
>>
>> If !*pptr && !auto_alloc, we'll load the marker, which must be [2]
>> VMS_MARKER_PTR_NULL, but the vmstate_ptr_marker_load function will also
>> happilly accept VMS_MARKER_PTR_VALID. I guess that's what the assert
>> down below is for.
>
> Yes.
>
>>
>> > + trace_vmstate_load_field_error(field->name, -EINVAL);
>> > + return false;
>> > + }
>> > +
>> > + if (load_field) {
>> > + /*
>> > + * When reaching here, it means we received a non-NULL ptr
>> > + * marker, so we need to populate the field before loading it.
>> > + *
>> > + * NOTE: do not use vmstate_size() here, because we need the
>> > + * object size, not entry size of the array.
>> > + */
>> > + assert(auto_alloc);
>> > + *pptr = g_malloc0(field->size);
>> > + } else {
>> > + /* Clear the pointer to imply a skip */
>> > + *next_elem = NULL;
>>
>> nit: if !*pptr then there's no need to set *next_elem to NULL. [3]
>
> Indeed; maybe this will make the code slightly harder to follow who will
> read it the first time.. but it's no problem, I can add a comment while
> removing this branch.
>
>>
>> > + return true;
>> > + }
>> > + }
>>
>> What about this version:
>>
>> [1] if (!*pptr) {
>
> This one I agree.
>
>> int byte = qemu_get_byte(f);
>>
>> if (byte == VMS_MARKER_PTR_NULL) {
>> /* When it's a null ptr marker, do not continue the load */
>> [3] goto out;
>> }
>>
>> [2] if (auto_alloc && byte == VMS_MARKER_PTR_VALID) {
>> /*
>> * When reaching here, it means we received a non-NULL ptr
>> * marker, so we need to populate the field before loading it.
>> *
>> * NOTE: do not use vmstate_size() here, because we need the
>> * object size, not entry size of the array.
>> */
>> *pptr = g_malloc0(field->size);
>> } else {
>> error_setg(errp, "Unexpected ptr marker: %d", byte);
>
> We'll need to attach the trace_*() to not lose capturing all errors in
> tracepoints.
>
>> return false;
>> }
>> }
>
> Let's do apples-to-apples compare, removing comments.
>
> This is the current version after I address 1+3:
>
> if (!*pptr) {
> if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> trace_vmstate_load_field_error(field->name, -EINVAL);
> return false;
> }
> if (load_field) {
> assert(auto_alloc);
> *pptr = g_malloc0(field->size);
> }
> }
>
Yep, could be.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-26 21:05 ` [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
2026-03-27 13:12 ` Fabiano Rosas
@ 2026-04-01 13:28 ` Alexander Mikhalitsyn
1 sibling, 0 replies; 32+ messages in thread
From: Alexander Mikhalitsyn @ 2026-04-01 13:28 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin
Am Do., 26. März 2026 um 22:05 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> Introduce a new flag, VMS_ARRAY_OF_POINTER_AUTO_ALLOC, for VMSD field. It
> must be used together with VMS_ARRAY_OF_POINTER.
>
> It can be used to allow migration of an array of pointers where the
> pointers may point to NULLs.
>
> Note that we used to allow migration of a NULL pointer within an array that
> is being migrated. That corresponds to the code around vmstate_info_nullptr
> where we may get/put one byte showing that the element of an array is NULL.
>
> That usage is fine but very limited, it's because even if it will migrate a
> NULL pointer with a marker, it still works in a way that both src and dest
> QEMUs must know exactly which elements of the array are non-NULL, so
> instead of dynamically loading an array (which can have NULL pointers), it
> actually only verifies the known NULL pointers are still NULL pointers
> after migration.
>
> Also, in that case since dest QEMU knows exactly which element is NULL,
> which is not NULL, dest QEMU's device code will manage all allocations for
> the elements before invoking vmstate_load_vmsd().
>
> That's not enough per evolving needs of new device states that may want to
> provide real dynamic array of pointers, like what Alexander proposed here
> with the NVMe device migration:
>
> https://lore.kernel.org/r/20260317102708.126725-1-alexander@mihalicyn.com
>
> This patch is an alternative approach to address the problem.
>
> Along with the flag, introduce two new macros:
>
> VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT{8|32}_ALLOC()
>
> Which will be used very soon in the NVMe series.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Thanks, Peter!
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
also, I have rebased my patches
(https://github.com/mihalicyn/qemu/commits/nvme-live-migration/)
and retested everything, so:
Tested-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Kind regards,
Alex
> ---
> include/migration/vmstate.h | 51 +++++++++++++-
> migration/savevm.c | 27 ++++++-
> migration/vmstate.c | 136 ++++++++++++++++++++++++++++++------
> 3 files changed, 190 insertions(+), 24 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2e51b5ea04..d844b46e63 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -161,8 +161,21 @@ enum VMStateFlags {
> * structure we are referencing to use. */
> VMS_VSTRUCT = 0x8000,
>
> + /*
> + * This is a sub-flag for VMS_ARRAY_OF_POINTER. When this flag is set,
> + * VMS_ARRAY_OF_POINTER must also be set. When set, it means array
> + * elements can contain either valid or NULL pointers, vmstate core
> + * will be responsible for synchronizing the pointer status, providing
> + * proper memory allocations on the pointer when it is populated on the
> + * source QEMU. It also means the user of the field must make sure all
> + * the elements in the array are NULL pointers before loading. This
> + * should also work with VMS_ALLOC when the array itself also needs to
> + * be allocated.
> + */
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC = 0x10000,
> +
> /* Marker for end of list */
> - VMS_END = 0x10000
> + VMS_END = 0x20000,
> };
>
> typedef enum {
> @@ -580,6 +593,42 @@ extern const VMStateInfo vmstate_info_qlist;
> .offset = vmstate_offset_array(_s, _f, _type*, _n), \
> }
>
> +/*
> + * For migrating a dynamically allocated uint{8,32}-indexed array of
> + * pointers to structures (with NULL entries and with auto memory
> + * allocation).
> + *
> + * _type: type of structure pointed to
> + * _vmsd: VMSD for structure _type (when VMS_STRUCT is set)
> + * _info: VMStateInfo for _type (when VMS_STRUCT is not set)
> + * start: size of (_type) pointed to (for auto memory allocation)
> + */
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT8_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT8 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> +#define VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(\
> + _field, _state, _field_num, _version, _vmsd, _type) { \
> + .name = (stringify(_field)), \
> + .version_id = (_version), \
> + .num_offset = vmstate_offset_value(_state, _field_num, uint32_t), \
> + .vmsd = &(_vmsd), \
> + .size = sizeof(_type), \
> + .flags = VMS_POINTER | VMS_VARRAY_UINT32 | \
> + VMS_ARRAY_OF_POINTER | VMS_STRUCT | \
> + VMS_ARRAY_OF_POINTER_AUTO_ALLOC, \
> + .offset = vmstate_offset_pointer(_state, _field, _type *), \
> +}
> +
> #define VMSTATE_VARRAY_OF_POINTER_UINT32(_field, _state, _field_num, _version, _info, _type) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5a6fd0c66..765df8ce2d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -869,8 +869,33 @@ static void vmstate_check(const VMStateDescription *vmsd)
> if (field) {
> while (field->name) {
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> - assert(field->size == 0);
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * Size must be provided because dest QEMU needs that
> + * info to know what to allocate
> + */
> + assert(field->size || field->size_offset);
> + } else {
> + /*
> + * Otherwise size info isn't useful (because it's
> + * always the size of host pointer), detect accidental
> + * setup of sizes in this case.
> + */
> + assert(field->size == 0 && field->size_offset == 0);
> + }
> + /*
> + * VMS_ARRAY_OF_POINTER must be used only together with one
> + * of VMS_(V)ARRAY* flags.
> + */
> + assert(field->flags & (VMS_ARRAY | VMS_VARRAY_INT32 |
> + VMS_VARRAY_UINT16 | VMS_VARRAY_UINT8 |
> + VMS_VARRAY_UINT32));
> }
> +
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + assert(field->flags & VMS_ARRAY_OF_POINTER);
> + }
> +
> if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) {
> /* Recurse to sub structures */
> vmstate_check(field->vmsd);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 47812eb882..9cd0a88ce9 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,6 +153,12 @@ static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> return true;
> }
>
> + if (byte == VMS_MARKER_PTR_VALID) {
> + /* We need to load the field right after the marker */
> + *load_field = true;
> + return true;
> + }
> +
> error_setg(errp, "Unexpected ptr marker: %d", byte);
> return false;
> }
> @@ -234,6 +240,67 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
> return true;
> }
>
> +/*
> + * Try to prepare loading the next element, the object pointer to be put
> + * into @next_elem. When @next_elem is NULL, it means we should skip
> + * loading this element.
> + *
> + * Returns false for errors, in which case *errp will be set, migration
> + * must be aborted.
> + */
> +static bool vmstate_load_next(QEMUFile *f, const VMStateField *field,
> + void *first_elem, void **next_elem,
> + int size, int i, Error **errp)
> +{
> + bool auto_alloc = field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> + void *ptr = first_elem + size * i, **pptr;
> + bool load_field;
> +
> + if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
> + /* Simplest case, no pointer involved */
> + *next_elem = ptr;
> + return true;
> + }
> +
> + /*
> + * We're loading an array of pointers, switch to use pptr to make it
> + * easier to read later
> + */
> + pptr = (void **)ptr;
> +
> + /*
> + * Some special cases use pointer markers: (1) _AUTO_ALLOC implies a
> + * ptr marker will always exist, or (2) the element on destination is
> + * NULL, which expects the src to send a NULL-only marker.
> + */
> + if (auto_alloc || !*pptr) {
> + if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> + trace_vmstate_load_field_error(field->name, -EINVAL);
> + return false;
> + }
> +
> + if (load_field) {
> + /*
> + * When reaching here, it means we received a non-NULL ptr
> + * marker, so we need to populate the field before loading it.
> + *
> + * NOTE: do not use vmstate_size() here, because we need the
> + * object size, not entry size of the array.
> + */
> + assert(auto_alloc);
> + *pptr = g_malloc0(field->size);
> + } else {
> + /* Clear the pointer to imply a skip */
> + *next_elem = NULL;
> + return true;
> + }
> + }
> +
> + /* Move the cursor to the next element for loading */
> + *next_elem = *pptr;
> + return true;
> +}
> +
> bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp)
> {
> @@ -279,27 +346,22 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> for (i = 0; i < n_elems; i++) {
> - /* If we will process the load of field? */
> - bool load_field = true;
> - bool ok = true;
> - void *curr_elem = first_elem + size * i;
> + void *curr_elem;
> + bool ok;
>
> - if (field->flags & VMS_ARRAY_OF_POINTER) {
> - curr_elem = *(void **)curr_elem;
> - if (!curr_elem) {
> - /* Read the marker instead of VMSD itself */
> - if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> - trace_vmstate_load_field_error(field->name,
> - -EINVAL);
> - return false;
> - }
> - }
> + ok = vmstate_load_next(f, field, first_elem, &curr_elem,
> + size, i, errp);
> + if (!ok) {
> + return false;
> }
>
> - if (load_field) {
> - ok = vmstate_load_field(f, curr_elem, size, field, errp);
> + if (!curr_elem) {
> + /* Implies a skip */
> + continue;
> }
>
> + ok = vmstate_load_field(f, curr_elem, size, field, errp);
> +
> if (ok) {
> int ret = qemu_file_get_error(f);
> if (ret < 0) {
> @@ -397,6 +459,16 @@ static bool vmsd_can_compress(const VMStateField *field)
> return false;
> }
>
> + if (field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> + /*
> + * This may involve two VMSD fields to be saved, one for the
> + * marker to show if the pointer is NULL, followed by the real
> + * vmstate object. To make it simple at least for now, skip
> + * compression for this one.
> + */
> + return false;
> + }
> +
> if (field->flags & VMS_STRUCT) {
> const VMStateField *sfield = field->vmsd->fields;
> while (sfield->name) {
> @@ -583,6 +655,12 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> bool is_prev_null = false;
> + /*
> + * When this is enabled, it means we will always push a ptr
> + * marker first for each element saying if it's populated.
> + */
> + bool use_dynamic_array =
> + field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -603,14 +681,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> is_null = !curr_elem && size;
> - use_marker_field = is_null;
> + use_marker_field = use_dynamic_array || is_null;
>
> if (use_marker_field) {
> - /*
> - * If null pointer found (which should only happen in
> - * an array of pointers), use null placeholder and do
> - * not follow.
> - */
> inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> @@ -657,6 +730,25 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> goto out;
> }
>
> + /*
> + * If we're using dynamic array and the element is
> + * populated, save the real object right after the marker.
> + */
> + if (use_dynamic_array && curr_elem) {
> + /*
> + * NOTE: do not use vmstate_size() here because we want
> + * to save the real VMSD object now.
> + */
> + ok = vmstate_save_field_with_vmdesc(f, curr_elem,
> + field->size, vmsd,
> + field, vmdesc_loop,
> + i, max_elems, errp);
> +
> + if (!ok) {
> + goto out;
> + }
> + }
> +
> /* Compressed arrays only care about the first element */
> if (vmdesc_loop && max_elems > 1) {
> vmdesc_loop = NULL;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 10/11] vmstate: Stop checking size for nullptr compression
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (8 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 09/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
2026-04-01 13:30 ` Alexander Mikhalitsyn
2026-03-26 21:05 ` [PATCH RFC v2 11/11] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
10 siblings, 1 reply; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin
From: Fabiano Rosas <farosas@suse.de>
The NULL pointer marker code applies only to VMS_ARRAY_OF_POINTER,
where the size is never NULL. Move the setting of is_null under
VMS_ARRAY_OF_POINTER, so we can stop checking the size.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/vmstate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 9cd0a88ce9..1923602f3b 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -673,14 +673,14 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
const VMStateField *inner_field;
/* maximum number of elements to compress in the JSON blob */
int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
- bool use_marker_field, is_null;
+ bool use_marker_field, is_null = false;
if (field->flags & VMS_ARRAY_OF_POINTER) {
assert(curr_elem);
curr_elem = *(void **)curr_elem;
+ is_null = !curr_elem;
}
- is_null = !curr_elem && size;
use_marker_field = use_dynamic_array || is_null;
if (use_marker_field) {
@@ -708,7 +708,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
for (int j = i + 1; j < n_elems; j++) {
void *elem = *(void **)(first_elem + size * j);
- bool elem_is_null = !elem && size;
+ bool elem_is_null = !elem;
if (is_null != elem_is_null) {
max_elems = j - i;
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH RFC v2 10/11] vmstate: Stop checking size for nullptr compression
2026-03-26 21:05 ` [PATCH RFC v2 10/11] vmstate: Stop checking size for nullptr compression Peter Xu
@ 2026-04-01 13:30 ` Alexander Mikhalitsyn
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Mikhalitsyn @ 2026-04-01 13:30 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Fabiano Rosas, Juraj Marcin
Am Do., 26. März 2026 um 22:05 Uhr schrieb Peter Xu <peterx@redhat.com>:
>
> From: Fabiano Rosas <farosas@suse.de>
>
> The NULL pointer marker code applies only to VMS_ARRAY_OF_POINTER,
> where the size is never NULL. Move the setting of is_null under
> VMS_ARRAY_OF_POINTER, so we can stop checking the size.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> migration/vmstate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 9cd0a88ce9..1923602f3b 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -673,14 +673,14 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
> const VMStateField *inner_field;
> /* maximum number of elements to compress in the JSON blob */
> int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
> - bool use_marker_field, is_null;
> + bool use_marker_field, is_null = false;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> assert(curr_elem);
> curr_elem = *(void **)curr_elem;
> + is_null = !curr_elem;
> }
>
> - is_null = !curr_elem && size;
> use_marker_field = use_dynamic_array || is_null;
>
> if (use_marker_field) {
> @@ -708,7 +708,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>
> for (int j = i + 1; j < n_elems; j++) {
> void *elem = *(void **)(first_elem + size * j);
> - bool elem_is_null = !elem && size;
> + bool elem_is_null = !elem;
>
> if (is_null != elem_is_null) {
> max_elems = j - i;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC v2 11/11] tests/unit/test-vmstate: add tests for VMS_ARRAY_OF_POINTER_AUTO_ALLOC
2026-03-26 21:05 [PATCH RFC v2 00/11] vmstate: Implement VMS_ARRAY_OF_POINTER_AUTO_ALLOC Peter Xu
` (9 preceding siblings ...)
2026-03-26 21:05 ` [PATCH RFC v2 10/11] vmstate: Stop checking size for nullptr compression Peter Xu
@ 2026-03-26 21:05 ` Peter Xu
10 siblings, 0 replies; 32+ messages in thread
From: Peter Xu @ 2026-03-26 21:05 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Mikhalitsyn, peterx, Fabiano Rosas, Juraj Marcin,
Alexander Mikhalitsyn
From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Add tests for VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC.
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
[peterx: Removed two tests due to macro not used, rebase, fix warning]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/unit/test-vmstate.c | 86 +++++++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index dae15786aa..df1fb4c778 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -702,6 +702,88 @@ static void test_arr_ptr_prim_0_load(void)
}
}
+static uint8_t wire_arr_ptr_with_nulls[] = {
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x00,
+ VMS_MARKER_PTR_NULL,
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x02,
+ VMS_MARKER_PTR_VALID,
+ 0x00, 0x00, 0x00, 0x03,
+ QEMU_VM_EOF
+};
+
+typedef struct {
+ uint32_t ar_items_num;
+ TestStructTriv **ar;
+} TestVArrayOfPtrToStuctWithNULLs;
+
+const VMStateDescription vmsd_arps_with_nulls = {
+ .name = "test/arps_with_nulls",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (const VMStateField[]) {
+ VMSTATE_VARRAY_OF_POINTER_TO_STRUCT_UINT32_ALLOC(
+ ar, TestVArrayOfPtrToStuctWithNULLs, ar_items_num,
+ 0, vmsd_tst, TestStructTriv),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void test_arr_ptr_nulls_str_save(void)
+{
+ TestStructTriv ar[AR_SIZE] = { {.i = 0}, {.i = 1}, {.i = 2}, {.i = 3} };
+ TestVArrayOfPtrToStuctWithNULLs sample = {};
+ int idx;
+
+ sample.ar_items_num = AR_SIZE;
+ sample.ar = g_new0(TestStructTriv*, sample.ar_items_num);
+ sample.ar[0] = g_new0(TestStructTriv, 1);
+ *sample.ar[0] = ar[0];
+ /* note, sample.ar[1] remains NULL */
+ sample.ar[2] = g_new0(TestStructTriv, 1);
+ *sample.ar[2] = ar[2];
+ sample.ar[3] = g_new0(TestStructTriv, 1);
+ *sample.ar[3] = ar[3];
+
+ save_vmstate(&vmsd_arps_with_nulls, &sample);
+ compare_vmstate(wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls));
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ g_free(sample.ar[idx]);
+ }
+ g_free(sample.ar);
+}
+
+static void test_arr_ptr_nulls_str_load(void)
+{
+ TestStructTriv ar_gt[AR_SIZE] = {{.i = 0}, {.i = 0}, {.i = 2}, {.i = 3} };
+ TestVArrayOfPtrToStuctWithNULLs obj = {};
+ int idx;
+
+ obj.ar_items_num = AR_SIZE;
+ obj.ar = g_new0(TestStructTriv*, obj.ar_items_num);
+
+ save_buffer(wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls));
+ SUCCESS(load_vmstate_one(
+ &vmsd_arps_with_nulls, &obj, 1,
+ wire_arr_ptr_with_nulls, sizeof(wire_arr_ptr_with_nulls)));
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ if (idx == 1) {
+ g_assert_cmpint((uintptr_t)(obj.ar[idx]), ==, 0);
+ } else {
+ /* compare the target array ar with the ground truth array ar_gt */
+ g_assert_cmpint(ar_gt[idx].i, ==, obj.ar[idx]->i);
+ }
+ }
+
+ for (idx = 0; idx < AR_SIZE; ++idx) {
+ g_free(obj.ar[idx]);
+ }
+ g_free(obj.ar);
+}
+
/* test QTAILQ migration */
typedef struct TestQtailqElement TestQtailqElement;
@@ -1568,6 +1650,10 @@ int main(int argc, char **argv)
test_arr_ptr_prim_0_save);
g_test_add_func("/vmstate/array/ptr/prim/0/load",
test_arr_ptr_prim_0_load);
+ g_test_add_func("/vmstate/array/ptr-nulls/str/save",
+ test_arr_ptr_nulls_str_save);
+ g_test_add_func("/vmstate/array/ptr-nulls/str/load",
+ test_arr_ptr_nulls_str_load);
g_test_add_func("/vmstate/qtailq/save/saveq", test_save_q);
g_test_add_func("/vmstate/qtailq/load/loadq", test_load_q);
g_test_add_func("/vmstate/gtree/save/savedomain", test_gtree_save_domain);
--
2.50.1
^ permalink raw reply related [flat|nested] 32+ messages in thread