Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 05:06:11PM -0300, Fabiano Rosas wrote:
> Fabiano Rosas <faro...@suse.de> writes:
>
> > Arun Menon <arme...@redhat.com> writes:
> >
> >> 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.
> >>
> >> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> >> Signed-off-by: Arun Menon <arme...@redhat.com>
> >> ---
> >> migration/vmstate.c | 11 ++++++++---
> >> 1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index
> >> 5feaa3244d259874f03048326b2497e7db32e47c..6108c7fe283a5013ce42ea9987723c489aef26a2
> >> 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, &error_fatal);
> >> if (ret != 0) {
> >> qemu_file_set_error(f, ret);
> >> return ret;
> >
> > This is now unreachable, no?
It is.
> >
>
> Also, this temporary &error_fatal here and throughout the series will
> break bisect badly, won't it? Imagine we have a bug in the code past
> this point (once the future patch from this series removes the
> &error_fatal), now every time this commit shows up in bisect, it will
> abort earlier.
>
> I get that having error_fatal here helps ensure the series is correct,
> but maybe we should do without it.
That is a valid point.
I think, we can pass local_err and then report it using
warn_report_err() whenever there are operations past the function call.
If we are calling the function in the return statement, then we can pass
error_fatal. If that is ok, I shall amend the series to pass local_err and
temporarily report it.
>
> Do others have an opinion here?
>
> >> @@ -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,8 @@ 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 '%s' in '%s' does not exist",
> >> + idstr, vmsd->name);
> >> return -ENOENT;
> >> }
> >> qemu_file_skip(f, 1); /* subsection */
> >> @@ -608,6 +610,9 @@ 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 '%s' in '%s' failed: %d",
> >> + idstr, vmsd->name, ret);
> >> return ret;
> >> }
> >> }
>
Regards,
Arun
