[Qemu-devel] [PATCH 0/2] Add error reporting in migration

[Qemu-devel] [PATCH 0/2] Add error reporting in migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

At the moment if you use a VMSTATE_*_EQUAL macro and the value
doesn't match you just get an error about the section that failed

e.g.
  qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
  qemu-system-ppc64: load of migration failed: Invalid argument

with this pair you get the field and the mismatched values.
e.g.
  qemu-system-ppc64: 8000600FE1FF7AE1 != 8000600FE1FF3A21
  qemu-system-ppc64: Failed to load cpu:env.insns_flags
  qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
  qemu-system-ppc64: load of migration failed: Invalid argument

which is much more likely to point you at the culprit.

(Broken out from a larger vmstatification series, the only change since
then is the values are printed in hex except for the le case).

Dave

Dr. David Alan Gilbert (2):
  migration: report an error giving the failed field
  migration: Report values for comparisons

 migration/vmstate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

When a field fails to load (typically due to a limit
check, or a call to a get/put) report the device and field
to give an indication of the cause.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/vmstate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index fc29acf..1d637b2 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -130,6 +130,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 }
                 if (ret < 0) {
                     qemu_file_set_error(f, ret);
+                    error_report("Failed to load %s:%s", vmsd->name,
+                                 field->name);
                     trace_vmstate_load_field_error(field->name, ret);
                     return ret;
                 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> When a field fails to load (typically due to a limit
> check, or a call to a get/put) report the device and field
> to give an indication of the cause.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/vmstate.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fc29acf..1d637b2 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -130,6 +130,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>                  if (ret < 0) {
>                      qemu_file_set_error(f, ret);
> +                    error_report("Failed to load %s:%s", vmsd->name,
> +                                 field->name);
>                      trace_vmstate_load_field_error(field->name, ret);
>                      return ret;
>                  }

Applied, thanks.

[Qemu-devel] [PATCH 2/2] migration: Report values for comparisons

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Report the values when a comparison fails; together with
the previous patch that prints the device and field names
this should give a good idea of why loading the migration failed.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/vmstate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 1d637b2..0bc9f35 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -557,6 +557,7 @@ static int get_int32_equal(QEMUFile *f, void *pv, size_t size)
     if (*v == v2) {
         return 0;
     }
+    error_report("%" PRIx32 " != %" PRIx32, *v, v2);
     return -EINVAL;
 }
 
@@ -580,6 +581,9 @@ static int get_int32_le(QEMUFile *f, void *pv, size_t size)
         *cur = loaded;
         return 0;
     }
+    error_report("Invalid value %" PRId32
+                 " expecting positive value <= %" PRId32,
+                 loaded, *cur);
     return -EINVAL;
 }
 
@@ -685,6 +689,7 @@ static int get_uint32_equal(QEMUFile *f, void *pv, size_t size)
     if (*v == v2) {
         return 0;
     }
+    error_report("%" PRIx32 " != %" PRIx32, *v, v2);
     return -EINVAL;
 }
 
@@ -727,6 +732,7 @@ static int get_uint64_equal(QEMUFile *f, void *pv, size_t size)
     if (*v == v2) {
         return 0;
     }
+    error_report("%" PRIx64 " != %" PRIx64, *v, v2);
     return -EINVAL;
 }
 
@@ -748,6 +754,7 @@ static int get_uint8_equal(QEMUFile *f, void *pv, size_t size)
     if (*v == v2) {
         return 0;
     }
+    error_report("%x != %x", *v, v2);
     return -EINVAL;
 }
 
@@ -769,6 +776,7 @@ static int get_uint16_equal(QEMUFile *f, void *pv, size_t size)
     if (*v == v2) {
         return 0;
     }
+    error_report("%x != %x", *v, v2);
     return -EINVAL;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Report the values when a comparison fails; together with
> the previous patch that prints the device and field names
> this should give a good idea of why loading the migration failed.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/vmstate.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 1d637b2..0bc9f35 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -557,6 +557,7 @@ static int get_int32_equal(QEMUFile *f, void *pv, size_t size)
>      if (*v == v2) {
>          return 0;
>      }
> +    error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>      return -EINVAL;
>  }
>  
> @@ -580,6 +581,9 @@ static int get_int32_le(QEMUFile *f, void *pv, size_t size)
>          *cur = loaded;
>          return 0;
>      }
> +    error_report("Invalid value %" PRId32
> +                 " expecting positive value <= %" PRId32,
> +                 loaded, *cur);
>      return -EINVAL;
>  }
>  
> @@ -685,6 +689,7 @@ static int get_uint32_equal(QEMUFile *f, void *pv, size_t size)
>      if (*v == v2) {
>          return 0;
>      }
> +    error_report("%" PRIx32 " != %" PRIx32, *v, v2);
>      return -EINVAL;
>  }
>  
> @@ -727,6 +732,7 @@ static int get_uint64_equal(QEMUFile *f, void *pv, size_t size)
>      if (*v == v2) {
>          return 0;
>      }
> +    error_report("%" PRIx64 " != %" PRIx64, *v, v2);
>      return -EINVAL;
>  }
>  
> @@ -748,6 +754,7 @@ static int get_uint8_equal(QEMUFile *f, void *pv, size_t size)
>      if (*v == v2) {
>          return 0;
>      }
> +    error_report("%x != %x", *v, v2);
>      return -EINVAL;
>  }
>  
> @@ -769,6 +776,7 @@ static int get_uint16_equal(QEMUFile *f, void *pv, size_t size)
>      if (*v == v2) {
>          return 0;
>      }
> +    error_report("%x != %x", *v, v2);
>      return -EINVAL;
>  }

Applied, thanks.

Re: [Qemu-devel] [PATCH 0/2] Add error reporting in migration

[John Snow]

On 09/27/2016 02:56 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> At the moment if you use a VMSTATE_*_EQUAL macro and the value
> doesn't match you just get an error about the section that failed
>
> e.g.
>   qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>   qemu-system-ppc64: load of migration failed: Invalid argument
>
> with this pair you get the field and the mismatched values.
> e.g.
>   qemu-system-ppc64: 8000600FE1FF7AE1 != 8000600FE1FF3A21
>   qemu-system-ppc64: Failed to load cpu:env.insns_flags
>   qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>   qemu-system-ppc64: load of migration failed: Invalid argument
>
> which is much more likely to point you at the culprit.
>
> (Broken out from a larger vmstatification series, the only change since
> then is the values are printed in hex except for the le case).
>
> Dave
>
> Dr. David Alan Gilbert (2):
>   migration: report an error giving the failed field
>   migration: Report values for comparisons
>
>  migration/vmstate.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

I see this as a strict improvement; though I don't know if there will be 
complaints about printing error messages instead of adding pathways for 
the Error object.

Meh. Existing errors here simply use error_report anyway, so:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] Add error reporting in migration

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/27/2016 02:56 PM, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> At the moment if you use a VMSTATE_*_EQUAL macro and the value
>> doesn't match you just get an error about the section that failed
>>
>> e.g.
>>   qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>>   qemu-system-ppc64: load of migration failed: Invalid argument
>>
>> with this pair you get the field and the mismatched values.
>> e.g.
>>   qemu-system-ppc64: 8000600FE1FF7AE1 != 8000600FE1FF3A21
>>   qemu-system-ppc64: Failed to load cpu:env.insns_flags
>>   qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>>   qemu-system-ppc64: load of migration failed: Invalid argument
>>
>> which is much more likely to point you at the culprit.
>>
>> (Broken out from a larger vmstatification series, the only change since
>> then is the values are printed in hex except for the le case).
>>
>> Dave
>>
>> Dr. David Alan Gilbert (2):
>>   migration: report an error giving the failed field
>>   migration: Report values for comparisons
>>
>>  migration/vmstate.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>
> I see this as a strict improvement; though I don't know if there will
> be complaints about printing error messages instead of adding pathways
> for the Error object.

Drive-by comment without having studied the patch: if a function can run
within a function that takes an Error * parameter, then error_report()
is probably wrong.  It's less wrong than not reporting the error at all,
though.  It's okay to point this out to the poster.  Perhaps he's
willing to go all the way, once aware.  Sometimes, going all the way is
more work than the poster can give.  Badgering him for it would be
ungrateful.  A small incremental improvement is still better than
nothing.  Recording the imperfect nature of the change in commit message
or comments may be in order then.

> Meh. Existing errors here simply use error_report anyway, so:
>
> Reviewed-by: John Snow <jsnow@redhat.com>