Hi Marc-André, Thanks for the review.
On Mon, Jul 28, 2025 at 12:44:53PM +0400, Marc-André Lureau wrote: > Hi > > On Fri, Jul 25, 2025 at 5:46 PM Marc-André Lureau < > marcandre.lur...@redhat.com> wrote: > > > > > > > On Fri, Jul 25, 2025 at 4:19 PM Arun Menon <arme...@redhat.com> 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 | 11 ++++++++--- > >> 1 file changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/migration/vmstate.c b/migration/vmstate.c > >> index > >> 5feaa3244d259874f03048326b2497e7db32e47c..aeffeafaa4fa7582076a4f2747906ddf9aca891b > >> 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,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", > >> > > > > extra space before ":" > > > > other than that > > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> > > > > Actually, almost systematically when you introduce an extra **errp > argument, you should ERRP_GUARD() in the function (see include/qapi/error.h > doc). Was this discussed before? Can you update the following patches too? I see. Thanks. I shall add ERRP_GUARD() whenever errp is dereferenced or used with error_prepend(). > > > > > >> + idstr, vmsd->name, ret); > >> return ret; > >> } > >> } > >> > >> -- > >> 2.50.0 > >> > >> Regards, Arun