Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 01:41:40PM -0300, Fabiano Rosas wrote:
> 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_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/savevm.c | 20 +++++++++++++++-----
> > 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 0c445a957fc99f826e6753ed3795bcdd51f1e3f5..7f79461844105bf672314c3325caee9cdb654c27
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -963,14 +963,20 @@ void vmstate_unregister(VMStateIf *obj, const
> > VMStateDescription *vmsd,
> > }
> > }
> >
> > -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> > +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
> > {
> > + int ret;
> > trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
> > if (!se->vmsd) { /* Old style */
> > - return se->ops->load_state(f, se->opaque, se->load_version_id);
> > + ret = se->ops->load_state(f, se->opaque, se->load_version_id);
> > + if (ret < 0) {
> > + error_setg(errp, "Failed to load VM version_id: %d, ret: %d",
>
> "VM" is ambiguous. I'd use "vmstate".
Sure, will do this.
>
> > + se->load_version_id, ret);
> > + }
> > + return ret;
> > }
> > return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > - &error_fatal);
> > + errp);
> > }
> >
> > static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > @@ -2692,6 +2698,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> > SaveStateEntry *se;
> > char idstr[256];
> > int ret;
> > + Error *local_err = NULL;
> >
> > /* Read section start */
> > section_id = qemu_get_be32(f);
> > @@ -2741,10 +2748,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> > start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> > }
> >
> > - ret = vmstate_load(f, se);
> > + ret = vmstate_load(f, se, &local_err);
> > if (ret < 0) {
> > error_report("error while loading state for instance 0x%"PRIx32"
> > of"
> > " device '%s'", instance_id, idstr);
> > + warn_report_err(local_err);
>
> I was about to ask why you're using a warning here, but I see you remove
> it futher in the series. That's fine, but this kind of thing should be
> mentioned in the commit message, otherwise it gets confusing to review.
Sure, I will add it in the commit message.
>
> > return ret;
> > }
> >
> > @@ -2769,6 +2777,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> > uint32_t section_id;
> > SaveStateEntry *se;
> > int ret;
> > + Error *local_err = NULL;
> >
> > section_id = qemu_get_be32(f);
> >
> > @@ -2794,10 +2803,11 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> > start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> > }
> >
> > - ret = vmstate_load(f, se);
> > + ret = vmstate_load(f, se, &local_err);
> > if (ret < 0) {
> > error_report("error while loading state section id %d(%s)",
> > section_id, se->idstr);
> > + warn_report_err(local_err);
> > return ret;
> > }
>
Regards,
Arun Menon
