* [Qemu-devel] [PATCH 0/2] Add error reporting in migration @ 2016-09-27 18:56 Dr. David Alan Gilbert (git) 2016-09-27 18:56 ` [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field Dr. David Alan Gilbert (git) ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Dr. David Alan Gilbert (git) @ 2016-09-27 18:56 UTC (permalink / raw) To: qemu-devel, amit.shah, quintela 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field 2016-09-27 18:56 [Qemu-devel] [PATCH 0/2] Add error reporting in migration Dr. David Alan Gilbert (git) @ 2016-09-27 18:56 ` Dr. David Alan Gilbert (git) 2016-10-05 8:49 ` Juan Quintela 2016-09-27 18:56 ` [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons Dr. David Alan Gilbert (git) 2016-09-27 21:22 ` [Qemu-devel] [PATCH 0/2] Add error reporting in migration John Snow 2 siblings, 1 reply; 7+ messages in thread From: Dr. David Alan Gilbert (git) @ 2016-09-27 18:56 UTC (permalink / raw) To: qemu-devel, amit.shah, quintela 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field 2016-09-27 18:56 ` [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field Dr. David Alan Gilbert (git) @ 2016-10-05 8:49 ` Juan Quintela 0 siblings, 0 replies; 7+ messages in thread From: Juan Quintela @ 2016-10-05 8:49 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, amit.shah "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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons 2016-09-27 18:56 [Qemu-devel] [PATCH 0/2] Add error reporting in migration Dr. David Alan Gilbert (git) 2016-09-27 18:56 ` [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field Dr. David Alan Gilbert (git) @ 2016-09-27 18:56 ` Dr. David Alan Gilbert (git) 2016-10-05 8:49 ` Juan Quintela 2016-09-27 21:22 ` [Qemu-devel] [PATCH 0/2] Add error reporting in migration John Snow 2 siblings, 1 reply; 7+ messages in thread From: Dr. David Alan Gilbert (git) @ 2016-09-27 18:56 UTC (permalink / raw) To: qemu-devel, amit.shah, quintela 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons 2016-09-27 18:56 ` [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons Dr. David Alan Gilbert (git) @ 2016-10-05 8:49 ` Juan Quintela 0 siblings, 0 replies; 7+ messages in thread From: Juan Quintela @ 2016-10-05 8:49 UTC (permalink / raw) To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, amit.shah "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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Add error reporting in migration 2016-09-27 18:56 [Qemu-devel] [PATCH 0/2] Add error reporting in migration Dr. David Alan Gilbert (git) 2016-09-27 18:56 ` [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field Dr. David Alan Gilbert (git) 2016-09-27 18:56 ` [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons Dr. David Alan Gilbert (git) @ 2016-09-27 21:22 ` John Snow 2016-09-28 2:16 ` Markus Armbruster 2 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2016-09-27 21:22 UTC (permalink / raw) To: Dr. David Alan Gilbert (git), qemu-devel, amit.shah, quintela 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Add error reporting in migration 2016-09-27 21:22 ` [Qemu-devel] [PATCH 0/2] Add error reporting in migration John Snow @ 2016-09-28 2:16 ` Markus Armbruster 0 siblings, 0 replies; 7+ messages in thread From: Markus Armbruster @ 2016-09-28 2:16 UTC (permalink / raw) To: John Snow; +Cc: Dr. David Alan Gilbert (git), qemu-devel, amit.shah, quintela 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-10-05 8:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-27 18:56 [Qemu-devel] [PATCH 0/2] Add error reporting in migration Dr. David Alan Gilbert (git) 2016-09-27 18:56 ` [Qemu-devel] [PATCH 1/2] migration: report an error giving the failed field Dr. David Alan Gilbert (git) 2016-10-05 8:49 ` Juan Quintela 2016-09-27 18:56 ` [Qemu-devel] [PATCH 2/2] migration: Report values for comparisons Dr. David Alan Gilbert (git) 2016-10-05 8:49 ` Juan Quintela 2016-09-27 21:22 ` [Qemu-devel] [PATCH 0/2] Add error reporting in migration John Snow 2016-09-28 2:16 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.