On Thu, Jul 17, 2025 at 06:07:24AM +0530, Arun Menon wrote:
> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that vmstate_subsection_load() must report an error
> in errp, in case of failure.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
> migration/vmstate.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5feaa3244d259874f03048326b2497e7db32e47c..526668a020562f303d2ddf030b1c8466659b67be 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -25,7 +25,7 @@ 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);
> + void *opaque, Error **errp);
>
> /* Whether this field should exist for either save or load the VM? */
> static bool
> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> field++;
> }
> assert(field->flags == VMS_END);
> - ret = vmstate_subsection_load(f, vmsd, opaque);
> + ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> if (ret != 0) {
> qemu_file_set_error(f, ret);
> return ret;
> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> }
>
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque)
> + void *opaque, Error **errp)
> {
> trace_vmstate_subsection_load(vmsd->name);
>
> @@ -598,6 +598,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> 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 does not exist");
Lets include the contextual details
error_setg(errp, "VM subsection '%s' in '%s' does not exist", idstr, vmsd->name);
> return -ENOENT;
> }
> qemu_file_skip(f, 1); /* subsection */
> @@ -608,6 +609,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> if (ret) {
> trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> + error_setg(errp, "Loading VM subsection failed : %d", ret);
error_setg(errp, "Loading VM subsection '%s' in '%s' failed : %d", idstr, vmsd->name, ret);
> return ret;
> }
> }
In general this method puzzles me. There are four places where we call
trace_vmstate_subsection_load_bad() whose name indicates it is for an
error condition.
The first two places, however, then 'return 0' to the caller indicating
success, which looks inconsistent with the trace point. Assuming that
is correct though, then your patch is also correct in only adding error
reports to these two places.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|