Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:29:49PM -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_start_full() 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 | 38 ++++++++++++++++++++------------------
> > 1 file changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > 9ec07892cd6ea666431410657c840b6325377d97..77408347c1f1ca7eb3a04f8f130c20a5a81f6db2
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2724,21 +2724,21 @@ static bool check_section_footer(QEMUFile *f,
> > SaveStateEntry *se)
> > }
> >
> > static int
> > -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
> > {
> > + ERRP_GUARD();
> > bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
> > uint32_t instance_id, version_id, section_id;
> > int64_t start_ts, end_ts;
> > SaveStateEntry *se;
> > char idstr[256];
> > int ret;
> > - Error *local_err = NULL;
> >
> > /* Read section start */
> > section_id = qemu_get_be32(f);
> > if (!qemu_get_counted_string(f, idstr)) {
> > - error_report("Unable to read ID string for section %u",
> > - section_id);
> > + error_setg(errp, "Unable to read ID string for section %u",
> > + section_id);
> > return -EINVAL;
> > }
> > instance_id = qemu_get_be32(f);
> > @@ -2746,8 +2746,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> >
> > ret = qemu_file_get_error(f);
> > if (ret) {
> > - error_report("%s: Failed to read instance/version ID: %d",
> > - __func__, ret);
> > + error_setg(errp, "Failed to read instance/version ID: %d", ret);
> > return ret;
> > }
> >
> > @@ -2756,17 +2755,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> > /* Find savevm section */
> > se = find_se(idstr, instance_id);
> > if (se == NULL) {
> > - error_report("Unknown savevm section or instance '%s' %"PRIu32". "
> > - "Make sure that your current VM setup matches your "
> > - "saved VM setup, including any hotplugged devices",
> > - idstr, instance_id);
> > + error_setg(errp, "Unknown savevm section or instance '%s'
> > %"PRIu32". "
>
> Drop the "savevm" please.
yes. Thanks.
>
> > + "Make sure that your current VM setup matches your "
> > + "saved VM setup, including any hotplugged devices",
> > + idstr, instance_id);
> > return -EINVAL;
> > }
> >
> > /* Validate version */
> > if (version_id > se->version_id) {
> > - error_report("savevm: unsupported version %d for '%s' v%d",
> > - version_id, idstr, se->version_id);
> > + error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
>
> Same.
yes
>
> > + version_id, idstr, se->version_id);
> > return -EINVAL;
> > }
> > se->load_version_id = version_id;
> > @@ -2774,7 +2773,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> >
> > /* Validate if it is a device's state */
> > if (xen_enabled() && se->is_ram) {
> > - error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
> > + error_setg(errp, "loadvm: %s RAM loading not allowed on Xen",
> > idstr);
> > return -EINVAL;
> > }
> >
> > @@ -2782,11 +2781,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, &local_err);
> > + ret = vmstate_load(f, se, errp);
> > if (ret < 0) {
> > - error_report("error while loading state for instance 0x%"PRIx32"
> > of"
> > - " device '%s'", instance_id, idstr);
> > - warn_report_err(local_err);
> > + error_prepend(errp,
> > + "error while loading state for instance 0x%"PRIx32"
> > of"
> > + " device '%s': ", instance_id, idstr);
> > return ret;
> > }
> >
> > @@ -2797,6 +2796,9 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t
> > type)
> > }
> >
> > if (!check_section_footer(f, se)) {
> > + error_setg(errp, "Reading footer section of instance "
> > + "0x%"PRIx32" of device '%s' for version_id: %d failed",
> > + instance_id, idstr, version_id);
>
> check_section_footer() already has messages saying something went wrong
> with the footer. Make sure you're not duplicating information. If
> necessary, trim it either here or there.
yes. The function indeed reports error. I needed to set errp though.
I will trim the message.
>
> > return -EINVAL;
> > }
> >
> > @@ -3103,7 +3105,7 @@ retry:
> > switch (section_type) {
> > case QEMU_VM_SECTION_START:
> > case QEMU_VM_SECTION_FULL:
> > - ret = qemu_loadvm_section_start_full(f, section_type);
> > + ret = qemu_loadvm_section_start_full(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
>
Regards,
Arun
