Hi,
Thanks for the review.
On Mon, Jul 21, 2025 at 09:10:21PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, 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 | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index
> > 5feaa3244d259874f03048326b2497e7db32e47c..129b19d7603a0ddf8ab6e946e41c1c4d773d1fa8
> > 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",
>
> There are two whitespaces before "in" but I think we only need one.
Yes, missed that. Thanks. Will amend in the next version.
>
> > + idstr, vmsd->name);
> > return -ENOENT;
> > }
> > qemu_file_skip(f, 1); /* subsection */
> > @@ -608,6 +610,8 @@ 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;
> > }
> > }
> >
>
