Re: [PATCH v2 05/16] migration: vmstate_save/load_state(): stop tracing errors

[Vladimir Sementsov-Ogievskiy]

On 03.03.26 18:13, Peter Xu wrote:

On Fri, Feb 27, 2026 at 07:44:48PM -0300, Fabiano Rosas wrote:

Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> writes:

We duplicate some of errors by trace points. That doesn't make
real sense: we report the error through errp, and stop migration,
no reason to duplicate the reported error by trace points, making
error path more complicated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
---
  migration/trace-events |  5 ++---
  migration/vmstate.c    | 14 ++++++--------
  2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/migration/trace-events b/migration/trace-events
index 90629f828f..e864d19c55 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -55,15 +55,14 @@ postcopy_pause_incoming_continued(void) ""
  postcopy_page_req_sync(void *host_addr) "sync page req %p"

# vmstate.c

-vmstate_load_field_error(const char *field, int ret) "field \"%s\" load failed, ret 
= %d"
  vmstate_load_state(const char *name, int version_id) "%s v%d"
-vmstate_load_state_end(const char *name, const char *reason, int val) "%s 
%s/%d"
+vmstate_load_state_end(const char *name) "%s"
  vmstate_load_state_field(const char *name, const char *field, bool exists) "%s:%s 
exists=%d"
  vmstate_n_elems(const char *name, int n_elems) "%s: %d"
  vmstate_subsection_load(const char *parent) "%s"
  vmstate_subsection_load_bad(const char *parent,  const char *sub, const char *sub2) 
"%s: %s/%s"
  vmstate_subsection_load_good(const char *parent) "%s"
-vmstate_save_state_pre_save_res(const char *name, int res) "%s/%d"
+vmstate_save_state_pre_save_done(const char *name) "%s"
  vmstate_save_state_loop(const char *name, const char *field, int n_elems) 
"%s/%s[%d]"
  vmstate_save_state_top(const char *idstr) "%s"
  vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
diff --git a/migration/vmstate.c b/migration/vmstate.c
index dd7cd27993..b07bbdd366 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -144,7 +144,6 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
          error_setg(errp, "%s: incoming version_id %d is too new "
                     "for local version_id %d",
                     vmsd->name, version_id, vmsd->version_id);
-        trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
          return -EINVAL;
      }

@@ -152,7 +151,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,

          error_setg(errp, "%s: incoming version_id %d is too old "
                     "for local minimum version_id %d",
                     vmsd->name, version_id, vmsd->minimum_version_id);
-        trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
          return -EINVAL;
      }

@@ -245,7 +243,6 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, if (ret < 0) {

                      qemu_file_set_error(f, ret);
-                    trace_vmstate_load_field_error(field->name, ret);
                      return ret;
                  }
              }
@@ -269,7 +266,7 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
              error_prepend(errp, "post load hook failed for: %s, version_id: "
                            "%d, minimum_version: %d: ", vmsd->name,
                            vmsd->version_id, vmsd->minimum_version_id);
-            ret = -EINVAL;
+            return -EINVAL;
          }
      } else if (vmsd->post_load) {
          ret = vmsd->post_load(opaque, version_id);
@@ -279,12 +276,13 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
                         "minimum_version: %d, ret: %d",
                         vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
                         ret);
+            return ret;
          }
      }

- trace_vmstate_load_state_end(vmsd->name, "end", ret);

+    trace_vmstate_load_state_end(vmsd->name);

- return ret;

+    return 0;
  }

static int vmfield_name_num(const VMStateField *start,

@@ -444,20 +442,20 @@ static int vmstate_save_state_v(QEMUFile *f, const 
VMStateDescription *vmsd,

if (vmsd->pre_save_errp) {

          ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
-        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
          if (ret < 0) {
              error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
              return ret;
          }
      } else if (vmsd->pre_save) {
          ret = vmsd->pre_save(opaque);
-        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
          if (ret) {
              error_setg(errp, "pre-save failed: %s", vmsd->name);
              return ret;
          }
      }

+ trace_vmstate_save_state_pre_save_done(vmsd->name);

+
      if (vmdesc) {
          json_writer_str(vmdesc, "vmsd_name", vmsd->name);
          json_writer_int64(vmdesc, "version", version_id);

One benefit of the traces is to be able to match with source code when
debugging a QEMU instance that cannot be rebuilt. Not so much about the
trace itself, but to be able to know _where_ the code exited.

Yep.  If we have them already and not yet too messy, shall we keep it?

Ok, let's keep




--
Best regards,
Vladimir