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 <arme...@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 :|
