Re: [Qemu-devel] [PATCH 2/3] Migration: Add lots of trace events

[Amit Shah]

On (Tue) 20 Jan 2015 [14:48:02], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> Mostly on the load side, so that when we get a complaint about
> a migration failure we can figure out what it didn't like.

Nice!

Just one note below.

> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  migration/vmstate.c | 22 +++++++++++++++++++---
>  savevm.c            | 10 +++++++---
>  trace-events        | 11 ++++++++++-
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 2c0b135..7b8dc3f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -75,14 +75,19 @@ int vmstate_load_state(QEMUFile *f, const 
> VMStateDescription *vmsd,
>      VMStateField *field = vmsd->fields;
>      int ret;
>  
> +    trace_vmstate_load_state(vmsd->name, version_id);
>      if (version_id > vmsd->version_id) {
> +        trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>          return -EINVAL;
>      }
>      if  (version_id < vmsd->minimum_version_id) {
>          if (vmsd->load_state_old &&
>              version_id >= vmsd->minimum_version_id_old) {
> -            return vmsd->load_state_old(f, opaque, version_id);
> +            ret = vmsd->load_state_old(f, opaque, version_id);
> +            trace_vmstate_load_state_end(vmsd->name, "old path", ret);
> +            return ret;
>          }
> +        trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>          return -EINVAL;
>      }
>      if (vmsd->pre_load) {
> @@ -92,6 +97,7 @@ int vmstate_load_state(QEMUFile *f, const 
> VMStateDescription *vmsd,
>          }
>      }
>      while (field->name) {
> +        trace_vmstate_load_state_field(vmsd->name, field->name);
>          if ((field->field_exists &&
>               field->field_exists(opaque, version_id)) ||
>              (!field->field_exists &&
> @@ -134,9 +140,10 @@ int vmstate_load_state(QEMUFile *f, const 
> VMStateDescription *vmsd,
>          return ret;
>      }
>      if (vmsd->post_load) {
> -        return vmsd->post_load(opaque, version_id);
> +        ret = vmsd->post_load(opaque, version_id);
>      }
> -    return 0;
> +    trace_vmstate_load_state_end(vmsd->name, "end", ret);
> +    return ret;
>  }

"return 0" becomes "return ret", and ret isn't assigned anywhere.

>  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> @@ -193,6 +200,8 @@ static const VMStateDescription *
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription 
> *vmsd,
>                                     void *opaque)
>  {
> +    trace_vmstate_subsection_load(vmsd->name);
> +
>      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
>          char idstr[256];
>          int ret;
> @@ -202,20 +211,24 @@ static int vmstate_subsection_load(QEMUFile *f, const 
> VMStateDescription *vmsd,
>          len = qemu_peek_byte(f, 1);
>          if (len < strlen(vmsd->name) + 1) {
>              /* subsection name has be be "section_name/a" */
> +            trace_vmstate_subsection_load_bad(vmsd->name, "(short)");
>              return 0;

Nice how you use the () to differentiate from the idstr below..

>          }
>          size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
>          if (size != len) {
> +            trace_vmstate_subsection_load_bad(vmsd->name, "(peek fail)");
>              return 0;
>          }
>          idstr[size] = 0;
>  
>          if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
> +            trace_vmstate_subsection_load_bad(vmsd->name, idstr);
>              /* it don't have a valid subsection name */
>              return 0;


                Amit