* [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
@ 2026-02-20 21:01 ` Vladimir Sementsov-Ogievskiy
2026-02-27 22:14 ` Fabiano Rosas
2026-03-03 14:30 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 02/16] migration: make vmstate_save_state_v() static Vladimir Sementsov-Ogievskiy
` (15 subsequent siblings)
16 siblings, 2 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:01 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
We may call error_setg twice on same errp if inner
vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
crash on assertion in error_setv().
Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 4d28364f7b..fccd030dfd 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -539,6 +539,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
} else {
ret = inner_field->info->put(f, curr_elem, size,
inner_field, vmdesc_loop);
+ if (ret < 0) {
+ error_setg(errp, "put failed");
+ }
}
written_bytes = qemu_file_transferred(f) - old_offset;
@@ -551,8 +554,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
}
if (ret) {
- error_setg(errp, "Save of field %s/%s failed",
- vmsd->name, field->name);
+ error_prepend(errp, "Save of field %s/%s failed: ",
+ vmsd->name, field->name);
if (vmsd->post_save) {
vmsd->post_save(opaque);
}
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg
2026-02-20 21:01 ` [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg Vladimir Sementsov-Ogievskiy
@ 2026-02-27 22:14 ` Fabiano Rosas
2026-03-03 14:30 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Fabiano Rosas @ 2026-02-27 22:14 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, peterx
Cc: vsementsov, open list:All patches CC here
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> We may call error_setg twice on same errp if inner
> vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
> crash on assertion in error_setv().
>
> Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> migration/vmstate.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 4d28364f7b..fccd030dfd 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -539,6 +539,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> } else {
> ret = inner_field->info->put(f, curr_elem, size,
> inner_field, vmdesc_loop);
> + if (ret < 0) {
> + error_setg(errp, "put failed");
> + }
> }
>
> written_bytes = qemu_file_transferred(f) - old_offset;
> @@ -551,8 +554,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> }
>
> if (ret) {
> - error_setg(errp, "Save of field %s/%s failed",
> - vmsd->name, field->name);
> + error_prepend(errp, "Save of field %s/%s failed: ",
> + vmsd->name, field->name);
> if (vmsd->post_save) {
> vmsd->post_save(opaque);
> }
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg
2026-02-20 21:01 ` [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg Vladimir Sementsov-Ogievskiy
2026-02-27 22:14 ` Fabiano Rosas
@ 2026-03-03 14:30 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 14:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:01:59AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We may call error_setg twice on same errp if inner
> vmstate_save_state_v() or vmstate_save_state() call fails. Next we will
> crash on assertion in error_setv().
>
> Fixes: 848a0503422d043 "migration: Update error description outside migration.c"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 02/16] migration: make vmstate_save_state_v() static
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
2026-02-20 21:01 ` [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-02-27 22:15 ` Fabiano Rosas
2026-03-03 15:01 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 03/16] migration: make .post_save() a void function Vladimir Sementsov-Ogievskiy
` (14 subsequent siblings)
16 siblings, 2 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
It's used only in vmstate.c.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/migration/vmstate.h | 3 ---
migration/vmstate.c | 19 ++++++++++---------
2 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 89f9f49d20..3695afd483 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1234,9 +1234,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp);
int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc, Error **errp);
-int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc,
- int version_id, Error **errp);
bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index fccd030dfd..651c3fe011 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -420,15 +420,9 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
return true;
}
-
-int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc_id, Error **errp)
-{
- return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id, errp);
-}
-
-int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
+static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc,
+ int version_id, Error **errp)
{
ERRP_GUARD();
int ret = 0;
@@ -608,6 +602,13 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
return NULL;
}
+int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc_id, Error **errp)
+{
+ return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id,
+ errp);
+}
+
static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, Error **errp)
{
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 02/16] migration: make vmstate_save_state_v() static
2026-02-20 21:02 ` [PATCH v2 02/16] migration: make vmstate_save_state_v() static Vladimir Sementsov-Ogievskiy
@ 2026-02-27 22:15 ` Fabiano Rosas
2026-03-03 15:01 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Fabiano Rosas @ 2026-02-27 22:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, peterx
Cc: vsementsov, open list:All patches CC here
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> It's used only in vmstate.c.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> include/migration/vmstate.h | 3 ---
> migration/vmstate.c | 19 ++++++++++---------
> 2 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 89f9f49d20..3695afd483 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1234,9 +1234,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, int version_id, Error **errp);
> int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc, Error **errp);
> -int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, JSONWriter *vmdesc,
> - int version_id, Error **errp);
>
> bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque);
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fccd030dfd..651c3fe011 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -420,15 +420,9 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
> return true;
> }
>
> -
> -int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, JSONWriter *vmdesc_id, Error **errp)
> -{
> - return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id, errp);
> -}
> -
> -int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
> +static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> + void *opaque, JSONWriter *vmdesc,
> + int version_id, Error **errp)
> {
> ERRP_GUARD();
> int ret = 0;
> @@ -608,6 +602,13 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> return NULL;
> }
>
> +int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> + void *opaque, JSONWriter *vmdesc_id, Error **errp)
> +{
> + return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id,
> + errp);
> +}
> +
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, Error **errp)
> {
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 02/16] migration: make vmstate_save_state_v() static
2026-02-20 21:02 ` [PATCH v2 02/16] migration: make vmstate_save_state_v() static Vladimir Sementsov-Ogievskiy
2026-02-27 22:15 ` Fabiano Rosas
@ 2026-03-03 15:01 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:01 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> It's used only in vmstate.c.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 03/16] migration: make .post_save() a void function
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
2026-02-20 21:01 ` [PATCH v2 01/16] migration: vmstate_save_state_v: fix double error_setg Vladimir Sementsov-Ogievskiy
2026-02-20 21:02 ` [PATCH v2 02/16] migration: make vmstate_save_state_v() static Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-02-27 22:41 ` Fabiano Rosas
2026-02-20 21:02 ` [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines Vladimir Sementsov-Ogievskiy
` (13 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx
Cc: farosas, vsementsov, Pierrick Bouvier, Nicholas Piggin,
Harsh Prateek Bora, Peter Maydell, open list:All patches CC here,
open list:sPAPR (pseries), open list:ARM TCG CPUs
All other handlers now have _errp() variants. Should we go this way
for .post_save()? Actually it's rather strange, when the vmstate do
successful preparations in .pre_save(), then successfully save all
sections and subsections, end then fail when all the state is
successfully transferred to the target.
Happily, we have only three .post_save() realizations, all always
successful. Let's make this a rule.
Also note, that we call .post_save() in two places, and handle
its (theoretical) failure inconsistently. Fix that too.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
docs/devel/migration/main.rst | 2 +-
hw/ppc/spapr_pci.c | 3 +--
include/migration/vmstate.h | 10 +++++++++-
migration/savevm.c | 3 +--
migration/vmstate.c | 12 +++---------
target/arm/machine.c | 4 +---
6 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 234d280249..2de7050764 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -439,7 +439,7 @@ The functions to do that are inside a vmstate definition, and are called:
This function is called before we save the state of one device.
-- ``int (*post_save)(void *opaque);``
+- ``void (*post_save)(void *opaque);``
This function is called after we save the state of one device
(even upon failure, unless the call to pre_save returned an error).
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ea998bdff1..1dc3b02659 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2093,14 +2093,13 @@ static int spapr_pci_pre_save(void *opaque)
return 0;
}
-static int spapr_pci_post_save(void *opaque)
+static void spapr_pci_post_save(void *opaque)
{
SpaprPhbState *sphb = opaque;
g_free(sphb->msi_devs);
sphb->msi_devs = NULL;
sphb->msi_devs_num = 0;
- return 0;
}
static int spapr_pci_post_load(void *opaque, int version_id)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 3695afd483..15578b3e28 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -223,7 +223,15 @@ struct VMStateDescription {
bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
int (*pre_save)(void *opaque);
bool (*pre_save_errp)(void *opaque, Error **errp);
- int (*post_save)(void *opaque);
+
+ /*
+ * post_save() rarely used to free some temporary resources.
+ * It's is called if .pre_save[_errp]() call was successful
+ * (or .pre_save[_errp] handler absent), regardless success
+ * or failure during fields and subsections save. If
+ * .pre_save[_errp]() fails, .post_save() is not called.
+ */
+ void (*post_save)(void *opaque);
bool (*needed)(void *opaque);
bool (*dev_unplug_pending)(void *opaque);
diff --git a/migration/savevm.c b/migration/savevm.c
index 3a16c467b2..c5236e71ba 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -321,14 +321,13 @@ static int configuration_pre_save(void *opaque)
return 0;
}
-static int configuration_post_save(void *opaque)
+static void configuration_post_save(void *opaque)
{
SaveState *state = opaque;
g_free(state->capabilities);
state->capabilities = NULL;
state->caps_count = 0;
- return 0;
}
static int configuration_pre_load(void *opaque)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 651c3fe011..5111e7a141 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -550,10 +550,7 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
if (ret) {
error_prepend(errp, "Save of field %s/%s failed: ",
vmsd->name, field->name);
- if (vmsd->post_save) {
- vmsd->post_save(opaque);
- }
- return ret;
+ goto out;
}
/* Compressed arrays only care about the first element */
@@ -578,12 +575,9 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
+out:
if (vmsd->post_save) {
- int ps_ret = vmsd->post_save(opaque);
- if (!ret && ps_ret) {
- ret = ps_ret;
- error_setg(errp, "post-save failed: %s", vmsd->name);
- }
+ vmsd->post_save(opaque);
}
return ret;
}
diff --git a/target/arm/machine.c b/target/arm/machine.c
index bbaae34449..de810220e2 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -993,15 +993,13 @@ static int cpu_pre_save(void *opaque)
return 0;
}
-static int cpu_post_save(void *opaque)
+static void cpu_post_save(void *opaque)
{
ARMCPU *cpu = opaque;
if (!kvm_enabled()) {
pmu_op_finish(&cpu->env);
}
-
- return 0;
}
static int cpu_pre_load(void *opaque)
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 03/16] migration: make .post_save() a void function
2026-02-20 21:02 ` [PATCH v2 03/16] migration: make .post_save() a void function Vladimir Sementsov-Ogievskiy
@ 2026-02-27 22:41 ` Fabiano Rosas
2026-02-27 23:56 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Fabiano Rosas @ 2026-02-27 22:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, peterx
Cc: vsementsov, Pierrick Bouvier, Nicholas Piggin, Harsh Prateek Bora,
Peter Maydell, open list:All patches CC here,
open list:sPAPR (pseries), open list:ARM TCG CPUs
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> All other handlers now have _errp() variants. Should we go this way
> for .post_save()? Actually it's rather strange, when the vmstate do
> successful preparations in .pre_save(), then successfully save all
> sections and subsections, end then fail when all the state is
> successfully transferred to the target.
>
> Happily, we have only three .post_save() realizations, all always
> successful. Let's make this a rule.
>
> Also note, that we call .post_save() in two places, and handle
> its (theoretical) failure inconsistently. Fix that too.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> docs/devel/migration/main.rst | 2 +-
> hw/ppc/spapr_pci.c | 3 +--
> include/migration/vmstate.h | 10 +++++++++-
> migration/savevm.c | 3 +--
> migration/vmstate.c | 12 +++---------
> target/arm/machine.c | 4 +---
> 6 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> index 234d280249..2de7050764 100644
> --- a/docs/devel/migration/main.rst
> +++ b/docs/devel/migration/main.rst
> @@ -439,7 +439,7 @@ The functions to do that are inside a vmstate definition, and are called:
>
> This function is called before we save the state of one device.
>
> -- ``int (*post_save)(void *opaque);``
> +- ``void (*post_save)(void *opaque);``
>
> This function is called after we save the state of one device
> (even upon failure, unless the call to pre_save returned an error).
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index ea998bdff1..1dc3b02659 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2093,14 +2093,13 @@ static int spapr_pci_pre_save(void *opaque)
> return 0;
> }
>
> -static int spapr_pci_post_save(void *opaque)
> +static void spapr_pci_post_save(void *opaque)
> {
> SpaprPhbState *sphb = opaque;
>
> g_free(sphb->msi_devs);
> sphb->msi_devs = NULL;
> sphb->msi_devs_num = 0;
> - return 0;
> }
>
> static int spapr_pci_post_load(void *opaque, int version_id)
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 3695afd483..15578b3e28 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -223,7 +223,15 @@ struct VMStateDescription {
> bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
> int (*pre_save)(void *opaque);
> bool (*pre_save_errp)(void *opaque, Error **errp);
> - int (*post_save)(void *opaque);
> +
> + /*
> + * post_save() rarely used to free some temporary resources.
> + * It's is called if .pre_save[_errp]() call was successful
> + * (or .pre_save[_errp] handler absent), regardless success
> + * or failure during fields and subsections save. If
> + * .pre_save[_errp]() fails, .post_save() is not called.
> + */
I would not mention usage directly and maybe also simplify the language
a bit. If there's doubt on the exact flow, people can read the code:
/*
* Unless .pre_save() fails, post_save() is called after saving
* fields and subsections. It should not fail because at this point
* the state has potentially already been transferred.
*/
> + void (*post_save)(void *opaque);
> bool (*needed)(void *opaque);
> bool (*dev_unplug_pending)(void *opaque);
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 3a16c467b2..c5236e71ba 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -321,14 +321,13 @@ static int configuration_pre_save(void *opaque)
> return 0;
> }
>
> -static int configuration_post_save(void *opaque)
> +static void configuration_post_save(void *opaque)
> {
> SaveState *state = opaque;
>
> g_free(state->capabilities);
> state->capabilities = NULL;
> state->caps_count = 0;
> - return 0;
> }
>
> static int configuration_pre_load(void *opaque)
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 651c3fe011..5111e7a141 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -550,10 +550,7 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> if (ret) {
> error_prepend(errp, "Save of field %s/%s failed: ",
> vmsd->name, field->name);
> - if (vmsd->post_save) {
> - vmsd->post_save(opaque);
> - }
> - return ret;
> + goto out;
> }
>
> /* Compressed arrays only care about the first element */
> @@ -578,12 +575,9 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>
> ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>
> +out:
> if (vmsd->post_save) {
> - int ps_ret = vmsd->post_save(opaque);
> - if (!ret && ps_ret) {
> - ret = ps_ret;
> - error_setg(errp, "post-save failed: %s", vmsd->name);
> - }
> + vmsd->post_save(opaque);
Nice cleanup.
> }
> return ret;
> }
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index bbaae34449..de810220e2 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -993,15 +993,13 @@ static int cpu_pre_save(void *opaque)
> return 0;
> }
>
> -static int cpu_post_save(void *opaque)
> +static void cpu_post_save(void *opaque)
> {
> ARMCPU *cpu = opaque;
>
> if (!kvm_enabled()) {
> pmu_op_finish(&cpu->env);
> }
> -
> - return 0;
> }
>
> static int cpu_pre_load(void *opaque)
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 03/16] migration: make .post_save() a void function
2026-02-27 22:41 ` Fabiano Rosas
@ 2026-02-27 23:56 ` Vladimir Sementsov-Ogievskiy
2026-03-03 15:05 ` Peter Xu
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-27 23:56 UTC (permalink / raw)
To: Fabiano Rosas, peterx
Cc: Pierrick Bouvier, Nicholas Piggin, Harsh Prateek Bora,
Peter Maydell, open list:All patches CC here,
open list:sPAPR (pseries), open list:ARM TCG CPUs
On 28.02.26 01:41, Fabiano Rosas wrote:
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 3695afd483..15578b3e28 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -223,7 +223,15 @@ struct VMStateDescription {
>> bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
>> int (*pre_save)(void *opaque);
>> bool (*pre_save_errp)(void *opaque, Error **errp);
>> - int (*post_save)(void *opaque);
>> +
>> + /*
>> + * post_save() rarely used to free some temporary resources.
>> + * It's is called if .pre_save[_errp]() call was successful
>> + * (or .pre_save[_errp] handler absent), regardless success
>> + * or failure during fields and subsections save. If
>> + * .pre_save[_errp]() fails, .post_save() is not called.
>> + */
> I would not mention usage directly and maybe also simplify the language
> a bit. If there's doubt on the exact flow, people can read the code:
>
> /*
> * Unless .pre_save() fails, post_save() is called after saving
> * fields and subsections. It should not fail because at this point
> * the state has potentially already been transferred.
> */
Agree, sounds good.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 03/16] migration: make .post_save() a void function
2026-02-27 23:56 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-03 15:05 ` Peter Xu
0 siblings, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:05 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Fabiano Rosas, Pierrick Bouvier, Nicholas Piggin,
Harsh Prateek Bora, Peter Maydell, open list:All patches CC here,
open list:sPAPR (pseries), open list:ARM TCG CPUs
On Sat, Feb 28, 2026 at 02:56:30AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 28.02.26 01:41, Fabiano Rosas wrote:
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index 3695afd483..15578b3e28 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -223,7 +223,15 @@ struct VMStateDescription {
> > > bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
> > > int (*pre_save)(void *opaque);
> > > bool (*pre_save_errp)(void *opaque, Error **errp);
> > > - int (*post_save)(void *opaque);
> > > +
> > > + /*
> > > + * post_save() rarely used to free some temporary resources.
> > > + * It's is called if .pre_save[_errp]() call was successful
> > > + * (or .pre_save[_errp] handler absent), regardless success
> > > + * or failure during fields and subsections save. If
> > > + * .pre_save[_errp]() fails, .post_save() is not called.
> > > + */
> > I would not mention usage directly and maybe also simplify the language
> > a bit. If there's doubt on the exact flow, people can read the code:
> >
> > /*
> > * Unless .pre_save() fails, post_save() is called after saving
> > * fields and subsections. It should not fail because at this point
> > * the state has potentially already been transferred.
> > */
>
>
> Agree, sounds good.
With the update, feel free to take:
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (2 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 03/16] migration: make .post_save() a void function Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-02-27 22:41 ` Fabiano Rosas
2026-03-03 15:06 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
16 siblings, 2 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Split logical blocks by newlines, that simplify reading the code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 5111e7a141..dd7cd27993 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -139,6 +139,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
int ret = 0;
trace_vmstate_load_state(vmsd->name, version_id);
+
if (version_id > vmsd->version_id) {
error_setg(errp, "%s: incoming version_id %d is too new "
"for local version_id %d",
@@ -146,6 +147,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
return -EINVAL;
}
+
if (version_id < vmsd->minimum_version_id) {
error_setg(errp, "%s: incoming version_id %d is too old "
"for local minimum version_id %d",
@@ -153,6 +155,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
return -EINVAL;
}
+
if (vmsd->pre_load_errp) {
if (!vmsd->pre_load_errp(opaque, errp)) {
error_prepend(errp, "pre load hook failed for: '%s', "
@@ -171,9 +174,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return ret;
}
}
+
while (field->name) {
bool exists = vmstate_field_exists(vmsd, field, opaque, version_id);
+
trace_vmstate_load_state_field(vmsd->name, field->name, exists);
+
if (exists) {
void *first_elem = opaque + field->offset;
int i, n_elems = vmstate_n_elems(opaque, field);
@@ -184,6 +190,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
first_elem = *(void **)first_elem;
assert(first_elem || !n_elems || !size);
}
+
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
@@ -235,6 +242,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
vmsd->name, ret);
}
}
+
if (ret < 0) {
qemu_file_set_error(f, ret);
trace_vmstate_load_field_error(field->name, ret);
@@ -249,11 +257,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
field++;
}
assert(field->flags == VMS_END);
+
ret = vmstate_subsection_load(f, vmsd, opaque, errp);
if (ret != 0) {
qemu_file_set_error(f, ret);
return ret;
}
+
if (vmsd->post_load_errp) {
if (!vmsd->post_load_errp(opaque, version_id, errp)) {
error_prepend(errp, "post load hook failed for: %s, version_id: "
@@ -271,7 +281,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
ret);
}
}
+
trace_vmstate_load_state_end(vmsd->name, "end", ret);
+
return ret;
}
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines
2026-02-20 21:02 ` [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines Vladimir Sementsov-Ogievskiy
@ 2026-02-27 22:41 ` Fabiano Rosas
2026-03-03 15:06 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Fabiano Rosas @ 2026-02-27 22:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, peterx
Cc: vsementsov, open list:All patches CC here
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> Split logical blocks by newlines, that simplify reading the code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> migration/vmstate.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5111e7a141..dd7cd27993 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -139,6 +139,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> int ret = 0;
>
> trace_vmstate_load_state(vmsd->name, version_id);
> +
> if (version_id > vmsd->version_id) {
> error_setg(errp, "%s: incoming version_id %d is too new "
> "for local version_id %d",
> @@ -146,6 +147,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> return -EINVAL;
> }
> +
> if (version_id < vmsd->minimum_version_id) {
> error_setg(errp, "%s: incoming version_id %d is too old "
> "for local minimum version_id %d",
> @@ -153,6 +155,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> return -EINVAL;
> }
> +
> if (vmsd->pre_load_errp) {
> if (!vmsd->pre_load_errp(opaque, errp)) {
> error_prepend(errp, "pre load hook failed for: '%s', "
> @@ -171,9 +174,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> return ret;
> }
> }
> +
> while (field->name) {
> bool exists = vmstate_field_exists(vmsd, field, opaque, version_id);
> +
> trace_vmstate_load_state_field(vmsd->name, field->name, exists);
> +
> if (exists) {
> void *first_elem = opaque + field->offset;
> int i, n_elems = vmstate_n_elems(opaque, field);
> @@ -184,6 +190,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> first_elem = *(void **)first_elem;
> assert(first_elem || !n_elems || !size);
> }
> +
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> @@ -235,6 +242,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> vmsd->name, ret);
> }
> }
> +
> if (ret < 0) {
> qemu_file_set_error(f, ret);
> trace_vmstate_load_field_error(field->name, ret);
> @@ -249,11 +257,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> field++;
> }
> assert(field->flags == VMS_END);
> +
> ret = vmstate_subsection_load(f, vmsd, opaque, errp);
> if (ret != 0) {
> qemu_file_set_error(f, ret);
> return ret;
> }
> +
> if (vmsd->post_load_errp) {
> if (!vmsd->post_load_errp(opaque, version_id, errp)) {
> error_prepend(errp, "post load hook failed for: %s, version_id: "
> @@ -271,7 +281,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> ret);
> }
> }
> +
> trace_vmstate_load_state_end(vmsd->name, "end", ret);
> +
> return ret;
> }
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines
2026-02-20 21:02 ` [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines Vladimir Sementsov-Ogievskiy
2026-02-27 22:41 ` Fabiano Rosas
@ 2026-03-03 15:06 ` Peter Xu
1 sibling, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:06 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:02AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Split logical blocks by newlines, that simplify reading the code.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (3 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 04/16] migration: vmstate_load_state(): add some newlines Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-02-27 22:44 ` Fabiano Rosas
2026-02-20 21:02 ` [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state() Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
We duplicate some of errors by trace points. That doesn't make
real sense: we report the error through errp, and stop migration,
no reason to duplicate the reported error by trace points, making
error path more complicated.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/trace-events | 5 ++---
migration/vmstate.c | 14 ++++++--------
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/migration/trace-events b/migration/trace-events
index 90629f828f..e864d19c55 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -55,15 +55,14 @@ postcopy_pause_incoming_continued(void) ""
postcopy_page_req_sync(void *host_addr) "sync page req %p"
# vmstate.c
-vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret = %d"
vmstate_load_state(const char *name, int version_id) "%s v%d"
-vmstate_load_state_end(const char *name, const char *reason, int val) "%s %s/%d"
+vmstate_load_state_end(const char *name) "%s"
vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s exists=%d"
vmstate_n_elems(const char *name, int n_elems) "%s: %d"
vmstate_subsection_load(const char *parent) "%s"
vmstate_subsection_load_bad(const char *parent, const char *sub, const char *sub2) "%s: %s/%s"
vmstate_subsection_load_good(const char *parent) "%s"
-vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d"
+vmstate_save_state_pre_save_done(const char *name) "%s"
vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s[%d]"
vmstate_save_state_top(const char *idstr) "%s"
vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
diff --git a/migration/vmstate.c b/migration/vmstate.c
index dd7cd27993..b07bbdd366 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -144,7 +144,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
error_setg(errp, "%s: incoming version_id %d is too new "
"for local version_id %d",
vmsd->name, version_id, vmsd->version_id);
- trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
return -EINVAL;
}
@@ -152,7 +151,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
error_setg(errp, "%s: incoming version_id %d is too old "
"for local minimum version_id %d",
vmsd->name, version_id, vmsd->minimum_version_id);
- trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
return -EINVAL;
}
@@ -245,7 +243,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (ret < 0) {
qemu_file_set_error(f, ret);
- trace_vmstate_load_field_error(field->name, ret);
return ret;
}
}
@@ -269,7 +266,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
error_prepend(errp, "post load hook failed for: %s, version_id: "
"%d, minimum_version: %d: ", vmsd->name,
vmsd->version_id, vmsd->minimum_version_id);
- ret = -EINVAL;
+ return -EINVAL;
}
} else if (vmsd->post_load) {
ret = vmsd->post_load(opaque, version_id);
@@ -279,12 +276,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
"minimum_version: %d, ret: %d",
vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
ret);
+ return ret;
}
}
- trace_vmstate_load_state_end(vmsd->name, "end", ret);
+ trace_vmstate_load_state_end(vmsd->name);
- return ret;
+ return 0;
}
static int vmfield_name_num(const VMStateField *start,
@@ -444,20 +442,20 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
if (vmsd->pre_save_errp) {
ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
- trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
if (ret < 0) {
error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
return ret;
}
} else if (vmsd->pre_save) {
ret = vmsd->pre_save(opaque);
- trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
if (ret) {
error_setg(errp, "pre-save failed: %s", vmsd->name);
return ret;
}
}
+ trace_vmstate_save_state_pre_save_done(vmsd->name);
+
if (vmdesc) {
json_writer_str(vmdesc, "vmsd_name", vmsd->name);
json_writer_int64(vmdesc, "version", version_id);
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors
2026-02-20 21:02 ` [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors Vladimir Sementsov-Ogievskiy
@ 2026-02-27 22:44 ` Fabiano Rosas
2026-03-03 15:13 ` Peter Xu
0 siblings, 1 reply; 50+ messages in thread
From: Fabiano Rosas @ 2026-02-27 22:44 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, peterx
Cc: vsementsov, open list:All patches CC here
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> We duplicate some of errors by trace points. That doesn't make
> real sense: we report the error through errp, and stop migration,
> no reason to duplicate the reported error by trace points, making
> error path more complicated.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> migration/trace-events | 5 ++---
> migration/vmstate.c | 14 ++++++--------
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/migration/trace-events b/migration/trace-events
> index 90629f828f..e864d19c55 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -55,15 +55,14 @@ postcopy_pause_incoming_continued(void) ""
> postcopy_page_req_sync(void *host_addr) "sync page req %p"
>
> # vmstate.c
> -vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret = %d"
> vmstate_load_state(const char *name, int version_id) "%s v%d"
> -vmstate_load_state_end(const char *name, const char *reason, int val) "%s %s/%d"
> +vmstate_load_state_end(const char *name) "%s"
> vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s exists=%d"
> vmstate_n_elems(const char *name, int n_elems) "%s: %d"
> vmstate_subsection_load(const char *parent) "%s"
> vmstate_subsection_load_bad(const char *parent, const char *sub, const char *sub2) "%s: %s/%s"
> vmstate_subsection_load_good(const char *parent) "%s"
> -vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d"
> +vmstate_save_state_pre_save_done(const char *name) "%s"
> vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s[%d]"
> vmstate_save_state_top(const char *idstr) "%s"
> vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index dd7cd27993..b07bbdd366 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -144,7 +144,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> error_setg(errp, "%s: incoming version_id %d is too new "
> "for local version_id %d",
> vmsd->name, version_id, vmsd->version_id);
> - trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> return -EINVAL;
> }
>
> @@ -152,7 +151,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> error_setg(errp, "%s: incoming version_id %d is too old "
> "for local minimum version_id %d",
> vmsd->name, version_id, vmsd->minimum_version_id);
> - trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> return -EINVAL;
> }
>
> @@ -245,7 +243,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>
> if (ret < 0) {
> qemu_file_set_error(f, ret);
> - trace_vmstate_load_field_error(field->name, ret);
> return ret;
> }
> }
> @@ -269,7 +266,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> error_prepend(errp, "post load hook failed for: %s, version_id: "
> "%d, minimum_version: %d: ", vmsd->name,
> vmsd->version_id, vmsd->minimum_version_id);
> - ret = -EINVAL;
> + return -EINVAL;
> }
> } else if (vmsd->post_load) {
> ret = vmsd->post_load(opaque, version_id);
> @@ -279,12 +276,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> "minimum_version: %d, ret: %d",
> vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> ret);
> + return ret;
> }
> }
>
> - trace_vmstate_load_state_end(vmsd->name, "end", ret);
> + trace_vmstate_load_state_end(vmsd->name);
>
> - return ret;
> + return 0;
> }
>
> static int vmfield_name_num(const VMStateField *start,
> @@ -444,20 +442,20 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>
> if (vmsd->pre_save_errp) {
> ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
> - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> if (ret < 0) {
> error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
> return ret;
> }
> } else if (vmsd->pre_save) {
> ret = vmsd->pre_save(opaque);
> - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> if (ret) {
> error_setg(errp, "pre-save failed: %s", vmsd->name);
> return ret;
> }
> }
>
> + trace_vmstate_save_state_pre_save_done(vmsd->name);
> +
> if (vmdesc) {
> json_writer_str(vmdesc, "vmsd_name", vmsd->name);
> json_writer_int64(vmdesc, "version", version_id);
One benefit of the traces is to be able to match with source code when
debugging a QEMU instance that cannot be rebuilt. Not so much about the
trace itself, but to be able to know _where_ the code exited.
However, I have no preference.
Acked-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors
2026-02-27 22:44 ` Fabiano Rosas
@ 2026-03-03 15:13 ` Peter Xu
2026-03-04 16:10 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:13 UTC (permalink / raw)
To: Fabiano Rosas; +Cc: Vladimir Sementsov-Ogievskiy, open list:All patches CC here
On Fri, Feb 27, 2026 at 07:44:48PM -0300, Fabiano Rosas wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>
> > We duplicate some of errors by trace points. That doesn't make
> > real sense: we report the error through errp, and stop migration,
> > no reason to duplicate the reported error by trace points, making
> > error path more complicated.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> > migration/trace-events | 5 ++---
> > migration/vmstate.c | 14 ++++++--------
> > 2 files changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/migration/trace-events b/migration/trace-events
> > index 90629f828f..e864d19c55 100644
> > --- a/migration/trace-events
> > +++ b/migration/trace-events
> > @@ -55,15 +55,14 @@ postcopy_pause_incoming_continued(void) ""
> > postcopy_page_req_sync(void *host_addr) "sync page req %p"
> >
> > # vmstate.c
> > -vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret = %d"
> > vmstate_load_state(const char *name, int version_id) "%s v%d"
> > -vmstate_load_state_end(const char *name, const char *reason, int val) "%s %s/%d"
> > +vmstate_load_state_end(const char *name) "%s"
> > vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s exists=%d"
> > vmstate_n_elems(const char *name, int n_elems) "%s: %d"
> > vmstate_subsection_load(const char *parent) "%s"
> > vmstate_subsection_load_bad(const char *parent, const char *sub, const char *sub2) "%s: %s/%s"
> > vmstate_subsection_load_good(const char *parent) "%s"
> > -vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d"
> > +vmstate_save_state_pre_save_done(const char *name) "%s"
> > vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s[%d]"
> > vmstate_save_state_top(const char *idstr) "%s"
> > vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index dd7cd27993..b07bbdd366 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -144,7 +144,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > error_setg(errp, "%s: incoming version_id %d is too new "
> > "for local version_id %d",
> > vmsd->name, version_id, vmsd->version_id);
> > - trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
> > return -EINVAL;
> > }
> >
> > @@ -152,7 +151,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > error_setg(errp, "%s: incoming version_id %d is too old "
> > "for local minimum version_id %d",
> > vmsd->name, version_id, vmsd->minimum_version_id);
> > - trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> > return -EINVAL;
> > }
> >
> > @@ -245,7 +243,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >
> > if (ret < 0) {
> > qemu_file_set_error(f, ret);
> > - trace_vmstate_load_field_error(field->name, ret);
> > return ret;
> > }
> > }
> > @@ -269,7 +266,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > error_prepend(errp, "post load hook failed for: %s, version_id: "
> > "%d, minimum_version: %d: ", vmsd->name,
> > vmsd->version_id, vmsd->minimum_version_id);
> > - ret = -EINVAL;
> > + return -EINVAL;
> > }
> > } else if (vmsd->post_load) {
> > ret = vmsd->post_load(opaque, version_id);
> > @@ -279,12 +276,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > "minimum_version: %d, ret: %d",
> > vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> > ret);
> > + return ret;
> > }
> > }
> >
> > - trace_vmstate_load_state_end(vmsd->name, "end", ret);
> > + trace_vmstate_load_state_end(vmsd->name);
> >
> > - return ret;
> > + return 0;
> > }
> >
> > static int vmfield_name_num(const VMStateField *start,
> > @@ -444,20 +442,20 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >
> > if (vmsd->pre_save_errp) {
> > ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
> > - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > if (ret < 0) {
> > error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
> > return ret;
> > }
> > } else if (vmsd->pre_save) {
> > ret = vmsd->pre_save(opaque);
> > - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > if (ret) {
> > error_setg(errp, "pre-save failed: %s", vmsd->name);
> > return ret;
> > }
> > }
> >
> > + trace_vmstate_save_state_pre_save_done(vmsd->name);
> > +
> > if (vmdesc) {
> > json_writer_str(vmdesc, "vmsd_name", vmsd->name);
> > json_writer_int64(vmdesc, "version", version_id);
>
> One benefit of the traces is to be able to match with source code when
> debugging a QEMU instance that cannot be rebuilt. Not so much about the
> trace itself, but to be able to know _where_ the code exited.
Yep. If we have them already and not yet too messy, shall we keep it?
>
> However, I have no preference.
>
> Acked-by: Fabiano Rosas <farosas@suse.de>
>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors
2026-03-03 15:13 ` Peter Xu
@ 2026-03-04 16:10 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 16:10 UTC (permalink / raw)
To: Peter Xu, Fabiano Rosas; +Cc: open list:All patches CC here
On 03.03.26 18:13, Peter Xu wrote:
> On Fri, Feb 27, 2026 at 07:44:48PM -0300, Fabiano Rosas wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>
>>> We duplicate some of errors by trace points. That doesn't make
>>> real sense: we report the error through errp, and stop migration,
>>> no reason to duplicate the reported error by trace points, making
>>> error path more complicated.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>> migration/trace-events | 5 ++---
>>> migration/vmstate.c | 14 ++++++--------
>>> 2 files changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/migration/trace-events b/migration/trace-events
>>> index 90629f828f..e864d19c55 100644
>>> --- a/migration/trace-events
>>> +++ b/migration/trace-events
>>> @@ -55,15 +55,14 @@ postcopy_pause_incoming_continued(void) ""
>>> postcopy_page_req_sync(void *host_addr) "sync page req %p"
>>>
>>> # vmstate.c
>>> -vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret = %d"
>>> vmstate_load_state(const char *name, int version_id) "%s v%d"
>>> -vmstate_load_state_end(const char *name, const char *reason, int val) "%s %s/%d"
>>> +vmstate_load_state_end(const char *name) "%s"
>>> vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s exists=%d"
>>> vmstate_n_elems(const char *name, int n_elems) "%s: %d"
>>> vmstate_subsection_load(const char *parent) "%s"
>>> vmstate_subsection_load_bad(const char *parent, const char *sub, const char *sub2) "%s: %s/%s"
>>> vmstate_subsection_load_good(const char *parent) "%s"
>>> -vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d"
>>> +vmstate_save_state_pre_save_done(const char *name) "%s"
>>> vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s[%d]"
>>> vmstate_save_state_top(const char *idstr) "%s"
>>> vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>> index dd7cd27993..b07bbdd366 100644
>>> --- a/migration/vmstate.c
>>> +++ b/migration/vmstate.c
>>> @@ -144,7 +144,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>> error_setg(errp, "%s: incoming version_id %d is too new "
>>> "for local version_id %d",
>>> vmsd->name, version_id, vmsd->version_id);
>>> - trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>>> return -EINVAL;
>>> }
>>>
>>> @@ -152,7 +151,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>> error_setg(errp, "%s: incoming version_id %d is too old "
>>> "for local minimum version_id %d",
>>> vmsd->name, version_id, vmsd->minimum_version_id);
>>> - trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>>> return -EINVAL;
>>> }
>>>
>>> @@ -245,7 +243,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>
>>> if (ret < 0) {
>>> qemu_file_set_error(f, ret);
>>> - trace_vmstate_load_field_error(field->name, ret);
>>> return ret;
>>> }
>>> }
>>> @@ -269,7 +266,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>> error_prepend(errp, "post load hook failed for: %s, version_id: "
>>> "%d, minimum_version: %d: ", vmsd->name,
>>> vmsd->version_id, vmsd->minimum_version_id);
>>> - ret = -EINVAL;
>>> + return -EINVAL;
>>> }
>>> } else if (vmsd->post_load) {
>>> ret = vmsd->post_load(opaque, version_id);
>>> @@ -279,12 +276,13 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>> "minimum_version: %d, ret: %d",
>>> vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
>>> ret);
>>> + return ret;
>>> }
>>> }
>>>
>>> - trace_vmstate_load_state_end(vmsd->name, "end", ret);
>>> + trace_vmstate_load_state_end(vmsd->name);
>>>
>>> - return ret;
>>> + return 0;
>>> }
>>>
>>> static int vmfield_name_num(const VMStateField *start,
>>> @@ -444,20 +442,20 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>>
>>> if (vmsd->pre_save_errp) {
>>> ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
>>> - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>>> if (ret < 0) {
>>> error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>>> return ret;
>>> }
>>> } else if (vmsd->pre_save) {
>>> ret = vmsd->pre_save(opaque);
>>> - trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>>> if (ret) {
>>> error_setg(errp, "pre-save failed: %s", vmsd->name);
>>> return ret;
>>> }
>>> }
>>>
>>> + trace_vmstate_save_state_pre_save_done(vmsd->name);
>>> +
>>> if (vmdesc) {
>>> json_writer_str(vmdesc, "vmsd_name", vmsd->name);
>>> json_writer_int64(vmdesc, "version", version_id);
>>
>> One benefit of the traces is to be able to match with source code when
>> debugging a QEMU instance that cannot be rebuilt. Not so much about the
>> trace itself, but to be able to know _where_ the code exited.
>
> Yep. If we have them already and not yet too messy, shall we keep it?
>
Ok, let's keep
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (4 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 15:22 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 07/16] migration: factor out vmstate_save_field() " Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Simplify vmstate_save_state() which is rather big, and simplify further
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index b07bbdd366..810b131f18 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -430,6 +430,26 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
return true;
}
+static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
+ Error **errp)
+{
+ ERRP_GUARD();
+
+ if (vmsd->pre_save_errp) {
+ if (!vmsd->pre_save_errp(opaque, errp)) {
+ error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
+ return false;
+ }
+ } else if (vmsd->pre_save) {
+ if (vmsd->pre_save(opaque) < 0) {
+ error_setg(errp, "pre-save failed: %s", vmsd->name);
+ return false;
+ }
+ }
+
+ return true;
+}
+
static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp)
@@ -440,18 +460,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
trace_vmstate_save_state_top(vmsd->name);
- if (vmsd->pre_save_errp) {
- ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
- if (ret < 0) {
- error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
- return ret;
- }
- } else if (vmsd->pre_save) {
- ret = vmsd->pre_save(opaque);
- if (ret) {
- error_setg(errp, "pre-save failed: %s", vmsd->name);
- return ret;
- }
+ if (!vmstate_pre_save(vmsd, opaque, errp)) {
+ return -EINVAL;
}
trace_vmstate_save_state_pre_save_done(vmsd->name);
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state()
2026-02-20 21:02 ` [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state() Vladimir Sementsov-Ogievskiy
@ 2026-03-03 15:22 ` Peter Xu
2026-03-04 16:27 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:22 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:04AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Simplify vmstate_save_state() which is rather big, and simplify further
> refactoring.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
One trivial thing..
> ---
> migration/vmstate.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b07bbdd366..810b131f18 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -430,6 +430,26 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
> return true;
> }
>
> +static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
> + Error **errp)
> +{
> + ERRP_GUARD();
I can come up with no reason a caller should pass in NULL for errp.. I
don't know why we assert(errp) so rarely in qemu for similar cases, but
iiuc that's what we really want here..
> +
> + if (vmsd->pre_save_errp) {
> + if (!vmsd->pre_save_errp(opaque, errp)) {
> + error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
> + return false;
> + }
> + } else if (vmsd->pre_save) {
> + if (vmsd->pre_save(opaque) < 0) {
> + error_setg(errp, "pre-save failed: %s", vmsd->name);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc,
> int version_id, Error **errp)
> @@ -440,18 +460,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>
> trace_vmstate_save_state_top(vmsd->name);
>
> - if (vmsd->pre_save_errp) {
> - ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
> - if (ret < 0) {
> - error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
> - return ret;
> - }
> - } else if (vmsd->pre_save) {
> - ret = vmsd->pre_save(opaque);
> - if (ret) {
> - error_setg(errp, "pre-save failed: %s", vmsd->name);
> - return ret;
> - }
> + if (!vmstate_pre_save(vmsd, opaque, errp)) {
> + return -EINVAL;
> }
>
> trace_vmstate_save_state_pre_save_done(vmsd->name);
> --
> 2.52.0
>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state()
2026-03-03 15:22 ` Peter Xu
@ 2026-03-04 16:27 ` Vladimir Sementsov-Ogievskiy
2026-03-04 16:42 ` Peter Xu
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 16:27 UTC (permalink / raw)
To: Peter Xu; +Cc: farosas, open list:All patches CC here
On 03.03.26 18:22, Peter Xu wrote:
> On Sat, Feb 21, 2026 at 12:02:04AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Simplify vmstate_save_state() which is rather big, and simplify further
>> refactoring.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One trivial thing..
>
>> ---
>> migration/vmstate.c | 34 ++++++++++++++++++++++------------
>> 1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index b07bbdd366..810b131f18 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -430,6 +430,26 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
>> return true;
>> }
>>
>> +static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
>> + Error **errp)
>> +{
>> + ERRP_GUARD();
>
> I can come up with no reason a caller should pass in NULL for errp.. I
> don't know why we assert(errp) so rarely in qemu for similar cases, but
> iiuc that's what we really want here..
assert(errp) will not handle error_fatal case correctly. ERRP_GUARD guarantees,
that in case of errp == &error_fatal, the error message will contain what we
prepend by error_prepend().
So, current practice is add ERRP_GUARD in any function which want to use
error_prepend(errp, ..).
May be, we may have some additional macro
#define ERRP_GOOD(); which will assert that errp is not in (NULL, &error_fatal),
and then, use it here, but then we should check, that for the whole callers *
caller either pass errp and has ERRP_GOOD() macro, or pass local error object.
>
>> +
>> + if (vmsd->pre_save_errp) {
>> + if (!vmsd->pre_save_errp(opaque, errp)) {
>> + error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>> + return false;
>> + }
>> + } else if (vmsd->pre_save) {
>> + if (vmsd->pre_save(opaque) < 0) {
>> + error_setg(errp, "pre-save failed: %s", vmsd->name);
>> + return false;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>> void *opaque, JSONWriter *vmdesc,
>> int version_id, Error **errp)
>> @@ -440,18 +460,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>
>> trace_vmstate_save_state_top(vmsd->name);
>>
>> - if (vmsd->pre_save_errp) {
>> - ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
>> - if (ret < 0) {
>> - error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>> - return ret;
>> - }
>> - } else if (vmsd->pre_save) {
>> - ret = vmsd->pre_save(opaque);
>> - if (ret) {
>> - error_setg(errp, "pre-save failed: %s", vmsd->name);
>> - return ret;
>> - }
>> + if (!vmstate_pre_save(vmsd, opaque, errp)) {
>> + return -EINVAL;
>> }
>>
>> trace_vmstate_save_state_pre_save_done(vmsd->name);
>> --
>> 2.52.0
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state()
2026-03-04 16:27 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-04 16:42 ` Peter Xu
0 siblings, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-04 16:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Wed, Mar 04, 2026 at 07:27:48PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 03.03.26 18:22, Peter Xu wrote:
> > On Sat, Feb 21, 2026 at 12:02:04AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Simplify vmstate_save_state() which is rather big, and simplify further
> > > refactoring.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > One trivial thing..
> >
> > > ---
> > > migration/vmstate.c | 34 ++++++++++++++++++++++------------
> > > 1 file changed, 22 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > index b07bbdd366..810b131f18 100644
> > > --- a/migration/vmstate.c
> > > +++ b/migration/vmstate.c
> > > @@ -430,6 +430,26 @@ bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque)
> > > return true;
> > > }
> > > +static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
> > > + Error **errp)
> > > +{
> > > + ERRP_GUARD();
> >
> > I can come up with no reason a caller should pass in NULL for errp.. I
> > don't know why we assert(errp) so rarely in qemu for similar cases, but
> > iiuc that's what we really want here..
>
> assert(errp) will not handle error_fatal case correctly. ERRP_GUARD guarantees,
> that in case of errp == &error_fatal, the error message will contain what we
> prepend by error_prepend().
>
> So, current practice is add ERRP_GUARD in any function which want to use
> error_prepend(errp, ..).
>
> May be, we may have some additional macro
>
> #define ERRP_GOOD(); which will assert that errp is not in (NULL, &error_fatal),
>
> and then, use it here, but then we should check, that for the whole callers *
> caller either pass errp and has ERRP_GOOD() macro, or pass local error object.
Ohhhh, OK, thanks. Let's leave that for later, keep my R-b.
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 07/16] migration: factor out vmstate_save_field() from vmstate_save_state()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (5 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 06/16] migration: factor out vmstate_pre_save() from vmstate_save_state() Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 15:38 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 08/16] migration: factor out vmstate_pre_load() from vmstate_load_state() Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Simplify vmstate_save_state() which is rather big, and simplify further
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 810b131f18..d8c30830d6 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -26,6 +26,9 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
Error **errp);
static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, Error **errp);
+static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc,
+ int version_id, Error **errp);
/* Whether this field should exist for either save or load the VM? */
static bool
@@ -450,6 +453,26 @@ static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
return true;
}
+static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field,
+ JSONWriter *vmdesc, Error **errp)
+{
+ if (field->flags & VMS_STRUCT) {
+ return vmstate_save_state(f, field->vmsd, pv, vmdesc, errp) >= 0;
+ } else if (field->flags & VMS_VSTRUCT) {
+ return vmstate_save_state_v(f, field->vmsd, pv, vmdesc,
+ field->struct_version_id,
+ errp) >= 0;
+ }
+
+ if (field->info->put(f, pv, size, field, vmdesc) < 0) {
+ error_setg(errp, "put failed");
+ return false;
+ }
+
+ return true;
+}
+
static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp)
@@ -542,21 +565,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
i, max_elems);
- if (inner_field->flags & VMS_STRUCT) {
- ret = vmstate_save_state(f, inner_field->vmsd,
- curr_elem, vmdesc_loop, errp);
- } else if (inner_field->flags & VMS_VSTRUCT) {
- ret = vmstate_save_state_v(f, inner_field->vmsd,
- curr_elem, vmdesc_loop,
- inner_field->struct_version_id,
- errp);
- } else {
- ret = inner_field->info->put(f, curr_elem, size,
- inner_field, vmdesc_loop);
- if (ret < 0) {
- error_setg(errp, "put failed");
- }
- }
+ ret = vmstate_save_field(f, curr_elem, size, inner_field,
+ vmdesc_loop, errp) ? 0 : -EINVAL;
written_bytes = qemu_file_transferred(f) - old_offset;
vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 07/16] migration: factor out vmstate_save_field() from vmstate_save_state()
2026-02-20 21:02 ` [PATCH v2 07/16] migration: factor out vmstate_save_field() " Vladimir Sementsov-Ogievskiy
@ 2026-03-03 15:38 ` Peter Xu
2026-03-04 17:22 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Peter Xu @ 2026-03-03 15:38 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:05AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Simplify vmstate_save_state() which is rather big, and simplify further
> refactoring.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
I wonder how hard it is to have this helper also cover the rest in the loop
over elements, on e.g. VMS_ARRAY_OF_POINTER, vmdesc_loop being able to
change, and all the nullptr handling.
I had a feeling that you intentionally skipped those because it'll not be
as trivial - I think it's fine, but IIUC we can always figure a way out.
Can be done later. Agree this is an improvement already,
Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
> migration/vmstate.c | 40 +++++++++++++++++++++++++---------------
> 1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 810b131f18..d8c30830d6 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -26,6 +26,9 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> Error **errp);
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, Error **errp);
> +static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> + void *opaque, JSONWriter *vmdesc,
> + int version_id, Error **errp);
>
> /* Whether this field should exist for either save or load the VM? */
> static bool
> @@ -450,6 +453,26 @@ static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
> return true;
> }
>
> +static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field,
> + JSONWriter *vmdesc, Error **errp)
> +{
> + if (field->flags & VMS_STRUCT) {
> + return vmstate_save_state(f, field->vmsd, pv, vmdesc, errp) >= 0;
> + } else if (field->flags & VMS_VSTRUCT) {
> + return vmstate_save_state_v(f, field->vmsd, pv, vmdesc,
> + field->struct_version_id,
> + errp) >= 0;
> + }
> +
> + if (field->info->put(f, pv, size, field, vmdesc) < 0) {
> + error_setg(errp, "put failed");
> + return false;
> + }
> +
> + return true;
> +}
> +
> static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc,
> int version_id, Error **errp)
> @@ -542,21 +565,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
> i, max_elems);
>
> - if (inner_field->flags & VMS_STRUCT) {
> - ret = vmstate_save_state(f, inner_field->vmsd,
> - curr_elem, vmdesc_loop, errp);
> - } else if (inner_field->flags & VMS_VSTRUCT) {
> - ret = vmstate_save_state_v(f, inner_field->vmsd,
> - curr_elem, vmdesc_loop,
> - inner_field->struct_version_id,
> - errp);
> - } else {
> - ret = inner_field->info->put(f, curr_elem, size,
> - inner_field, vmdesc_loop);
> - if (ret < 0) {
> - error_setg(errp, "put failed");
> - }
> - }
> + ret = vmstate_save_field(f, curr_elem, size, inner_field,
> + vmdesc_loop, errp) ? 0 : -EINVAL;
>
> written_bytes = qemu_file_transferred(f) - old_offset;
> vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
> --
> 2.52.0
>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 07/16] migration: factor out vmstate_save_field() from vmstate_save_state()
2026-03-03 15:38 ` Peter Xu
@ 2026-03-04 17:22 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 17:22 UTC (permalink / raw)
To: Peter Xu; +Cc: farosas, open list:All patches CC here
On 03.03.26 18:38, Peter Xu wrote:
> On Sat, Feb 21, 2026 at 12:02:05AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Simplify vmstate_save_state() which is rather big, and simplify further
>> refactoring.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> I wonder how hard it is to have this helper also cover the rest in the loop
> over elements, on e.g. VMS_ARRAY_OF_POINTER, vmdesc_loop being able to
> change, and all the nullptr handling.
>
> I had a feeling that you intentionally skipped those because it'll not be
> as trivial
Yes, at least it would make me to dig deeper and understand the logic
around vmdesc and vmdesc_loop :) So I decided to limit the change to
what I need for further conversion to bool+errp.
> - I think it's fine, but IIUC we can always figure a way out.
> Can be done later. Agree this is an improvement already,
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 08/16] migration: factor out vmstate_pre_load() from vmstate_load_state()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (6 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 07/16] migration: factor out vmstate_save_field() " Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:11 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 09/16] migration: factor out vmstate_load_field() " Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Simplify vmstate_load_state() which is rather big, and simplify further
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 46 ++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index d8c30830d6..68d5c37992 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -134,6 +134,33 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
}
}
+static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
+ Error **errp)
+{
+ ERRP_GUARD();
+
+ if (vmsd->pre_load_errp) {
+ if (!vmsd->pre_load_errp(opaque, errp)) {
+ error_prepend(errp, "pre load hook failed for: '%s', "
+ "version_id: %d, minimum version_id: %d: ",
+ vmsd->name, vmsd->version_id,
+ vmsd->minimum_version_id);
+ return false;
+ }
+ } else if (vmsd->pre_load) {
+ int ret = vmsd->pre_load(opaque);
+ if (ret) {
+ error_setg(errp, "pre load hook failed for: '%s', "
+ "version_id: %d, minimum version_id: %d, ret: %d",
+ vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
+ ret);
+ return false;
+ }
+ }
+
+ return true;
+}
+
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
@@ -157,23 +184,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return -EINVAL;
}
- if (vmsd->pre_load_errp) {
- if (!vmsd->pre_load_errp(opaque, errp)) {
- error_prepend(errp, "pre load hook failed for: '%s', "
- "version_id: %d, minimum version_id: %d: ",
- vmsd->name, vmsd->version_id,
- vmsd->minimum_version_id);
- return -EINVAL;
- }
- } else if (vmsd->pre_load) {
- ret = vmsd->pre_load(opaque);
- if (ret) {
- error_setg(errp, "pre load hook failed for: '%s', "
- "version_id: %d, minimum version_id: %d, ret: %d",
- vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
- ret);
- return ret;
- }
+ if (!vmstate_pre_load(vmsd, opaque, errp)) {
+ return -EINVAL;
}
while (field->name) {
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 09/16] migration: factor out vmstate_load_field() from vmstate_load_state()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (7 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 08/16] migration: factor out vmstate_pre_load() from vmstate_load_state() Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:14 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 10/16] migration: factor out vmstate_post_load() " Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Simplify vmstate_load_state() which is rather big, and simplify further
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 68d5c37992..7d776ce27a 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -161,6 +161,27 @@ static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
return true;
}
+static bool vmstate_load_field(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
+{
+ if (field->flags & VMS_STRUCT) {
+ return vmstate_load_state(f, field->vmsd, pv, field->vmsd->version_id,
+ errp) >= 0;
+ } else if (field->flags & VMS_VSTRUCT) {
+ return vmstate_load_state(f, field->vmsd, pv, field->struct_version_id,
+ errp) >= 0;
+ }
+
+ if (field->info->get(f, pv, size, field) < 0) {
+ error_setg(errp,
+ "Failed to load element of type %s for %s",
+ field->info->name, field->name);
+ return false;
+ }
+
+ return true;
+}
+
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
@@ -223,24 +244,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
inner_field = field;
}
- if (inner_field->flags & VMS_STRUCT) {
- ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
- inner_field->vmsd->version_id,
- errp);
- } else if (inner_field->flags & VMS_VSTRUCT) {
- ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
- inner_field->struct_version_id,
- errp);
- } else {
- ret = inner_field->info->get(f, curr_elem, size,
- inner_field);
- if (ret < 0) {
- error_setg(errp,
- "Failed to load element of type %s for %s: "
- "%d", inner_field->info->name,
- inner_field->name, ret);
- }
- }
+ ret = vmstate_load_field(f, curr_elem, size, inner_field,
+ errp) ? 0 : -EINVAL;
/* If we used a fake temp field.. free it now */
if (inner_field != field) {
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 10/16] migration: factor out vmstate_post_load() from vmstate_load_state()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (8 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 09/16] migration: factor out vmstate_load_field() " Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:16 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 11/16] migration: convert vmstate_subsection_save/load functions to bool Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Simplify vmstate_load_state() which is rather big, and simplify further
refactoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 46 ++++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 7d776ce27a..8b31aa1c9f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -182,6 +182,33 @@ static bool vmstate_load_field(QEMUFile *f, void *pv, size_t size,
return true;
}
+static bool vmstate_post_load(const VMStateDescription *vmsd,
+ void *opaque, int version_id, Error **errp)
+{
+ ERRP_GUARD();
+
+ if (vmsd->post_load_errp) {
+ if (!vmsd->post_load_errp(opaque, version_id, errp)) {
+ error_prepend(errp, "post load hook failed for: %s, version_id: "
+ "%d, minimum_version: %d: ", vmsd->name,
+ vmsd->version_id, vmsd->minimum_version_id);
+ return false;
+ }
+ } else if (vmsd->post_load) {
+ int ret = vmsd->post_load(opaque, version_id);
+ if (ret < 0) {
+ error_setg(errp,
+ "post load hook failed for: %s, version_id: %d, "
+ "minimum_version: %d, ret: %d",
+ vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
+ ret);
+ return false;
+ }
+ }
+
+ return true;
+}
+
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
@@ -281,23 +308,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return ret;
}
- if (vmsd->post_load_errp) {
- if (!vmsd->post_load_errp(opaque, version_id, errp)) {
- error_prepend(errp, "post load hook failed for: %s, version_id: "
- "%d, minimum_version: %d: ", vmsd->name,
- vmsd->version_id, vmsd->minimum_version_id);
- return -EINVAL;
- }
- } else if (vmsd->post_load) {
- ret = vmsd->post_load(opaque, version_id);
- if (ret < 0) {
- error_setg(errp,
- "post load hook failed for: %s, version_id: %d, "
- "minimum_version: %d, ret: %d",
- vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
- ret);
- return ret;
- }
+ if (!vmstate_post_load(vmsd, opaque, version_id, errp)) {
+ return -EINVAL;
}
trace_vmstate_load_state_end(vmsd->name);
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 11/16] migration: convert vmstate_subsection_save/load functions to bool
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (9 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 10/16] migration: factor out vmstate_post_load() " Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:17 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 12/16] migration: VMStateInfo: introduce new handlers with errp Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Convert them to bool return value, as preparation to further
convertion of vmstate_save/load_state().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/vmstate.c | 57 +++++++++++++++++++++------------------------
1 file changed, 26 insertions(+), 31 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 8b31aa1c9f..69340036f0 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -21,11 +21,11 @@
#include "qemu/error-report.h"
#include "trace.h"
-static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc,
- Error **errp);
-static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, Error **errp);
+static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc,
+ Error **errp);
+static bool vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, Error **errp);
static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp);
@@ -302,10 +302,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
}
assert(field->flags == VMS_END);
- ret = vmstate_subsection_load(f, vmsd, opaque, errp);
- if (ret != 0) {
- qemu_file_set_error(f, ret);
- return ret;
+ if (!vmstate_subsection_load(f, vmsd, opaque, errp)) {
+ qemu_file_set_error(f, -EINVAL);
+ return -EINVAL;
}
if (!vmstate_post_load(vmsd, opaque, version_id, errp)) {
@@ -632,7 +631,7 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
json_writer_end_array(vmdesc);
}
- ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
+ ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp) ? 0 : -EINVAL;
out:
if (vmsd->post_save) {
@@ -662,15 +661,14 @@ int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
errp);
}
-static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, Error **errp)
+static bool vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, Error **errp)
{
ERRP_GUARD();
trace_vmstate_subsection_load(vmsd->name);
while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
char idstr[256], *idstr_ret;
- int ret;
uint8_t version_id, len, size;
const VMStateDescription *sub_vmsd;
@@ -678,12 +676,12 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
if (len < strlen(vmsd->name) + 1) {
/* subsection name has to be "section_name/a" */
trace_vmstate_subsection_load_bad(vmsd->name, "(short)", "");
- return 0;
+ return true;
}
size = qemu_peek_buffer(f, (uint8_t **)&idstr_ret, len, 2);
if (size != len) {
trace_vmstate_subsection_load_bad(vmsd->name, "(peek fail)", "");
- return 0;
+ return true;
}
memcpy(idstr, idstr_ret, size);
idstr[size] = 0;
@@ -691,41 +689,39 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(prefix)");
/* it doesn't have a valid subsection name */
- return 0;
+ return true;
}
sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
if (sub_vmsd == NULL) {
trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
error_setg(errp, "VM subsection '%s' in '%s' does not exist",
idstr, vmsd->name);
- return -ENOENT;
+ return false;
}
qemu_file_skip(f, 1); /* subsection */
qemu_file_skip(f, 1); /* len */
qemu_file_skip(f, len); /* idstr */
version_id = qemu_get_be32(f);
- ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
- if (ret) {
+ if (vmstate_load_state(f, sub_vmsd, opaque, version_id, errp) < 0) {
trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
error_prepend(errp,
- "Loading VM subsection '%s' in '%s' failed: %d: ",
- idstr, vmsd->name, ret);
- return ret;
+ "Loading VM subsection '%s' in '%s' failed: ",
+ idstr, vmsd->name);
+ return false;
}
}
trace_vmstate_subsection_load_good(vmsd->name);
- return 0;
+ return true;
}
-static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, JSONWriter *vmdesc,
- Error **errp)
+static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc,
+ Error **errp)
{
const VMStateDescription * const *sub = vmsd->subsections;
bool vmdesc_has_subsections = false;
- int ret = 0;
trace_vmstate_subsection_save_top(vmsd->name);
while (sub && *sub) {
@@ -749,9 +745,8 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
qemu_put_byte(f, len);
qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
qemu_put_be32(f, vmsdsub->version_id);
- ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc, errp);
- if (ret) {
- return ret;
+ if (vmstate_save_state(f, vmsdsub, opaque, vmdesc, errp) < 0) {
+ return false;
}
if (vmdesc) {
@@ -765,5 +760,5 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
json_writer_end_array(vmdesc);
}
- return ret;
+ return true;
}
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 12/16] migration: VMStateInfo: introduce new handlers with errp
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (10 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 11/16] migration: convert vmstate_subsection_save/load functions to bool Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:19 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 13/16] migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd() Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Add new APIs with errp, to allow handlers report good
error messages. We'll convert existing handlers soon.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/migration/vmstate.h | 17 ++++++++++++++---
migration/vmstate.c | 4 ++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 15578b3e28..167b1aa9cf 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -32,12 +32,16 @@
typedef struct VMStateInfo VMStateInfo;
typedef struct VMStateField VMStateField;
-/* VMStateInfo allows customized migration of objects that don't fit in
+/*
+ * VMStateInfo allows customized migration of objects that don't fit in
* any category in VMStateFlags. Additional information is always passed
- * into get and put in terms of field and vmdesc parameters. However
+ * into load and save in terms of field and vmdesc parameters. However
* these two parameters should only be used in cases when customized
* handling is needed, such as QTAILQ. For primitive data types such as
- * integer, field and vmdesc parameters should be ignored inside get/put.
+ * integer, field and vmdesc parameters should be ignored inside load/save.
+ *
+ * @get and @put are deprecated copies of @load and @save. For new interfaces
+ * use @load and @save.
*/
struct VMStateInfo {
const char *name;
@@ -46,6 +50,13 @@ struct VMStateInfo {
int coroutine_mixed_fn (*put)(QEMUFile *f, void *pv, size_t size,
const VMStateField *field,
JSONWriter *vmdesc);
+ bool coroutine_mixed_fn (*load)(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field,
+ Error **errp);
+ bool coroutine_mixed_fn (*save)(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field,
+ JSONWriter *vmdesc,
+ Error **errp);
};
enum VMStateFlags {
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 69340036f0..efef4d0433 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -170,6 +170,8 @@ static bool vmstate_load_field(QEMUFile *f, void *pv, size_t size,
} else if (field->flags & VMS_VSTRUCT) {
return vmstate_load_state(f, field->vmsd, pv, field->struct_version_id,
errp) >= 0;
+ } else if (field->info->load) {
+ return field->info->load(f, pv, size, field, errp);
}
if (field->info->get(f, pv, size, field) < 0) {
@@ -491,6 +493,8 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
return vmstate_save_state_v(f, field->vmsd, pv, vmdesc,
field->struct_version_id,
errp) >= 0;
+ } else if (field->info->save) {
+ return field->info->save(f, pv, size, field, vmdesc, errp);
}
if (field->info->put(f, pv, size, field, vmdesc) < 0) {
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 13/16] migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd()
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (11 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 12/16] migration: VMStateInfo: introduce new handlers with errp Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:22 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 14/16] migration/cpr: move to new migration APIs Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Introduce new APIs, returning bool.
The analysis
https://lore.kernel.org/qemu-devel/aQDdRn8t0B8oE3gf@x1.local/
shows, that vmstate_load_state() return value actually only
used to check for success, specific errno values doesn't make
sense.
With this commit we introduce new functions with modern bool
interface, and in following commits we'll update the
code base to use them, starting from migration/ code, and
finally we will remove old vmstate_load_state() and
vmstate_save_state().
This patch reworks existing functions to new one, so that
old interfaces are simple wrappers, which will be easy to
remove later.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/migration/vmstate.h | 9 ++++
migration/vmstate.c | 89 ++++++++++++++++++++-----------------
2 files changed, 58 insertions(+), 40 deletions(-)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 167b1aa9cf..a76f259180 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1249,10 +1249,19 @@ extern const VMStateInfo vmstate_info_qlist;
.flags = VMS_END, \
}
+/*
+ * vmstate_load_state() and vmstate_save_state() are
+ * depreacated, use vmstate_load_vmsd() and vmstate_save_vmsd()
+ * instead.
+ */
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp);
int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc, Error **errp);
+bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, int version_id, Error **errp);
+bool vmstate_save_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc, Error **errp);
bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index efef4d0433..84ad8b08d3 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -26,7 +26,7 @@ static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
Error **errp);
static bool vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, Error **errp);
-static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp);
@@ -165,11 +165,11 @@ static bool vmstate_load_field(QEMUFile *f, void *pv, size_t size,
const VMStateField *field, Error **errp)
{
if (field->flags & VMS_STRUCT) {
- return vmstate_load_state(f, field->vmsd, pv, field->vmsd->version_id,
- errp) >= 0;
+ return vmstate_load_vmsd(f, field->vmsd, pv, field->vmsd->version_id,
+ errp);
} else if (field->flags & VMS_VSTRUCT) {
- return vmstate_load_state(f, field->vmsd, pv, field->struct_version_id,
- errp) >= 0;
+ return vmstate_load_vmsd(f, field->vmsd, pv, field->struct_version_id,
+ errp);
} else if (field->info->load) {
return field->info->load(f, pv, size, field, errp);
}
@@ -211,12 +211,11 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
return true;
}
-int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
+bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, int version_id, Error **errp)
{
ERRP_GUARD();
const VMStateField *field = vmsd->fields;
- int ret = 0;
trace_vmstate_load_state(vmsd->name, version_id);
@@ -224,18 +223,18 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
error_setg(errp, "%s: incoming version_id %d is too new "
"for local version_id %d",
vmsd->name, version_id, vmsd->version_id);
- return -EINVAL;
+ return false;
}
if (version_id < vmsd->minimum_version_id) {
error_setg(errp, "%s: incoming version_id %d is too old "
"for local minimum version_id %d",
vmsd->name, version_id, vmsd->minimum_version_id);
- return -EINVAL;
+ return false;
}
if (!vmstate_pre_load(vmsd, opaque, errp)) {
- return -EINVAL;
+ return false;
}
while (field->name) {
@@ -255,6 +254,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
}
for (i = 0; i < n_elems; i++) {
+ bool ok;
void *curr_elem = first_elem + size * i;
const VMStateField *inner_field;
@@ -273,32 +273,30 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
inner_field = field;
}
- ret = vmstate_load_field(f, curr_elem, size, inner_field,
- errp) ? 0 : -EINVAL;
+ 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 (ret >= 0) {
- ret = qemu_file_get_error(f);
+ if (ok) {
+ int ret = qemu_file_get_error(f);
if (ret < 0) {
error_setg(errp,
"Failed to load %s state: stream error: %d",
vmsd->name, ret);
+ return false;
}
- }
-
- if (ret < 0) {
- qemu_file_set_error(f, ret);
- return ret;
+ } else {
+ qemu_file_set_error(f, -EINVAL);
+ return false;
}
}
} else if (field->flags & VMS_MUST_EXIST) {
error_setg(errp, "Input validation failed: %s/%s version_id: %d",
vmsd->name, field->name, vmsd->version_id);
- return -1;
+ return false;
}
field++;
}
@@ -306,16 +304,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (!vmstate_subsection_load(f, vmsd, opaque, errp)) {
qemu_file_set_error(f, -EINVAL);
- return -EINVAL;
+ return false;
}
if (!vmstate_post_load(vmsd, opaque, version_id, errp)) {
- return -EINVAL;
+ return false;
}
trace_vmstate_load_state_end(vmsd->name);
- return 0;
+ return true;
}
static int vmfield_name_num(const VMStateField *start,
@@ -488,11 +486,10 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
JSONWriter *vmdesc, Error **errp)
{
if (field->flags & VMS_STRUCT) {
- return vmstate_save_state(f, field->vmsd, pv, vmdesc, errp) >= 0;
+ return vmstate_save_vmsd(f, field->vmsd, pv, vmdesc, errp);
} else if (field->flags & VMS_VSTRUCT) {
- return vmstate_save_state_v(f, field->vmsd, pv, vmdesc,
- field->struct_version_id,
- errp) >= 0;
+ return vmstate_save_vmsd_v(f, field->vmsd, pv, vmdesc,
+ field->struct_version_id, errp);
} else if (field->info->save) {
return field->info->save(f, pv, size, field, vmdesc, errp);
}
@@ -505,18 +502,18 @@ static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
return true;
}
-static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
+static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
int version_id, Error **errp)
{
ERRP_GUARD();
- int ret = 0;
+ bool ok = true;
const VMStateField *field = vmsd->fields;
trace_vmstate_save_state_top(vmsd->name);
if (!vmstate_pre_save(vmsd, opaque, errp)) {
- return -EINVAL;
+ return false;
}
trace_vmstate_save_state_pre_save_done(vmsd->name);
@@ -597,8 +594,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
i, max_elems);
- ret = vmstate_save_field(f, curr_elem, size, inner_field,
- vmdesc_loop, errp) ? 0 : -EINVAL;
+ 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,
@@ -609,7 +606,7 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
g_clear_pointer((gpointer *)&inner_field, g_free);
}
- if (ret) {
+ if (!ok) {
error_prepend(errp, "Save of field %s/%s failed: ",
vmsd->name, field->name);
goto out;
@@ -635,13 +632,13 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
json_writer_end_array(vmdesc);
}
- ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp) ? 0 : -EINVAL;
+ ok = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
out:
if (vmsd->post_save) {
vmsd->post_save(opaque);
}
- return ret;
+ return ok;
}
static const VMStateDescription *
@@ -658,11 +655,11 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
return NULL;
}
-int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
+bool vmstate_save_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc_id, Error **errp)
{
- return vmstate_save_state_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id,
- errp);
+ return vmstate_save_vmsd_v(f, vmsd, opaque, vmdesc_id, vmsd->version_id,
+ errp);
}
static bool vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
@@ -707,7 +704,7 @@ static bool vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
qemu_file_skip(f, len); /* idstr */
version_id = qemu_get_be32(f);
- if (vmstate_load_state(f, sub_vmsd, opaque, version_id, errp) < 0) {
+ if (!vmstate_load_vmsd(f, sub_vmsd, opaque, version_id, errp)) {
trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
error_prepend(errp,
"Loading VM subsection '%s' in '%s' failed: ",
@@ -749,7 +746,7 @@ static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
qemu_put_byte(f, len);
qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
qemu_put_be32(f, vmsdsub->version_id);
- if (vmstate_save_state(f, vmsdsub, opaque, vmdesc, errp) < 0) {
+ if (!vmstate_save_vmsd(f, vmsdsub, opaque, vmdesc, errp)) {
return false;
}
@@ -766,3 +763,15 @@ static bool vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
return true;
}
+
+int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, JSONWriter *vmdesc_id, Error **errp)
+{
+ return vmstate_save_vmsd(f, vmsd, opaque, vmdesc_id, errp) ? 0 : -EINVAL;
+}
+
+int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
+ void *opaque, int version_id, Error **errp)
+{
+ return vmstate_load_vmsd(f, vmsd, opaque, version_id, errp) ? 0 : -EINVAL;
+}
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 13/16] migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd()
2026-02-20 21:02 ` [PATCH v2 13/16] migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd() Vladimir Sementsov-Ogievskiy
@ 2026-03-03 16:22 ` Peter Xu
0 siblings, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 16:22 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:11AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Introduce new APIs, returning bool.
> The analysis
> https://lore.kernel.org/qemu-devel/aQDdRn8t0B8oE3gf@x1.local/
> shows, that vmstate_load_state() return value actually only
> used to check for success, specific errno values doesn't make
> sense.
>
> With this commit we introduce new functions with modern bool
> interface, and in following commits we'll update the
> code base to use them, starting from migration/ code, and
> finally we will remove old vmstate_load_state() and
> vmstate_save_state().
>
> This patch reworks existing functions to new one, so that
> old interfaces are simple wrappers, which will be easy to
> remove later.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 14/16] migration/cpr: move to new migration APIs
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (12 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 13/16] migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd() Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:23 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 15/16] migration/savevm: " Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx
Cc: farosas, vsementsov, Mark Kanda, Ben Chaney,
open list:All patches CC here
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
include/migration/cpr.h | 2 +-
migration/cpr.c | 22 +++++++++-------------
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 5850fd1788..96ce26e711 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -43,7 +43,7 @@ void cpr_set_incoming_mode(MigMode mode);
bool cpr_is_incoming(void);
bool cpr_state_save(MigrationChannel *channel, Error **errp);
-int cpr_state_load(MigrationChannel *channel, Error **errp);
+bool cpr_state_load(MigrationChannel *channel, Error **errp);
void cpr_state_close(void);
struct QIOChannel *cpr_state_ioc(void);
diff --git a/migration/cpr.c b/migration/cpr.c
index a0b37007f5..05266dfcfd 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -178,7 +178,6 @@ bool cpr_is_incoming(void)
bool cpr_state_save(MigrationChannel *channel, Error **errp)
{
- int ret;
QEMUFile *f;
MigMode mode = migrate_mode();
@@ -199,8 +198,7 @@ bool cpr_state_save(MigrationChannel *channel, Error **errp)
qemu_put_be32(f, QEMU_CPR_FILE_MAGIC);
qemu_put_be32(f, QEMU_CPR_FILE_VERSION);
- ret = vmstate_save_state(f, &vmstate_cpr_state, &cpr_state, 0, errp);
- if (ret) {
+ if (!vmstate_save_vmsd(f, &vmstate_cpr_state, &cpr_state, 0, errp)) {
qemu_fclose(f);
return false;
}
@@ -223,9 +221,8 @@ bool cpr_state_save(MigrationChannel *channel, Error **errp)
return true;
}
-int cpr_state_load(MigrationChannel *channel, Error **errp)
+bool cpr_state_load(MigrationChannel *channel, Error **errp)
{
- int ret;
uint32_t v;
QEMUFile *f;
MigMode mode = 0;
@@ -241,10 +238,10 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
cpr_set_incoming_mode(mode);
f = cpr_transfer_input(channel, errp);
} else {
- return 0;
+ return true;
}
if (!f) {
- return -1;
+ return false;
}
trace_cpr_state_load(MigMode_str(mode));
@@ -254,19 +251,18 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
if (v != QEMU_CPR_FILE_MAGIC) {
error_setg(errp, "Not a migration stream (bad magic %x)", v);
qemu_fclose(f);
- return -EINVAL;
+ return false;
}
v = qemu_get_be32(f);
if (v != QEMU_CPR_FILE_VERSION) {
error_setg(errp, "Unsupported migration stream version %d", v);
qemu_fclose(f);
- return -ENOTSUP;
+ return false;
}
- ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
- if (ret) {
+ if (!vmstate_load_vmsd(f, &vmstate_cpr_state, &cpr_state, 1, errp)) {
qemu_fclose(f);
- return ret;
+ return false;
}
if (migrate_mode() == MIG_MODE_CPR_EXEC) {
@@ -280,7 +276,7 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
*/
cpr_state_file = f;
- return ret;
+ return true;
}
void cpr_state_close(void)
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v2 15/16] migration/savevm: move to new migration APIs
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (13 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 14/16] migration/cpr: move to new migration APIs Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:42 ` Peter Xu
2026-02-20 21:02 ` [PATCH v2 16/16] migration/vmstate-types: " Vladimir Sementsov-Ogievskiy
2026-02-20 21:05 ` [PATCH v2 00/16] migration: more bool+errp APIs Vladimir Sementsov-Ogievskiy
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/savevm.c | 107 +++++++++++++++++++++++----------------------
1 file changed, 55 insertions(+), 52 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index c5236e71ba..8c001d7468 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -205,27 +205,28 @@ void timer_get(QEMUFile *f, QEMUTimer *ts)
* Not in vmstate.c to not add qemu-timer.c as dependency to vmstate.c
*/
-static int get_timer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_timer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
QEMUTimer *v = pv;
timer_get(f, v);
- return 0;
+ return true;
}
-static int put_timer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_timer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
QEMUTimer *v = pv;
timer_put(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_timer = {
.name = "timer",
- .get = get_timer,
- .put = put_timer,
+ .load = load_timer,
+ .save = save_timer,
};
@@ -297,7 +298,7 @@ static uint32_t get_validatable_capabilities_count(void)
return result;
}
-static int configuration_pre_save(void *opaque)
+static bool configuration_pre_save(void *opaque, Error **errp)
{
SaveState *state = opaque;
const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
@@ -318,7 +319,7 @@ static int configuration_pre_save(void *opaque)
}
state->uuid = qemu_uuid;
- return 0;
+ return true;
}
static void configuration_post_save(void *opaque)
@@ -330,7 +331,7 @@ static void configuration_post_save(void *opaque)
state->caps_count = 0;
}
-static int configuration_pre_load(void *opaque)
+static bool configuration_pre_load(void *opaque, Error **errp)
{
SaveState *state = opaque;
@@ -339,7 +340,7 @@ static int configuration_pre_load(void *opaque)
* minimum possible value for this CPU.
*/
state->target_page_bits = migration_legacy_page_bits();
- return 0;
+ return true;
}
static bool configuration_validate_capabilities(SaveState *state)
@@ -376,28 +377,31 @@ static bool configuration_validate_capabilities(SaveState *state)
return ret;
}
-static int configuration_post_load(void *opaque, int version_id)
+static bool configuration_post_load(void *opaque, int version_id, Error **errp)
{
SaveState *state = opaque;
const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
- int ret = 0;
+ bool ok = true;
if (strncmp(state->name, current_name, state->len) != 0) {
- error_report("Machine type received is '%.*s' and local is '%s'",
- (int) state->len, state->name, current_name);
- ret = -EINVAL;
+ error_setg(errp,
+ "Machine type received is '%.*s' and local is '%s'",
+ (int) state->len, state->name, current_name);
+ ok = false;
goto out;
}
if (state->target_page_bits != qemu_target_page_bits()) {
- error_report("Received TARGET_PAGE_BITS is %d but local is %d",
- state->target_page_bits, qemu_target_page_bits());
- ret = -EINVAL;
+ error_setg(errp,
+ "Received TARGET_PAGE_BITS is %d but local is %d",
+ state->target_page_bits, qemu_target_page_bits());
+ ok = false;
goto out;
}
if (!configuration_validate_capabilities(state)) {
- ret = -EINVAL;
+ error_setg(errp, "Failed to validate capabilities");
+ ok = false;
goto out;
}
@@ -409,11 +413,12 @@ out:
state->capabilities = NULL;
state->caps_count = 0;
- return ret;
+ return ok;
}
-static int get_capability(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_capability(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field,
+ Error **errp)
{
MigrationCapability *capability = pv;
char capability_str[UINT8_MAX + 1];
@@ -426,15 +431,16 @@ static int get_capability(QEMUFile *f, void *pv, size_t size,
for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
if (!strcmp(MigrationCapability_str(i), capability_str)) {
*capability = i;
- return 0;
+ return true;
}
}
- error_report("Received unknown capability %s", capability_str);
- return -EINVAL;
+ error_setg(errp, "Received unknown capability %s", capability_str);
+ return false;
}
-static int put_capability(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_capability(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
MigrationCapability *capability = pv;
const char *capability_str = MigrationCapability_str(*capability);
@@ -443,13 +449,13 @@ static int put_capability(QEMUFile *f, void *pv, size_t size,
qemu_put_byte(f, len);
qemu_put_buffer(f, (uint8_t *)capability_str, len);
- return 0;
+ return true;
}
static const VMStateInfo vmstate_info_capability = {
.name = "capability",
- .get = get_capability,
- .put = put_capability,
+ .load = load_capability,
+ .save = save_capability,
};
/* The target-page-bits subsection is present only if the
@@ -539,9 +545,9 @@ static const VMStateDescription vmstate_uuid = {
static const VMStateDescription vmstate_configuration = {
.name = "configuration",
.version_id = 1,
- .pre_load = configuration_pre_load,
- .post_load = configuration_post_load,
- .pre_save = configuration_pre_save,
+ .pre_load_errp = configuration_pre_load,
+ .post_load_errp = configuration_post_load,
+ .pre_save_errp = configuration_pre_save,
.post_save = configuration_post_save,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(len, SaveState),
@@ -969,8 +975,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
}
return ret;
}
- return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
- errp);
+
+ if (!vmstate_load_vmsd(f, se->vmsd, se->opaque, se->load_version_id,
+ errp)) {
+ return -EINVAL;
+ }
+
+ return 0;
}
static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
@@ -1028,8 +1039,6 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
Error **errp)
{
- int ret;
-
if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
return 0;
}
@@ -1049,12 +1058,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
if (!se->vmsd) {
vmstate_save_old_style(f, se, vmdesc);
- } else {
- ret = vmstate_save_state(f, se->vmsd, se->opaque, vmdesc,
- errp);
- if (ret) {
- return ret;
- }
+ } else if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
+ return -EINVAL;
}
trace_savevm_section_end(se->idstr, se->section_id, 0);
@@ -1317,8 +1322,8 @@ static void qemu_savevm_send_configuration(MigrationState *s, QEMUFile *f)
json_writer_start_object(vmdesc, "configuration");
}
- vmstate_save_state(f, &vmstate_configuration, &savevm_state,
- vmdesc, &local_err);
+ vmstate_save_vmsd(f, &vmstate_configuration, &savevm_state,
+ vmdesc, &local_err);
if (local_err) {
error_report_err(local_err);
}
@@ -2748,7 +2753,6 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
{
unsigned int v;
- int ret;
v = qemu_get_be32(f);
if (v != QEMU_VM_FILE_MAGIC) {
@@ -2779,10 +2783,9 @@ static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
return -EINVAL;
}
- ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
- errp);
- if (ret) {
- return ret;
+ if (!vmstate_load_vmsd(f, &vmstate_configuration, &savevm_state, 0,
+ errp)) {
+ return -EINVAL;
}
}
return 0;
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 15/16] migration/savevm: move to new migration APIs
2026-02-20 21:02 ` [PATCH v2 15/16] migration/savevm: " Vladimir Sementsov-Ogievskiy
@ 2026-03-03 16:42 ` Peter Xu
2026-03-04 17:04 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Peter Xu @ 2026-03-03 16:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:13AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
One trivial thing to mention below.
> ---
> migration/savevm.c | 107 +++++++++++++++++++++++----------------------
> 1 file changed, 55 insertions(+), 52 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c5236e71ba..8c001d7468 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -205,27 +205,28 @@ void timer_get(QEMUFile *f, QEMUTimer *ts)
> * Not in vmstate.c to not add qemu-timer.c as dependency to vmstate.c
> */
>
> -static int get_timer(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field)
> +static bool load_timer(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
> {
> QEMUTimer *v = pv;
> timer_get(f, v);
> - return 0;
> + return true;
> }
>
> -static int put_timer(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc)
> +static bool save_timer(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
> {
> QEMUTimer *v = pv;
> timer_put(f, v);
>
> - return 0;
> + return true;
> }
>
> const VMStateInfo vmstate_info_timer = {
> .name = "timer",
> - .get = get_timer,
> - .put = put_timer,
> + .load = load_timer,
> + .save = save_timer,
> };
>
>
> @@ -297,7 +298,7 @@ static uint32_t get_validatable_capabilities_count(void)
> return result;
> }
>
> -static int configuration_pre_save(void *opaque)
> +static bool configuration_pre_save(void *opaque, Error **errp)
> {
> SaveState *state = opaque;
> const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> @@ -318,7 +319,7 @@ static int configuration_pre_save(void *opaque)
> }
> state->uuid = qemu_uuid;
>
> - return 0;
> + return true;
> }
>
> static void configuration_post_save(void *opaque)
> @@ -330,7 +331,7 @@ static void configuration_post_save(void *opaque)
> state->caps_count = 0;
> }
>
> -static int configuration_pre_load(void *opaque)
> +static bool configuration_pre_load(void *opaque, Error **errp)
> {
> SaveState *state = opaque;
>
> @@ -339,7 +340,7 @@ static int configuration_pre_load(void *opaque)
> * minimum possible value for this CPU.
> */
> state->target_page_bits = migration_legacy_page_bits();
> - return 0;
> + return true;
> }
>
> static bool configuration_validate_capabilities(SaveState *state)
> @@ -376,28 +377,31 @@ static bool configuration_validate_capabilities(SaveState *state)
> return ret;
> }
>
> -static int configuration_post_load(void *opaque, int version_id)
> +static bool configuration_post_load(void *opaque, int version_id, Error **errp)
> {
> SaveState *state = opaque;
> const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
> - int ret = 0;
> + bool ok = true;
>
> if (strncmp(state->name, current_name, state->len) != 0) {
> - error_report("Machine type received is '%.*s' and local is '%s'",
> - (int) state->len, state->name, current_name);
> - ret = -EINVAL;
> + error_setg(errp,
> + "Machine type received is '%.*s' and local is '%s'",
> + (int) state->len, state->name, current_name);
> + ok = false;
> goto out;
> }
>
> if (state->target_page_bits != qemu_target_page_bits()) {
> - error_report("Received TARGET_PAGE_BITS is %d but local is %d",
> - state->target_page_bits, qemu_target_page_bits());
> - ret = -EINVAL;
> + error_setg(errp,
> + "Received TARGET_PAGE_BITS is %d but local is %d",
> + state->target_page_bits, qemu_target_page_bits());
> + ok = false;
> goto out;
> }
>
> if (!configuration_validate_capabilities(state)) {
> - ret = -EINVAL;
> + error_setg(errp, "Failed to validate capabilities");
> + ok = false;
> goto out;
> }
>
> @@ -409,11 +413,12 @@ out:
> state->capabilities = NULL;
> state->caps_count = 0;
>
> - return ret;
> + return ok;
> }
>
> -static int get_capability(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field)
> +static bool load_capability(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field,
> + Error **errp)
> {
> MigrationCapability *capability = pv;
> char capability_str[UINT8_MAX + 1];
> @@ -426,15 +431,16 @@ static int get_capability(QEMUFile *f, void *pv, size_t size,
> for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> if (!strcmp(MigrationCapability_str(i), capability_str)) {
> *capability = i;
> - return 0;
> + return true;
> }
> }
> - error_report("Received unknown capability %s", capability_str);
> - return -EINVAL;
> + error_setg(errp, "Received unknown capability %s", capability_str);
> + return false;
> }
>
> -static int put_capability(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc)
> +static bool save_capability(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
> {
> MigrationCapability *capability = pv;
> const char *capability_str = MigrationCapability_str(*capability);
> @@ -443,13 +449,13 @@ static int put_capability(QEMUFile *f, void *pv, size_t size,
>
> qemu_put_byte(f, len);
> qemu_put_buffer(f, (uint8_t *)capability_str, len);
> - return 0;
> + return true;
> }
>
> static const VMStateInfo vmstate_info_capability = {
> .name = "capability",
> - .get = get_capability,
> - .put = put_capability,
> + .load = load_capability,
> + .save = save_capability,
> };
>
> /* The target-page-bits subsection is present only if the
> @@ -539,9 +545,9 @@ static const VMStateDescription vmstate_uuid = {
> static const VMStateDescription vmstate_configuration = {
> .name = "configuration",
> .version_id = 1,
> - .pre_load = configuration_pre_load,
> - .post_load = configuration_post_load,
> - .pre_save = configuration_pre_save,
> + .pre_load_errp = configuration_pre_load,
> + .post_load_errp = configuration_post_load,
> + .pre_save_errp = configuration_pre_save,
> .post_save = configuration_post_save,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT32(len, SaveState),
> @@ -969,8 +975,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
> }
> return ret;
> }
> - return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> - errp);
> +
> + if (!vmstate_load_vmsd(f, se->vmsd, se->opaque, se->load_version_id,
> + errp)) {
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> @@ -1028,8 +1039,6 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
> static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
> Error **errp)
> {
> - int ret;
> -
> if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
> return 0;
> }
> @@ -1049,12 +1058,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc,
> trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
> if (!se->vmsd) {
> vmstate_save_old_style(f, se, vmdesc);
> - } else {
> - ret = vmstate_save_state(f, se->vmsd, se->opaque, vmdesc,
> - errp);
> - if (ret) {
> - return ret;
> - }
> + } else if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
> + return -EINVAL;
> }
I understand you may prefer merging them whenever possible, but for things
like this personally I normally keep "else" and "if" separate:
if (!se->vmsd) {
vmstate_save_old_style(f, se, vmdesc);
} else {
if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
return -EINVAL;
}
}
"if" / "else if" can hid problems when we grow the condition in the future
(I recall we hit one when reviewing the other series adding the _errp()
variances). That doesn't apply here but split "if" and "else" also helps
slightly on readabilty for me, so it says: there's no priority of doing
old_style or vmsd_style, it's just that we support both with/without vmsd
pointer, and it shows clearly what we do on which path.
I would expect the compiler will always generate the same byte code, but I
didn't check.
No strong feelings.
>
> trace_savevm_section_end(se->idstr, se->section_id, 0);
> @@ -1317,8 +1322,8 @@ static void qemu_savevm_send_configuration(MigrationState *s, QEMUFile *f)
> json_writer_start_object(vmdesc, "configuration");
> }
>
> - vmstate_save_state(f, &vmstate_configuration, &savevm_state,
> - vmdesc, &local_err);
> + vmstate_save_vmsd(f, &vmstate_configuration, &savevm_state,
> + vmdesc, &local_err);
> if (local_err) {
> error_report_err(local_err);
> }
> @@ -2748,7 +2753,6 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
> static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
> {
> unsigned int v;
> - int ret;
>
> v = qemu_get_be32(f);
> if (v != QEMU_VM_FILE_MAGIC) {
> @@ -2779,10 +2783,9 @@ static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
> return -EINVAL;
> }
>
> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> - errp);
> - if (ret) {
> - return ret;
> + if (!vmstate_load_vmsd(f, &vmstate_configuration, &savevm_state, 0,
> + errp)) {
> + return -EINVAL;
> }
> }
> return 0;
> --
> 2.52.0
>
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 15/16] migration/savevm: move to new migration APIs
2026-03-03 16:42 ` Peter Xu
@ 2026-03-04 17:04 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 17:04 UTC (permalink / raw)
To: Peter Xu; +Cc: farosas, open list:All patches CC here
On 03.03.26 19:42, Peter Xu wrote:
>> - errp);
>> - if (ret) {
>> - return ret;
>> - }
>> + } else if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
>> + return -EINVAL;
>> }
> I understand you may prefer merging them whenever possible, but for things
> like this personally I normally keep "else" and "if" separate:
>
> if (!se->vmsd) {
> vmstate_save_old_style(f, se, vmdesc);
> } else {
> if (!vmstate_save_vmsd(f, se->vmsd, se->opaque, vmdesc, errp)) {
> return -EINVAL;
> }
> }
>
> "if" / "else if" can hid problems when we grow the condition in the future
> (I recall we hit one when reviewing the other series adding the _errp()
> variances). That doesn't apply here but split "if" and "else" also helps
> slightly on readabilty for me,
Hmm I thought about that. But it seemed strange to me to keep
} else {
if () {
}
}
Now looking at the code after a while, first thought is "o, I'm doing something
wrong removing "else" branch, some cases may be lost!", some extra time is
needed to understand it.
What's not pretty with my code is that when we have some
if () {}
else if () {}
else if () {}
...
We expect that conditions are of the same type, and actions are in corresponding
{} bodies - a kind of smart switch-case. But this final "else if" has absolutely
another type of condition: it's actually an action. And that's why it's hard to
read it for me.
I'll follow your suggestion.
> so it says: there's no priority of doing
> old_style or vmsd_style, it's just that we support both with/without vmsd
> pointer, and it shows clearly what we do on which path.
>
> I would expect the compiler will always generate the same byte code, but I
> didn't check.
>
> No strong feelings.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (14 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 15/16] migration/savevm: " Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:02 ` Vladimir Sementsov-Ogievskiy
2026-03-03 16:56 ` Peter Xu
2026-02-20 21:05 ` [PATCH v2 00/16] migration: more bool+errp APIs Vladimir Sementsov-Ogievskiy
16 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:02 UTC (permalink / raw)
To: peterx; +Cc: farosas, vsementsov, open list:All patches CC here
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
migration/trace-events | 24 +-
migration/vmstate-types.c | 637 +++++++++++++++++++-------------------
2 files changed, 332 insertions(+), 329 deletions(-)
diff --git a/migration/trace-events b/migration/trace-events
index e864d19c55..0d44b4c39a 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -70,20 +70,20 @@ vmstate_subsection_save_top(const char *idstr) "%s"
vmstate_field_exists(const char *vmsd, const char *name, int field_version, int version, int result) "%s:%s field_version %d version %d result %d"
# vmstate-types.c
-get_qtailq(const char *name, int version_id) "%s v%d"
-get_qtailq_end(const char *name, const char *reason, int val) "%s %s/%d"
-put_qtailq(const char *name, int version_id) "%s v%d"
-put_qtailq_end(const char *name, const char *reason) "%s %s"
+load_qtailq(const char *name, int version_id) "%s v%d"
+load_qtailq_end(const char *name) "%s"
+save_qtailq(const char *name, int version_id) "%s v%d"
+save_qtailq_end(const char *name) "%s"
-get_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d"
-get_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, int ret) "%s(%s/%s) %d"
-put_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d"
-put_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, int ret) "%s(%s/%s) %d"
+load_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d"
+load_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name) "%s(%s/%s)"
+save_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d"
+save_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name) "%s(%s/%s)"
-get_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)"
-get_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)"
-put_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)"
-put_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)"
+load_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)"
+load_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)"
+save_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)"
+save_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)"
# qemu-file.c
qemu_file_fclose(void) ""
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 89cb211472..502a3651b3 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -23,132 +23,138 @@
/* bool */
-static int get_bool(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_bool(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
bool *v = pv;
*v = qemu_get_byte(f);
- return 0;
+ return true;
}
-static int put_bool(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_bool(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
bool *v = pv;
qemu_put_byte(f, *v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_bool = {
.name = "bool",
- .get = get_bool,
- .put = put_bool,
+ .load = load_bool,
+ .save = save_bool,
};
/* 8 bit int */
-static int get_int8(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int8(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int8_t *v = pv;
qemu_get_s8s(f, v);
- return 0;
+ return true;
}
-static int put_int8(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_int8(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
int8_t *v = pv;
qemu_put_s8s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_int8 = {
.name = "int8",
- .get = get_int8,
- .put = put_int8,
+ .load = load_int8,
+ .save = save_int8,
};
/* 16 bit int */
-static int get_int16(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int16(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int16_t *v = pv;
qemu_get_sbe16s(f, v);
- return 0;
+ return true;
}
-static int put_int16(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_int16(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
int16_t *v = pv;
qemu_put_sbe16s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_int16 = {
.name = "int16",
- .get = get_int16,
- .put = put_int16,
+ .load = load_int16,
+ .save = save_int16,
};
/* 32 bit int */
-static int get_int32(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int32(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int32_t *v = pv;
qemu_get_sbe32s(f, v);
- return 0;
+ return true;
}
-static int put_int32(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_int32(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
int32_t *v = pv;
qemu_put_sbe32s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_int32 = {
.name = "int32",
- .get = get_int32,
- .put = put_int32,
+ .load = load_int32,
+ .save = save_int32,
};
/* 32 bit int. See that the received value is the same than the one
in the field */
-static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int32_equal(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
+ ERRP_GUARD();
int32_t *v = pv;
int32_t v2;
qemu_get_sbe32s(f, &v2);
if (*v == v2) {
- return 0;
+ return true;
}
- error_report("%" PRIx32 " != %" PRIx32, *v, v2);
+
+ error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
if (field->err_hint) {
- error_printf("%s\n", field->err_hint);
+ error_append_hint(errp, "%s\n", field->err_hint);
}
- return -EINVAL;
+ return false;
}
const VMStateInfo vmstate_info_int32_equal = {
.name = "int32 equal",
- .get = get_int32_equal,
- .put = put_int32,
+ .load = load_int32_equal,
+ .save = save_int32,
};
/* 32 bit int. Check that the received value is non-negative
* and less than or equal to the one in the field.
*/
-static int get_int32_le(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int32_le(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int32_t *cur = pv;
int32_t loaded;
@@ -156,360 +162,385 @@ static int get_int32_le(QEMUFile *f, void *pv, size_t size,
if (loaded >= 0 && loaded <= *cur) {
*cur = loaded;
- return 0;
+ return true;
}
- error_report("Invalid value %" PRId32
- " expecting positive value <= %" PRId32,
- loaded, *cur);
- return -EINVAL;
+
+ error_setg(errp, "Invalid value %" PRId32
+ " expecting positive value <= %" PRId32,
+ loaded, *cur);
+ return false;
}
const VMStateInfo vmstate_info_int32_le = {
.name = "int32 le",
- .get = get_int32_le,
- .put = put_int32,
+ .load = load_int32_le,
+ .save = save_int32,
};
/* 64 bit int */
-static int get_int64(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_int64(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int64_t *v = pv;
qemu_get_sbe64s(f, v);
- return 0;
+ return true;
}
-static int put_int64(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_int64(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
int64_t *v = pv;
qemu_put_sbe64s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_int64 = {
.name = "int64",
- .get = get_int64,
- .put = put_int64,
+ .load = load_int64,
+ .save = save_int64,
};
/* 8 bit unsigned int */
-static int get_uint8(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint8(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint8_t *v = pv;
qemu_get_8s(f, v);
- return 0;
+ return true;
}
-static int put_uint8(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_uint8(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
uint8_t *v = pv;
qemu_put_8s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_uint8 = {
.name = "uint8",
- .get = get_uint8,
- .put = put_uint8,
+ .load = load_uint8,
+ .save = save_uint8,
};
/* 16 bit unsigned int */
-static int get_uint16(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint16(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint16_t *v = pv;
qemu_get_be16s(f, v);
- return 0;
+ return true;
}
-static int put_uint16(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_uint16(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
uint16_t *v = pv;
qemu_put_be16s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_uint16 = {
.name = "uint16",
- .get = get_uint16,
- .put = put_uint16,
+ .load = load_uint16,
+ .save = save_uint16,
};
/* 32 bit unsigned int */
-static int get_uint32(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint32(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint32_t *v = pv;
qemu_get_be32s(f, v);
- return 0;
+ return true;
}
-static int put_uint32(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_uint32(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
uint32_t *v = pv;
qemu_put_be32s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_uint32 = {
.name = "uint32",
- .get = get_uint32,
- .put = put_uint32,
+ .load = load_uint32,
+ .save = save_uint32,
};
/* 32 bit uint. See that the received value is the same than the one
in the field */
-static int get_uint32_equal(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint32_equal(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
+ ERRP_GUARD();
uint32_t *v = pv;
uint32_t v2;
qemu_get_be32s(f, &v2);
if (*v == v2) {
- return 0;
+ return true;
}
- error_report("%" PRIx32 " != %" PRIx32, *v, v2);
+
+ error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
if (field->err_hint) {
- error_printf("%s\n", field->err_hint);
+ error_append_hint(errp, "%s\n", field->err_hint);
}
- return -EINVAL;
+ return false;
}
const VMStateInfo vmstate_info_uint32_equal = {
.name = "uint32 equal",
- .get = get_uint32_equal,
- .put = put_uint32,
+ .load = load_uint32_equal,
+ .save = save_uint32,
};
/* 64 bit unsigned int */
-static int get_uint64(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint64(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint64_t *v = pv;
qemu_get_be64s(f, v);
- return 0;
+ return true;
}
-static int put_uint64(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_uint64(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
uint64_t *v = pv;
qemu_put_be64s(f, v);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_uint64 = {
.name = "uint64",
- .get = get_uint64,
- .put = put_uint64,
+ .load = load_uint64,
+ .save = save_uint64,
};
/* File descriptor communicated via SCM_RIGHTS */
-static int get_fd(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_fd(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
int32_t *v = pv;
if (migrate_mode() == MIG_MODE_CPR_EXEC) {
qemu_get_sbe32s(f, v);
- return 0;
+ return true;
}
- return qemu_file_get_fd(f, v);
+ return qemu_file_get_fd(f, v) >= 0;
}
-static int put_fd(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_fd(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
int32_t *v = pv;
+
if (migrate_mode() == MIG_MODE_CPR_EXEC) {
qemu_put_sbe32s(f, v);
- return 0;
+ return true;
}
- return qemu_file_put_fd(f, *v);
+
+ return qemu_file_put_fd(f, *v) >= 0;
}
const VMStateInfo vmstate_info_fd = {
.name = "fd",
- .get = get_fd,
- .put = put_fd,
+ .load = load_fd,
+ .save = save_fd,
};
-static int get_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
if (qemu_get_byte(f) == VMS_NULLPTR_MARKER) {
- return 0;
+ return true;
}
- error_report("vmstate: get_nullptr expected VMS_NULLPTR_MARKER");
- return -EINVAL;
+
+ error_setg(errp, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
+ return false;
}
-static int put_nullptr(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
if (pv == NULL) {
qemu_put_byte(f, VMS_NULLPTR_MARKER);
- return 0;
+ return true;
}
- error_report("vmstate: put_nullptr must be called with pv == NULL");
- return -EINVAL;
+
+ error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
+ return false;
}
const VMStateInfo vmstate_info_nullptr = {
.name = "nullptr",
- .get = get_nullptr,
- .put = put_nullptr,
+ .load = load_nullptr,
+ .save = save_nullptr,
};
/* 64 bit unsigned int. See that the received value is the same than the one
in the field */
-static int get_uint64_equal(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint64_equal(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
+ ERRP_GUARD();
uint64_t *v = pv;
uint64_t v2;
+
qemu_get_be64s(f, &v2);
if (*v == v2) {
- return 0;
+ return true;
}
- error_report("%" PRIx64 " != %" PRIx64, *v, v2);
+
+ error_setg(errp, "%" PRIx64 " != %" PRIx64, *v, v2);
if (field->err_hint) {
- error_printf("%s\n", field->err_hint);
+ error_append_hint(errp, "%s\n", field->err_hint);
}
- return -EINVAL;
+ return false;
}
const VMStateInfo vmstate_info_uint64_equal = {
.name = "int64 equal",
- .get = get_uint64_equal,
- .put = put_uint64,
+ .load = load_uint64_equal,
+ .save = save_uint64,
};
/* 8 bit int. See that the received value is the same than the one
in the field */
-static int get_uint8_equal(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint8_equal(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
+ ERRP_GUARD();
uint8_t *v = pv;
uint8_t v2;
+
qemu_get_8s(f, &v2);
if (*v == v2) {
- return 0;
+ return true;
}
- error_report("%x != %x", *v, v2);
+
+ error_setg(errp, "%x != %x", *v, v2);
if (field->err_hint) {
- error_printf("%s\n", field->err_hint);
+ error_append_hint(errp, "%s\n", field->err_hint);
}
- return -EINVAL;
+ return false;
}
const VMStateInfo vmstate_info_uint8_equal = {
.name = "uint8 equal",
- .get = get_uint8_equal,
- .put = put_uint8,
+ .load = load_uint8_equal,
+ .save = save_uint8,
};
/* 16 bit unsigned int int. See that the received value is the same than the one
in the field */
-static int get_uint16_equal(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_uint16_equal(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
+ ERRP_GUARD();
uint16_t *v = pv;
uint16_t v2;
+
qemu_get_be16s(f, &v2);
if (*v == v2) {
- return 0;
+ return true;
}
- error_report("%x != %x", *v, v2);
+
+ error_setg(errp, "%x != %x", *v, v2);
if (field->err_hint) {
- error_printf("%s\n", field->err_hint);
+ error_append_hint(errp, "%s\n", field->err_hint);
}
- return -EINVAL;
+ return false;
}
const VMStateInfo vmstate_info_uint16_equal = {
.name = "uint16 equal",
- .get = get_uint16_equal,
- .put = put_uint16,
+ .load = load_uint16_equal,
+ .save = save_uint16,
};
/* CPU_DoubleU type */
-static int get_cpudouble(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_cpudouble(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
CPU_DoubleU *v = pv;
qemu_get_be32s(f, &v->l.upper);
qemu_get_be32s(f, &v->l.lower);
- return 0;
+ return true;
}
-static int put_cpudouble(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_cpudouble(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
CPU_DoubleU *v = pv;
qemu_put_be32s(f, &v->l.upper);
qemu_put_be32s(f, &v->l.lower);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_cpudouble = {
.name = "CPU_Double_U",
- .get = get_cpudouble,
- .put = put_cpudouble,
+ .load = load_cpudouble,
+ .save = save_cpudouble,
};
/* uint8_t buffers */
-static int get_buffer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_buffer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint8_t *v = pv;
qemu_get_buffer(f, v, size);
- return 0;
+ return true;
}
-static int put_buffer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_buffer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
uint8_t *v = pv;
qemu_put_buffer(f, v, size);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_buffer = {
.name = "buffer",
- .get = get_buffer,
- .put = put_buffer,
+ .load = load_buffer,
+ .save = save_buffer,
};
/* unused buffers: space that was used for some fields that are
not useful anymore */
-static int get_unused_buffer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_unused_buffer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
uint8_t buf[1024];
int block_len;
@@ -519,11 +550,13 @@ static int get_unused_buffer(QEMUFile *f, void *pv, size_t size,
size -= block_len;
qemu_get_buffer(f, buf, block_len);
}
- return 0;
+
+ return true;
}
-static int put_unused_buffer(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_unused_buffer(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
static const uint8_t buf[1024];
int block_len;
@@ -534,13 +567,13 @@ static int put_unused_buffer(QEMUFile *f, void *pv, size_t size,
qemu_put_buffer(f, buf, block_len);
}
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_unused_buffer = {
.name = "unused_buffer",
- .get = get_unused_buffer,
- .put = put_unused_buffer,
+ .load = load_unused_buffer,
+ .save = save_unused_buffer,
};
/* vmstate_info_tmp, see VMSTATE_WITH_TMP, the idea is that we allocate
@@ -549,48 +582,34 @@ const VMStateInfo vmstate_info_unused_buffer = {
* in fields that don't really exist in the parent but need to be in the
* stream.
*/
-static int get_tmp(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_tmp(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
- int ret;
- Error *local_err = NULL;
const VMStateDescription *vmsd = field->vmsd;
int version_id = field->version_id;
- void *tmp = g_malloc(size);
+ g_autofree void *tmp = g_malloc(size);
/* Writes the parent field which is at the start of the tmp */
*(void **)tmp = pv;
- ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
- if (ret < 0) {
- error_report_err(local_err);
- }
- g_free(tmp);
- return ret;
+ return vmstate_load_vmsd(f, vmsd, tmp, version_id, errp);
}
-static int put_tmp(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_tmp(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
const VMStateDescription *vmsd = field->vmsd;
- void *tmp = g_malloc(size);
- int ret;
- Error *local_err = NULL;
+ g_autofree void *tmp = g_malloc(size);
/* Writes the parent field which is at the start of the tmp */
*(void **)tmp = pv;
- ret = vmstate_save_state(f, vmsd, tmp, vmdesc, &local_err);
- if (ret) {
- error_report_err(local_err);
- }
- g_free(tmp);
-
- return ret;
+ return vmstate_save_vmsd(f, vmsd, tmp, vmdesc, errp);
}
const VMStateInfo vmstate_info_tmp = {
.name = "tmp",
- .get = get_tmp,
- .put = put_tmp,
+ .load = load_tmp,
+ .save = save_tmp,
};
/* bitmaps (as defined by bitmap.h). Note that size here is the size
@@ -600,11 +619,12 @@ const VMStateInfo vmstate_info_tmp = {
*/
/* This is the number of 64 bit words sent over the wire */
#define BITS_TO_U64S(nr) DIV_ROUND_UP(nr, 64)
-static int get_bitmap(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field)
+static bool load_bitmap(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, Error **errp)
{
unsigned long *bmp = pv;
int i, idx = 0;
+
for (i = 0; i < BITS_TO_U64S(size); i++) {
uint64_t w = qemu_get_be64(f);
bmp[idx++] = w;
@@ -612,14 +632,17 @@ static int get_bitmap(QEMUFile *f, void *pv, size_t size,
bmp[idx++] = w >> 32;
}
}
- return 0;
+
+ return true;
}
-static int put_bitmap(QEMUFile *f, void *pv, size_t size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_bitmap(QEMUFile *f, void *pv, size_t size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
unsigned long *bmp = pv;
int i, idx = 0;
+
for (i = 0; i < BITS_TO_U64S(size); i++) {
uint64_t w = bmp[idx++];
if (sizeof(unsigned long) == 4 && idx < BITS_TO_LONGS(size)) {
@@ -628,23 +651,21 @@ static int put_bitmap(QEMUFile *f, void *pv, size_t size,
qemu_put_be64(f, w);
}
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_bitmap = {
.name = "bitmap",
- .get = get_bitmap,
- .put = put_bitmap,
+ .load = load_bitmap,
+ .save = save_bitmap,
};
/* get for QTAILQ
* meta data about the QTAILQ is encoded in a VMStateField structure
*/
-static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field)
+static bool load_qtailq(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, Error **errp)
{
- int ret = 0;
- Error *local_err = NULL;
const VMStateDescription *vmsd = field->vmsd;
/* size of a QTAILQ element */
size_t size = field->size;
@@ -653,80 +674,76 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
int version_id = field->version_id;
void *elm;
- trace_get_qtailq(vmsd->name, version_id);
+ trace_load_qtailq(vmsd->name, version_id);
if (version_id > vmsd->version_id) {
- error_report("%s %s", vmsd->name, "too new");
- trace_get_qtailq_end(vmsd->name, "too new", -EINVAL);
-
- return -EINVAL;
+ error_setg(errp, "%s %s", vmsd->name, "too new");
+ return false;
}
if (version_id < vmsd->minimum_version_id) {
- error_report("%s %s", vmsd->name, "too old");
- trace_get_qtailq_end(vmsd->name, "too old", -EINVAL);
- return -EINVAL;
+ error_setg(errp, "%s %s", vmsd->name, "too old");
+ return false;
}
while (qemu_get_byte(f)) {
elm = g_malloc(size);
- ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
- if (ret) {
- error_report_err(local_err);
- return ret;
+ if (!vmstate_load_vmsd(f, vmsd, elm, version_id, errp)) {
+ g_free(elm);
+ return false;
}
QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
}
- trace_get_qtailq_end(vmsd->name, "end", ret);
- return ret;
+ trace_load_qtailq_end(vmsd->name);
+ return true;
}
-/* put for QTAILQ */
-static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field, JSONWriter *vmdesc)
+/* save for QTAILQ */
+static bool save_qtailq(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
const VMStateDescription *vmsd = field->vmsd;
/* offset of the QTAILQ entry in a QTAILQ element*/
size_t entry_offset = field->start;
void *elm;
- int ret;
- Error *local_err = NULL;
- trace_put_qtailq(vmsd->name, vmsd->version_id);
+ trace_save_qtailq(vmsd->name, vmsd->version_id);
QTAILQ_RAW_FOREACH(elm, pv, entry_offset) {
qemu_put_byte(f, true);
- ret = vmstate_save_state(f, vmsd, elm, vmdesc, &local_err);
- if (ret) {
- error_report_err(local_err);
- return ret;
+ if (!vmstate_save_vmsd(f, vmsd, elm, vmdesc, errp)) {
+ return false;
}
}
qemu_put_byte(f, false);
- trace_put_qtailq_end(vmsd->name, "end");
+ trace_save_qtailq_end(vmsd->name);
- return 0;
+ return true;
}
const VMStateInfo vmstate_info_qtailq = {
.name = "qtailq",
- .get = get_qtailq,
- .put = put_qtailq,
+ .load = load_qtailq,
+ .save = save_qtailq,
};
-struct put_gtree_data {
+struct save_gtree_data {
QEMUFile *f;
const VMStateDescription *key_vmsd;
const VMStateDescription *val_vmsd;
JSONWriter *vmdesc;
- int ret;
+ Error **errp;
+ bool failed;
};
-static gboolean put_gtree_elem(gpointer key, gpointer value, gpointer data)
+/*
+ * save_gtree_elem - func for g_tree_foreach, return true to stop
+ * iteration.
+ */
+static gboolean save_gtree_elem(gpointer key, gpointer value, gpointer data)
{
- struct put_gtree_data *capsule = (struct put_gtree_data *)data;
+ struct save_gtree_data *capsule = (struct save_gtree_data *)data;
QEMUFile *f = capsule->f;
- int ret;
- Error *local_err = NULL;
qemu_put_byte(f, true);
@@ -734,58 +751,56 @@ static gboolean put_gtree_elem(gpointer key, gpointer value, gpointer data)
if (!capsule->key_vmsd) {
qemu_put_be64(f, (uint64_t)(uintptr_t)(key)); /* direct key */
} else {
- ret = vmstate_save_state(f, capsule->key_vmsd, key, capsule->vmdesc,
- &local_err);
- if (ret) {
- error_report_err(local_err);
- capsule->ret = ret;
+ if (!vmstate_save_vmsd(f, capsule->key_vmsd, key, capsule->vmdesc,
+ capsule->errp)) {
+ capsule->failed = true;
return true;
}
}
/* put the data */
- ret = vmstate_save_state(f, capsule->val_vmsd, value, capsule->vmdesc,
- &local_err);
- if (ret) {
- error_report_err(local_err);
- capsule->ret = ret;
+ if (!vmstate_save_vmsd(f, capsule->val_vmsd, value, capsule->vmdesc,
+ capsule->errp)) {
+ capsule->failed = true;
return true;
}
return false;
}
-static int put_gtree(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_gtree(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
bool direct_key = (!field->start);
const VMStateDescription *key_vmsd = direct_key ? NULL : &field->vmsd[1];
const VMStateDescription *val_vmsd = &field->vmsd[0];
const char *key_vmsd_name = direct_key ? "direct" : key_vmsd->name;
- struct put_gtree_data capsule = {
+ struct save_gtree_data capsule = {
.f = f,
.key_vmsd = key_vmsd,
.val_vmsd = val_vmsd,
.vmdesc = vmdesc,
- .ret = 0};
+ .errp = errp,
+ .failed = false};
GTree **pval = pv;
GTree *tree = *pval;
uint32_t nnodes = g_tree_nnodes(tree);
- int ret;
- trace_put_gtree(field->name, key_vmsd_name, val_vmsd->name, nnodes);
+ trace_save_gtree(field->name, key_vmsd_name, val_vmsd->name, nnodes);
qemu_put_be32(f, nnodes);
- g_tree_foreach(tree, put_gtree_elem, (gpointer)&capsule);
+ g_tree_foreach(tree, save_gtree_elem, (gpointer)&capsule);
qemu_put_byte(f, false);
- ret = capsule.ret;
- if (ret) {
- error_report("%s : failed to save gtree (%d)", field->name, ret);
+ if (capsule.failed) {
+ trace_save_gtree_end(field->name, key_vmsd_name, val_vmsd->name);
+ return false;
}
- trace_put_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
- return ret;
+
+ trace_save_gtree_end(field->name, key_vmsd_name, val_vmsd->name);
+ return true;
}
-static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field)
+static bool load_gtree(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, Error **errp)
{
bool direct_key = (!field->start);
const VMStateDescription *key_vmsd = direct_key ? NULL : &field->vmsd[1];
@@ -798,107 +813,97 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
GTree **pval = pv;
GTree *tree = *pval;
void *key, *val;
- int ret = 0;
- Error *local_err = NULL;
/* in case of direct key, the key vmsd can be {}, ie. check fields */
if (!direct_key && version_id > key_vmsd->version_id) {
- error_report("%s %s", key_vmsd->name, "too new");
- return -EINVAL;
+ error_setg(errp, "%s %s", key_vmsd->name, "too new");
+ return false;
}
if (!direct_key && version_id < key_vmsd->minimum_version_id) {
- error_report("%s %s", key_vmsd->name, "too old");
- return -EINVAL;
+ error_setg(errp, "%s %s", key_vmsd->name, "too old");
+ return false;
}
if (version_id > val_vmsd->version_id) {
- error_report("%s %s", val_vmsd->name, "too new");
- return -EINVAL;
+ error_setg(errp, "%s %s", val_vmsd->name, "too new");
+ return false;
}
if (version_id < val_vmsd->minimum_version_id) {
- error_report("%s %s", val_vmsd->name, "too old");
- return -EINVAL;
+ error_setg(errp, "%s %s", val_vmsd->name, "too old");
+ return false;
}
nnodes = qemu_get_be32(f);
- trace_get_gtree(field->name, key_vmsd_name, val_vmsd->name, nnodes);
+ trace_load_gtree(field->name, key_vmsd_name, val_vmsd->name, nnodes);
while (qemu_get_byte(f)) {
if ((++count) > nnodes) {
- ret = -EINVAL;
break;
}
if (direct_key) {
key = (void *)(uintptr_t)qemu_get_be64(f);
} else {
key = g_malloc0(key_size);
- ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
- if (ret) {
- error_report_err(local_err);
+ if (!vmstate_load_vmsd(f, key_vmsd, key, version_id, errp)) {
goto key_error;
}
}
val = g_malloc0(val_size);
- ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
- if (ret) {
- error_report_err(local_err);
+ if (!vmstate_load_vmsd(f, val_vmsd, val, version_id, errp)) {
goto val_error;
}
g_tree_insert(tree, key, val);
}
if (count != nnodes) {
- error_report("%s inconsistent stream when loading the gtree",
- field->name);
- return -EINVAL;
+ error_setg(errp, "%s inconsistent stream when loading the gtree",
+ field->name);
+ return false;
}
- trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
- return ret;
+
+ trace_load_gtree_end(field->name, key_vmsd_name, val_vmsd->name);
+ return true;
+
val_error:
g_free(val);
+
key_error:
if (!direct_key) {
g_free(key);
}
- trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
- return ret;
+ return false;
}
const VMStateInfo vmstate_info_gtree = {
.name = "gtree",
- .get = get_gtree,
- .put = put_gtree,
+ .load = load_gtree,
+ .save = save_gtree,
};
-static int put_qlist(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field, JSONWriter *vmdesc)
+static bool save_qlist(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, JSONWriter *vmdesc,
+ Error **errp)
{
const VMStateDescription *vmsd = field->vmsd;
/* offset of the QTAILQ entry in a QTAILQ element*/
size_t entry_offset = field->start;
void *elm;
- int ret;
- Error *local_err = NULL;
- trace_put_qlist(field->name, vmsd->name, vmsd->version_id);
+ trace_save_qlist(field->name, vmsd->name, vmsd->version_id);
QLIST_RAW_FOREACH(elm, pv, entry_offset) {
qemu_put_byte(f, true);
- ret = vmstate_save_state(f, vmsd, elm, vmdesc, &local_err);
- if (ret) {
- error_report_err(local_err);
- return ret;
+ if (!vmstate_save_vmsd(f, vmsd, elm, vmdesc, errp)) {
+ return false;
}
}
qemu_put_byte(f, false);
- trace_put_qlist_end(field->name, vmsd->name);
+ trace_save_qlist_end(field->name, vmsd->name);
- return 0;
+ return true;
}
-static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
- const VMStateField *field)
+static bool load_qlist(QEMUFile *f, void *pv, size_t unused_size,
+ const VMStateField *field, Error **errp)
{
- int ret = 0;
- Error *local_err = NULL;
const VMStateDescription *vmsd = field->vmsd;
/* size of a QLIST element */
size_t size = field->size;
@@ -907,23 +912,21 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
int version_id = field->version_id;
void *elm, *prev = NULL;
- trace_get_qlist(field->name, vmsd->name, vmsd->version_id);
+ trace_load_qlist(field->name, vmsd->name, vmsd->version_id);
if (version_id > vmsd->version_id) {
- error_report("%s %s", vmsd->name, "too new");
- return -EINVAL;
+ error_setg(errp, "%s %s", vmsd->name, "too new");
+ return false;
}
if (version_id < vmsd->minimum_version_id) {
- error_report("%s %s", vmsd->name, "too old");
- return -EINVAL;
+ error_setg(errp, "%s %s", vmsd->name, "too old");
+ return false;
}
while (qemu_get_byte(f)) {
elm = g_malloc(size);
- ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
- if (ret) {
- error_report_err(local_err);
+ if (!vmstate_load_vmsd(f, vmsd, elm, version_id, errp)) {
g_free(elm);
- return ret;
+ return false;
}
if (!prev) {
QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset);
@@ -932,13 +935,13 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
}
prev = elm;
}
- trace_get_qlist_end(field->name, vmsd->name);
+ trace_load_qlist_end(field->name, vmsd->name);
- return ret;
+ return true;
}
const VMStateInfo vmstate_info_qlist = {
.name = "qlist",
- .get = get_qlist,
- .put = put_qlist,
+ .load = load_qlist,
+ .save = save_qlist,
};
--
2.52.0
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-02-20 21:02 ` [PATCH v2 16/16] migration/vmstate-types: " Vladimir Sementsov-Ogievskiy
@ 2026-03-03 16:56 ` Peter Xu
2026-03-04 17:38 ` Vladimir Sementsov-Ogievskiy
2026-03-04 19:15 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-03 16:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Sat, Feb 21, 2026 at 12:02:14AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
Likewise.. one trivial thing to mention below..
[...]
> /* 32 bit int. See that the received value is the same than the one
> in the field */
>
> -static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field)
> +static bool load_int32_equal(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
> {
> + ERRP_GUARD();
> int32_t *v = pv;
> int32_t v2;
> qemu_get_sbe32s(f, &v2);
>
> if (*v == v2) {
> - return 0;
> + return true;
> }
> - error_report("%" PRIx32 " != %" PRIx32, *v, v2);
> +
> + error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
> if (field->err_hint) {
> - error_printf("%s\n", field->err_hint);
> + error_append_hint(errp, "%s\n", field->err_hint);
According to doc for error_append_hint():
/*
* Append a printf-style human-readable explanation to an existing error.
* If the error is later reported to a human user with
* error_report_err() or warn_report_err(), the hints will be shown,
* too. If it's reported via QMP, the hints will be ignored.
* Intended use is adding helpful hints on the human user interface,
* e.g. a list of valid values. It's not for clarifying a confusing
* error message.
* @errp may be NULL, but not &error_fatal or &error_abort.
* Trivially the case if you call it only after error_setg() or
* error_propagate().
* May be called multiple times. The resulting hint should end with a
* newline.
*/
It may be ignored if the errp will be finally persisted in MigrationState.
Not a huge deal because I believe we also do error_report_err()
somewhere.. but if we want, I think we could use error_append() to make the
hint available to both.
This comment applies to all 5 similar occurances in this patch.
> }
> - return -EINVAL;
> + return false;
> }
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-03 16:56 ` Peter Xu
@ 2026-03-04 17:38 ` Vladimir Sementsov-Ogievskiy
2026-03-04 18:20 ` Peter Xu
2026-03-04 19:15 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 17:38 UTC (permalink / raw)
To: Peter Xu; +Cc: farosas, open list:All patches CC here
On 03.03.26 19:56, Peter Xu wrote:
> On Sat, Feb 21, 2026 at 12:02:14AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Likewise.. one trivial thing to mention below..
>
> [...]
>
>> /* 32 bit int. See that the received value is the same than the one
>> in the field */
>>
>> -static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
>> - const VMStateField *field)
>> +static bool load_int32_equal(QEMUFile *f, void *pv, size_t size,
>> + const VMStateField *field, Error **errp)
>> {
>> + ERRP_GUARD();
>> int32_t *v = pv;
>> int32_t v2;
>> qemu_get_sbe32s(f, &v2);
>>
>> if (*v == v2) {
>> - return 0;
>> + return true;
>> }
>> - error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>> +
>> + error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
>> if (field->err_hint) {
>> - error_printf("%s\n", field->err_hint);
>> + error_append_hint(errp, "%s\n", field->err_hint);
>
> According to doc for error_append_hint():
>
> /*
> * Append a printf-style human-readable explanation to an existing error.
> * If the error is later reported to a human user with
> * error_report_err() or warn_report_err(), the hints will be shown,
> * too. If it's reported via QMP, the hints will be ignored.
> * Intended use is adding helpful hints on the human user interface,
> * e.g. a list of valid values. It's not for clarifying a confusing
> * error message.
> * @errp may be NULL, but not &error_fatal or &error_abort.
> * Trivially the case if you call it only after error_setg() or
> * error_propagate().
> * May be called multiple times. The resulting hint should end with a
> * newline.
> */
>
> It may be ignored if the errp will be finally persisted in MigrationState.
>
> Not a huge deal because I believe we also do error_report_err()
> somewhere.. but if we want, I think we could use error_append() to make the
> hint available to both.
>
> This comment applies to all 5 similar occurances in this patch.
Agree, will do. That was intuitive choice "err_hint" -> "append_hint".
Actually, I'm unsure, how much is good to omit hints for QMP interface.
Of course, QMP is for machines. But provided errors are for humans anyway,
machines never parse them (OK, sometimes they do, but that's not a good
practice).
I always wandered, why we try to hide information when reporting errors:
don't report filename, line number.. Don't report these hints. In production
solutions final user will never see any errors from QEMU, so these error
messages are only for engineers. Why we have to grep error messages in
code, instead of get exact filename and line number?
>
>> }
>> - return -EINVAL;
>> + return false;
>> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-04 17:38 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-04 18:20 ` Peter Xu
0 siblings, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-04 18:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Wed, Mar 04, 2026 at 08:38:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 03.03.26 19:56, Peter Xu wrote:
> > On Sat, Feb 21, 2026 at 12:02:14AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > Likewise.. one trivial thing to mention below..
> >
> > [...]
> >
> > > /* 32 bit int. See that the received value is the same than the one
> > > in the field */
> > > -static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
> > > - const VMStateField *field)
> > > +static bool load_int32_equal(QEMUFile *f, void *pv, size_t size,
> > > + const VMStateField *field, Error **errp)
> > > {
> > > + ERRP_GUARD();
> > > int32_t *v = pv;
> > > int32_t v2;
> > > qemu_get_sbe32s(f, &v2);
> > > if (*v == v2) {
> > > - return 0;
> > > + return true;
> > > }
> > > - error_report("%" PRIx32 " != %" PRIx32, *v, v2);
> > > +
> > > + error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
> > > if (field->err_hint) {
> > > - error_printf("%s\n", field->err_hint);
> > > + error_append_hint(errp, "%s\n", field->err_hint);
> >
> > According to doc for error_append_hint():
> >
> > /*
> > * Append a printf-style human-readable explanation to an existing error.
> > * If the error is later reported to a human user with
> > * error_report_err() or warn_report_err(), the hints will be shown,
> > * too. If it's reported via QMP, the hints will be ignored.
> > * Intended use is adding helpful hints on the human user interface,
> > * e.g. a list of valid values. It's not for clarifying a confusing
> > * error message.
> > * @errp may be NULL, but not &error_fatal or &error_abort.
> > * Trivially the case if you call it only after error_setg() or
> > * error_propagate().
> > * May be called multiple times. The resulting hint should end with a
> > * newline.
> > */
> >
> > It may be ignored if the errp will be finally persisted in MigrationState.
> >
> > Not a huge deal because I believe we also do error_report_err()
> > somewhere.. but if we want, I think we could use error_append() to make the
> > hint available to both.
> >
> > This comment applies to all 5 similar occurances in this patch.
>
> Agree, will do. That was intuitive choice "err_hint" -> "append_hint".
>
> Actually, I'm unsure, how much is good to omit hints for QMP interface.
> Of course, QMP is for machines. But provided errors are for humans anyway,
> machines never parse them (OK, sometimes they do, but that's not a good
> practice).
>
> I always wandered, why we try to hide information when reporting errors:
> don't report filename, line number.. Don't report these hints. In production
> solutions final user will never see any errors from QEMU, so these error
> messages are only for engineers. Why we have to grep error messages in
> code, instead of get exact filename and line number?
I confess I don't know a solid answer.
My expectation on whatever that will be visible via QMP is, there is always
chance an user will read the message as-is.
Maybe reading file name and line numbers are too much for most users,
especially if that'll be a default behavior for all errors. So it makes
some sense to me to not include those at least in most errors.
For this specific one, I think it's slightly different: here we already
report something as hard to understand to the user with an "A != B"
message.. hence maybe it'll always be good to prepend with "what is put
into the equation", until we will change that to a more human-friendly
error..
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-03 16:56 ` Peter Xu
2026-03-04 17:38 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-04 19:15 ` Vladimir Sementsov-Ogievskiy
2026-03-04 19:20 ` Peter Xu
1 sibling, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 19:15 UTC (permalink / raw)
To: Peter Xu; +Cc: farosas, open list:All patches CC here
On 03.03.26 19:56, Peter Xu wrote:
> On Sat, Feb 21, 2026 at 12:02:14AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Likewise.. one trivial thing to mention below..
>
> [...]
>
>> /* 32 bit int. See that the received value is the same than the one
>> in the field */
>>
>> -static int get_int32_equal(QEMUFile *f, void *pv, size_t size,
>> - const VMStateField *field)
>> +static bool load_int32_equal(QEMUFile *f, void *pv, size_t size,
>> + const VMStateField *field, Error **errp)
>> {
>> + ERRP_GUARD();
>> int32_t *v = pv;
>> int32_t v2;
>> qemu_get_sbe32s(f, &v2);
>>
>> if (*v == v2) {
>> - return 0;
>> + return true;
>> }
>> - error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>> +
>> + error_setg(errp, "%" PRIx32 " != %" PRIx32, *v, v2);
>> if (field->err_hint) {
>> - error_printf("%s\n", field->err_hint);
>> + error_append_hint(errp, "%s\n", field->err_hint);
>
> According to doc for error_append_hint():
>
> /*
> * Append a printf-style human-readable explanation to an existing error.
> * If the error is later reported to a human user with
> * error_report_err() or warn_report_err(), the hints will be shown,
> * too. If it's reported via QMP, the hints will be ignored.
> * Intended use is adding helpful hints on the human user interface,
> * e.g. a list of valid values. It's not for clarifying a confusing
> * error message.
> * @errp may be NULL, but not &error_fatal or &error_abort.
> * Trivially the case if you call it only after error_setg() or
> * error_propagate().
> * May be called multiple times. The resulting hint should end with a
> * newline.
> */
>
> It may be ignored if the errp will be finally persisted in MigrationState.
>
> Not a huge deal because I believe we also do error_report_err()
> somewhere.. but if we want, I think we could use error_append() to make the
> hint available to both.
The only problem is that we don't have error_append() function)
>
> This comment applies to all 5 similar occurances in this patch.
>
>> }
>> - return -EINVAL;
>> + return false;
>> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-04 19:15 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-04 19:20 ` Peter Xu
2026-03-04 20:06 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Peter Xu @ 2026-03-04 19:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: farosas, open list:All patches CC here
On Wed, Mar 04, 2026 at 10:15:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The only problem is that we don't have error_append() function)
I definitely wanted to type error_prepend() when replying... :(
I think it might be better when putting upfront / prepend, say, the
original error should look like this, which is weird to me:
"XXX != YYY NAME"
(maybe even no space???)
With append (when doing that, we may also want to add a comma and some "0x"
prefix?), it can be:
"NAME: XXX != YYY"
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-04 19:20 ` Peter Xu
@ 2026-03-04 20:06 ` Vladimir Sementsov-Ogievskiy
2026-03-04 21:53 ` Peter Xu
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-03-04 20:06 UTC (permalink / raw)
To: Peter Xu
Cc: farosas, open list:All patches CC here, Halil Pasic,
Christian Borntraeger, Eric Farman, alifm, open list:virtio-ccw
[add s390 channel maintainers]
On 04.03.26 22:20, Peter Xu wrote:
> On Wed, Mar 04, 2026 at 10:15:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> The only problem is that we don't have error_append() function)
>
> I definitely wanted to type error_prepend() when replying... :(
>
> I think it might be better when putting upfront / prepend, say, the
> original error should look like this, which is weird to me:
>
> "XXX != YYY NAME"
>
> (maybe even no space???)
>
> With append (when doing that, we may also want to add a comma and some "0x"
> prefix?), it can be:
>
> "NAME: XXX != YYY"
>
Hmm. I'm now trying to find, where this .err_hint is set to not-NULL.. Don't see...
O, I see them:
in hw/s390x/css.c
const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!"
" Likely reason: some sequences of plug and unplug can break"
" migration for machine versions prior to 2.7 (known design flaw).";
const VMStateDescription vmstate_subch_dev = {
.name = "s390_subch_dev",
.version_id = 1,
.minimum_version_id = 1,
.post_load = subch_dev_post_load,
.pre_save = subch_dev_pre_save,
.fields = (const VMStateField[]) {
VMSTATE_UINT8_EQUAL(cssid, SubchDev, "Bug!"), <<<<<<<<<
VMSTATE_UINT8_EQUAL(ssid, SubchDev, "Bug!"), <<<<<<<<<
VMSTATE_UINT16(migrated_schid, SubchDev),
VMSTATE_UINT16_EQUAL(devno, SubchDev, err_hint_devno), <<<<<<<<
"Bug" hints are obviously useless.
About err_hint_devno.. Doesn't seem a good reason to have this .err_hint
field and the whole logic, including extra parameter to several vmstate
macros through the whole code base. And at least mention about 2.7 looks
not relevant today.
I'd replace definition of err_hint_devno by a comment, and drop the whole
.err_hint feature.
What do you think?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v2 16/16] migration/vmstate-types: move to new migration APIs
2026-03-04 20:06 ` Vladimir Sementsov-Ogievskiy
@ 2026-03-04 21:53 ` Peter Xu
0 siblings, 0 replies; 50+ messages in thread
From: Peter Xu @ 2026-03-04 21:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: farosas, open list:All patches CC here, Halil Pasic,
Christian Borntraeger, Eric Farman, alifm, open list:virtio-ccw
On Wed, Mar 04, 2026 at 11:06:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> [add s390 channel maintainers]
>
> On 04.03.26 22:20, Peter Xu wrote:
> > On Wed, Mar 04, 2026 at 10:15:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > The only problem is that we don't have error_append() function)
> >
> > I definitely wanted to type error_prepend() when replying... :(
> >
> > I think it might be better when putting upfront / prepend, say, the
> > original error should look like this, which is weird to me:
> >
> > "XXX != YYY NAME"
> >
> > (maybe even no space???)
> >
> > With append (when doing that, we may also want to add a comma and some "0x"
> > prefix?), it can be:
> >
> > "NAME: XXX != YYY"
> >
>
>
> Hmm. I'm now trying to find, where this .err_hint is set to not-NULL.. Don't see...
>
> O, I see them:
>
> in hw/s390x/css.c
Ouch.. This is the 2nd time in exactly the same day I saw a feature
introduced into migration core with an use case only in s390's css.c file,
which only works for very limited use case but likely not usable for the
rest... The other one is VMS_NULLPTR_MARKER..
https://lore.kernel.org/all/aahgfLaybfjOpsX1@x1.local/#t
>
> const char err_hint_devno[] = "Devno mismatch, tried to load wrong section!"
> " Likely reason: some sequences of plug and unplug can break"
> " migration for machine versions prior to 2.7 (known design flaw).";
>
> const VMStateDescription vmstate_subch_dev = {
> .name = "s390_subch_dev",
> .version_id = 1,
> .minimum_version_id = 1,
> .post_load = subch_dev_post_load,
> .pre_save = subch_dev_pre_save,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT8_EQUAL(cssid, SubchDev, "Bug!"), <<<<<<<<<
> VMSTATE_UINT8_EQUAL(ssid, SubchDev, "Bug!"), <<<<<<<<<
> VMSTATE_UINT16(migrated_schid, SubchDev),
> VMSTATE_UINT16_EQUAL(devno, SubchDev, err_hint_devno), <<<<<<<<
>
>
>
> "Bug" hints are obviously useless.
>
> About err_hint_devno.. Doesn't seem a good reason to have this .err_hint
> field and the whole logic, including extra parameter to several vmstate
> macros through the whole code base. And at least mention about 2.7 looks
> not relevant today.
>
> I'd replace definition of err_hint_devno by a comment, and drop the whole
> .err_hint feature.
>
> What do you think?
Yes, I'd agree.
--
Peter Xu
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v2 00/16] migration: more bool+errp APIs
[not found] <20260220210214.800050-1-vsementsov@yandex-team.ru>
` (15 preceding siblings ...)
2026-02-20 21:02 ` [PATCH v2 16/16] migration/vmstate-types: " Vladimir Sementsov-Ogievskiy
@ 2026-02-20 21:05 ` Vladimir Sementsov-Ogievskiy
16 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2026-02-20 21:05 UTC (permalink / raw)
To: peterx; +Cc: farosas, qemu-devel
I'm sorry, cover-letter missed qemu-devel. add it.
On 21.02.26 00:01, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> As it was discussed here
>
> https://lore.kernel.org/qemu-devel/aQDdRn8t0B8oE3gf@x1.local/
>
> vmstate_load/save_state() return code is not actually used other
> then check for success. Let's convert them to bool.
>
> Also, on same way, introduce errp handlers in VMStateInfo, and
> move to them.
>
> This series converts only migration/ code. Series converting
> other subsystems will follow.
>
> v1 was "[RFC 00/22] migration: convert vmstate_load/save_state"
> Supersedes: <20251028231347.194844-1-vsementsov@yandex-team.ru>
>
> Vladimir Sementsov-Ogievskiy (16):
> migration: vmstate_save_state_v: fix double error_setg
> migration: make vmstate_save_state_v() static
> migration: make .post_save() a void function
> migration: vmstate_load_state(): add some newlines
> migration: vmstate_save/load_state(): stop tracing errors
> migration: factor out vmstate_pre_save() from vmstate_save_state()
> migration: factor out vmstate_save_field() from vmstate_save_state()
> migration: factor out vmstate_pre_load() from vmstate_load_state()
> migration: factor out vmstate_load_field() from vmstate_load_state()
> migration: factor out vmstate_post_load() from vmstate_load_state()
> migration: convert vmstate_subsection_save/load functions to bool
> migration: VMStateInfo: introduce new handlers with errp
> migration: introduce vmstate_load_vmsd() and vmstate_save_vmsd()
> migration/cpr: move to new migration APIs
> migration/savevm: move to new migration APIs
> migration/vmstate-types: move to new migration APIs
>
> docs/devel/migration/main.rst | 2 +-
> hw/ppc/spapr_pci.c | 3 +-
> include/migration/cpr.h | 2 +-
> include/migration/vmstate.h | 39 ++-
> migration/cpr.c | 22 +-
> migration/savevm.c | 110 +++---
> migration/trace-events | 29 +-
> migration/vmstate-types.c | 637 +++++++++++++++++-----------------
> migration/vmstate.c | 349 +++++++++++--------
> target/arm/machine.c | 4 +-
> 10 files changed, 642 insertions(+), 555 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread