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

[Fabiano Rosas <farosas@suse.de>]

Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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.

However, I have no preference.

Acked-by: Fabiano Rosas <farosas@suse.de>