On Fri, Feb 27, 2026 at 07:44:48PM -0300, Fabiano Rosas wrote: > Vladimir Sementsov-Ogievskiy <[email protected]> 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 <[email protected]> > > --- > > 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? > > However, I have no preference. > > Acked-by: Fabiano Rosas <[email protected]> > -- Peter Xu
