Re: [PATCH v8 05/27] migration: push Error **errp into qemu_loadvm_section_start_full()

[Akihiko Odaki]

On 2025/08/01 22:27, Arun Menon wrote:
Hi Akihiko,
Thanks for the review.

On Fri, Aug 01, 2025 at 04:12:34PM +0900, Akihiko Odaki wrote:
On 2025/07/31 22:20, 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 qemu_loadvm_section_start_full() must report an error
in errp, in case of failure.

Signed-off-by: Arun Menon <arme...@redhat.com>
---
   migration/savevm.c | 36 ++++++++++++++++++++----------------
   1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 
736410be867a29efa24d749528c9bc203a3e8131..59751677c1bb7c893b4ba61cbfe1f55ade6ad598
 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2690,8 +2690,9 @@ 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;
@@ -2702,8 +2703,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
       /* 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);
@@ -2711,8 +2712,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;
       }
@@ -2721,17 +2721,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". "
+                   "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",
+                   version_id, idstr, se->version_id);
           return -EINVAL;
       }
       se->load_version_id = version_id;
@@ -2739,7 +2739,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;
       }
@@ -2747,10 +2747,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, NULL);
+    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);
+        error_prepend(errp,
+                      "error while loading state for instance 0x%"PRIx32" of"
+                      " device '%s': ", instance_id, idstr);
           return ret;
       }
@@ -2761,6 +2762,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);
           return -EINVAL;
       }
@@ -3063,7 +3067,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, NULL);

The converted error_report() calls are temporarily dismissed. This can be
fixed by moving "[PATCH v8 19/27] migration: push Error **errp into
qemu_loadvm_state_main()" and changes for its caller earlier than this.

qemu_loadvm_state_main() can have some code to fill errp until all the
functions its calls get converted. It will not make the patch bigger thanks
to the unified error handling path with "goto out" and the removal of code
changes to replace NULL with errp.

I see your point.
There is a cyclic dependency between few functions.
For example, qemu_loadvm_state_main() calls -> loadvm_process_command()
                                      calls -> loadvm_handle_cmd_packaged()
                                      calls -> qemu_loadvm_state_main()
That is why I passed NULL temporarily and then change that in the subsequent
patches. However, I see that this will cause problems for reviewing and 
bisection.
I can introduce a local_err in the caller, and when errp is available, I can 
pass that.
That way I will be reporting local_err after it returns. That way the individual
patches will report the error.

It is not necessary to introduce "local_err"; ERRP_GUARD() is added in 
"[PATCH v8 19/27] migration: push Error **errp into 
qemu_loadvm_state_main()" so you can check if *errp == NULL. This 
hopefully break the cycle dependency without making patches bigger.

Regards,
Akihiko Odaki