Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:35:36PM -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 qemu_loadvm_section_part_end() 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 | 23 ++++++++++-------------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 77408347c1f1ca7eb3a04f8f130c20a5a81f6db2..ff2e4f75e070d0f452414f28435905928b1480a7
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2806,21 +2806,20 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type, Error **errp)
> > }
> >
> > static int
> > -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
> > {
> > + ERRP_GUARD();
> > bool trace_downtime = (type == QEMU_VM_SECTION_END);
> > int64_t start_ts, end_ts;
> > uint32_t section_id;
> > SaveStateEntry *se;
> > int ret;
> > - Error *local_err = NULL;
> >
> > section_id = qemu_get_be32(f);
> >
> > ret = qemu_file_get_error(f);
> > if (ret) {
> > - error_report("%s: Failed to read section ID: %d",
> > - __func__, ret);
> > + error_setg(errp, "Failed to read section ID: %d", ret);
> > return ret;
> > }
> >
> > @@ -2831,7 +2830,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> > }
> > }
> > if (se == NULL) {
> > - error_report("Unknown savevm section %d", section_id);
> > + error_setg(errp, "Unknown savevm section %d", section_id);
>
> Drop "savevm" please.
yes, will do.
>
> > return -EINVAL;
> > }
> >
> > @@ -2839,11 +2838,10 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> > start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> > }
> >
> > - ret = vmstate_load(f, se, &local_err);
> > + ret = vmstate_load(f, se, errp);
> > if (ret < 0) {
> > - error_report("error while loading state section id %d(%s)",
> > - section_id, se->idstr);
> > - warn_report_err(local_err);
> > + error_prepend(errp, "error while loading state section id %d(%s):
> > ",
> > + section_id, se->idstr);
> > return ret;
> > }
> >
> > @@ -2854,6 +2852,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t
> > type)
> > }
> >
> > if (!check_section_footer(f, se)) {
> > + error_setg(errp, "Check section footer error, section_id: %d",
>
> This became not very grammatical, maybe drop the "Check" word.
sure, will do.
>
> > + section_id);
> > return -EINVAL;
> > }
> >
> > @@ -3112,7 +3112,7 @@ retry:
> > break;
> > case QEMU_VM_SECTION_PART:
> > case QEMU_VM_SECTION_END:
> > - ret = qemu_loadvm_section_part_end(f, section_type);
> > + ret = qemu_loadvm_section_part_end(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
> > @@ -3136,9 +3136,6 @@ retry:
> >
> > out:
> > if (ret < 0) {
> > - if (*errp == NULL) {
> > - error_setg(errp, "Loading VM state failed: %d", ret);
> > - }
>
> Good. Could have been mentioned in that patch's commit message, or even
> a /* temporary */ comment in the code.
Yes, I shall add it in the commit message.
>
> > qemu_file_set_error(f, ret);
> >
> > /* Cancel bitmaps incoming regardless of recovery */
>
Regards,
Arun
