| Message ID | 20170606165510.33057-3-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL > so the hint states the assertion probably failed due to a bug. Introduce > _EQUAL_HINT for specifying a context specific hint. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> I'd prefer not to print 'Bug!?' by default - they already get the message telling them something didn't match and the migration fails. There are none-bug ways of this happening, e.g. a user starting a VM on the source and destination with different configs. (I also worry we have a lot f macros for each size; EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for that) Dave > --- > Keeping this separate for now because we may want something different > here. E.g. no new macros and adding an extra NULL parameter for all > pre-existing _EQUAL usages. > --- > include/migration/vmstate.h | 54 ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index d90d9b12ca..ed1e1fd047 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -302,6 +302,18 @@ extern const VMStateInfo vmstate_info_qtailq; > .offset = vmstate_offset_value(_state, _field, _type), \ > } > > +#define VMSTATE_SINGLE_FULL(_field, _state, _test, _version, _info, \ > + _type, _err_hint) { \ > + .name = (stringify(_field)), \ > + .err_hint = (_err_hint), \ > + .version_id = (_version), \ > + .field_exists = (_test), \ > + .size = sizeof(_type), \ > + .info = &(_info), \ > + .flags = VMS_SINGLE, \ > + .offset = vmstate_offset_value(_state, _field, _type), \ > +} > + > /* Validate state using a boolean predicate. */ > #define VMSTATE_VALIDATE(_name, _test) { \ > .name = (_name), \ > @@ -808,30 +820,60 @@ extern const VMStateInfo vmstate_info_qtailq; > #define VMSTATE_UINT64(_f, _s) \ > VMSTATE_UINT64_V(_f, _s, 0) > > +#define VMSTATE_UINT8_EQUAL_HINT(_f, _s, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_uint8_equal, \ > + uint8_t, _err_hint) > + > #define VMSTATE_UINT8_EQUAL(_f, _s) \ > - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint8_equal, uint8_t) > + VMSTATE_UINT8_EQUAL_HINT(_f, _s, "Bug!?") > + > +#define VMSTATE_UINT16_EQUAL_HINT(_f, _s, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_uint16_equal, \ > + uint16_t, _err_hint) > > #define VMSTATE_UINT16_EQUAL(_f, _s) \ > - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint16_equal, uint16_t) > + VMSTATE_UINT16_EQUAL_HINT(_f, _s, "Bug!?") > + > +#define VMSTATE_UINT16_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_uint16_equal, \ > + uint16_t, _err_hint) > > #define VMSTATE_UINT16_EQUAL_V(_f, _s, _v) \ > - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16_equal, uint16_t) > + VMSTATE_UINT16_EQUAL_V_HINT(_f, _s, _v, "Bug!?") > + > +#define VMSTATE_INT32_EQUAL_HINT(_f, _s, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_int32_equal, \ > + int32_t, _err_hint) > > #define VMSTATE_INT32_EQUAL(_f, _s) \ > - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_equal, int32_t) > + VMSTATE_INT32_EQUAL_HINT(_f, _s, "Bug!?") > + > +#define VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_uint32_equal, \ > + uint32_t, _err_hint) > > #define VMSTATE_UINT32_EQUAL_V(_f, _s, _v) \ > - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32_equal, uint32_t) > + VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, _v, "Bug!?") > > #define VMSTATE_UINT32_EQUAL(_f, _s) \ > VMSTATE_UINT32_EQUAL_V(_f, _s, 0) > > +#define VMSTATE_UINT32_EQUAL_HINT(_f, _s, _err_hint) \ > + VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, 0, _err_hint) > + > +#define VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ > + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_int64_equal, \ > + uint64_t, _err_hint) > + > #define VMSTATE_UINT64_EQUAL_V(_f, _s, _v) \ > - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64_equal, uint64_t) > + VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, _v, "Bug!?") > > #define VMSTATE_UINT64_EQUAL(_f, _s) \ > VMSTATE_UINT64_EQUAL_V(_f, _s, 0) > > +#define VMSTATE_UINT64_EQUAL_HINT(_f, _s, _err_hint) \ > + VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, 0, _err_hint) > + > #define VMSTATE_INT32_POSITIVE_LE(_f, _s) \ > VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t) > > -- > 2.11.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL >> so the hint states the assertion probably failed due to a bug. Introduce >> _EQUAL_HINT for specifying a context specific hint. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > I'd prefer not to print 'Bug!?' by default - they already get the > message telling them something didn't match and the migration fails. > There are none-bug ways of this happening, e.g. a user starting a VM on > the source and destination with different configs. I admit, my objective with 'Bug!?' was to provoke. My train of thought is to encourage the programmer to think about and document the circumstances under which such an assertion is supposed to fail (and against which it is supposed to guard). I do not know how skillful are our users but a 4 != 5 then maybe a name of a vmstate field is probably quite scary and not very revealing. I doubt a non qemu developer can use it for something else that reporting a bug. Consequently if there are non-bug ways one can use the hint and state them. Your example with the misconfigured target, by the way, is IMHO also be due to a bug of the management software IMHO. To sum it up: IMHO the message provided by a failing _EQUAL is to ugly and Qemuspeak to be presented to an user-user in non-bug cases. Agree? Disagree? > > (I also worry we have a lot f macros for each size; > EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for > that) > If we are going to drop the default hint ('Bug?!' or whatever) then I think we could just add an extra NULL hint to each existing _EQUAL usage, re-purpose EQUAL, and forget about introducing new _HINT macros. What to you think? Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL > >> so the hint states the assertion probably failed due to a bug. Introduce > >> _EQUAL_HINT for specifying a context specific hint. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > > > I'd prefer not to print 'Bug!?' by default - they already get the > > message telling them something didn't match and the migration fails. > > There are none-bug ways of this happening, e.g. a user starting a VM on > > the source and destination with different configs. > > I admit, my objective with 'Bug!?' was to provoke. My train of thought is > to encourage the programmer to think about and document the circumstances > under which such an assertion is supposed to fail (and against which it > is supposed to guard). > > I do not know how skillful are our users but a 4 != 5 then maybe a name > of a vmstate field is probably quite scary and not very revealing. I doubt > a non qemu developer can use it for something else that reporting a bug. > > Consequently if there are non-bug ways one can use the hint and state them. > Your example with the misconfigured target, by the way, is IMHO also be due > to a bug of the management software IMHO. > > To sum it up: IMHO the message provided by a failing _EQUAL is to ugly > and Qemuspeak to be presented to an user-user in non-bug cases. Agree? > Disagree? Disagree. I don't mind giving field names etc; they make it easy for us as developers to track down what's happening, but also sometimes they help endusers work around a prolem or see where the problem is; of course that varies depending on the field name, but some of our names are reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in vmmouse.c). They're also pretty good if two end users hit the same problem they can see the same error message in a bug report. We often have customer-facing support people look at logs before they get as far as us developers; if we have bugs that are 'if it's a failing BLAH device complaining about the BAR field' then this fixes it, then that helps them find workarounds/fixes quickly even if they don't understand what the BAR field is. > > > > > (I also worry we have a lot f macros for each size; > > EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for > > that) > > > > If we are going to drop the default hint ('Bug?!' or whatever) then > I think we could just add an extra NULL hint to each existing _EQUAL > usage, re-purpose EQUAL, and forget about introducing new _HINT macros. > > What to you think? Yes, that would be a lot simpler; and there aren't that many VMSTATE*EQUAL* macros in use. Dave > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/07/2017 02:01 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL >>>> so the hint states the assertion probably failed due to a bug. Introduce >>>> _EQUAL_HINT for specifying a context specific hint. >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> >>> I'd prefer not to print 'Bug!?' by default - they already get the >>> message telling them something didn't match and the migration fails. >>> There are none-bug ways of this happening, e.g. a user starting a VM on >>> the source and destination with different configs. >> >> I admit, my objective with 'Bug!?' was to provoke. My train of thought is >> to encourage the programmer to think about and document the circumstances >> under which such an assertion is supposed to fail (and against which it >> is supposed to guard). >> >> I do not know how skillful are our users but a 4 != 5 then maybe a name >> of a vmstate field is probably quite scary and not very revealing. I doubt >> a non qemu developer can use it for something else that reporting a bug. >> >> Consequently if there are non-bug ways one can use the hint and state them. >> Your example with the misconfigured target, by the way, is IMHO also be due >> to a bug of the management software IMHO. >> >> To sum it up: IMHO the message provided by a failing _EQUAL is to ugly >> and Qemuspeak to be presented to an user-user in non-bug cases. Agree? >> Disagree? > > Disagree. > > I don't mind giving field names etc; they make it easy for us as > developers to track down what's happening, but also sometimes they help > endusers work around a prolem or see where the problem is; of course > that varies depending on the field name, but some of our names are > reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in > vmmouse.c). They're also pretty good if two end users hit the same > problem they can see the same error message in a bug report. > > We often have customer-facing support people look at logs before they > get as far as us developers; if we have bugs that are > 'if it's a failing BLAH device complaining about the BAR field' > then this fixes it, then that helps them find workarounds/fixes quickly > even if they don't understand what the BAR field is. > You seem to forget, that I'm not proposing omitting this information, but extending it with something civilized so one can distinguish between an assert failed should have never happened situation an a as good as reasonable error handling for an expected error scenario. IMHO the current EQUAL looks more like the former (assert) and less like the later (error reporting for an expected error scenario). Agree? Dissagree? Having a field name is great! That's beyond discussion. I see, my 'sum it up' above was a bit unfortunate: it sounds like I'm against the inclusion of technical info and not against a lack of non technical info. Sorry for that! >> >>> >>> (I also worry we have a lot f macros for each size; >>> EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for >>> that) >>> >> >> If we are going to drop the default hint ('Bug?!' or whatever) then >> I think we could just add an extra NULL hint to each existing _EQUAL >> usage, re-purpose EQUAL, and forget about introducing new _HINT macros. >> >> What to you think? > > Yes, that would be a lot simpler; and there aren't that many > VMSTATE*EQUAL* macros in use. > I still have not given up on the discussion above. Will do depending on the outcome. Regards, Halil
Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL > so the hint states the assertion probably failed due to a bug. Introduce > _EQUAL_HINT for specifying a context specific hint. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > Keeping this separate for now because we may want something different > here. E.g. no new macros and adding an extra NULL parameter for all > pre-existing _EQUAL usages. I think that the best thing is to just add the HINT always. I checked and there are only 25 uses of VMSTATE_*_EQUAL. I agree with dave that adding a NULL there, and make they work is a better strategy. We are adding 8 new macros so we don't have to change 25 callers? Later, Juan.
On 06/07/2017 06:35 PM, Juan Quintela wrote: > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: >> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL >> so the hint states the assertion probably failed due to a bug. Introduce >> _EQUAL_HINT for specifying a context specific hint. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> Keeping this separate for now because we may want something different >> here. E.g. no new macros and adding an extra NULL parameter for all >> pre-existing _EQUAL usages. > > > I think that the best thing is to just add the HINT always. I checked > and there are only 25 uses of VMSTATE_*_EQUAL. I agree with dave that > adding a NULL there, and make they work is a better strategy. > Will do that! > We are adding 8 new macros so we don't have to change 25 callers? > It was more the fear of needlessly complicating the interface that made me do this version first (I assumed typical usage is without hint with the purpose of catching bugs). I also wanted to avoid bothering a lot's of people in the first round (I assume the usages are spread over files owned by various maintainers). Thanks for your review! Regards, Halil
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 06/07/2017 02:01 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: > >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >>>> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL > >>>> so the hint states the assertion probably failed due to a bug. Introduce > >>>> _EQUAL_HINT for specifying a context specific hint. > >>>> > >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>> > >>> I'd prefer not to print 'Bug!?' by default - they already get the > >>> message telling them something didn't match and the migration fails. > >>> There are none-bug ways of this happening, e.g. a user starting a VM on > >>> the source and destination with different configs. > >> > >> I admit, my objective with 'Bug!?' was to provoke. My train of thought is > >> to encourage the programmer to think about and document the circumstances > >> under which such an assertion is supposed to fail (and against which it > >> is supposed to guard). > >> > >> I do not know how skillful are our users but a 4 != 5 then maybe a name > >> of a vmstate field is probably quite scary and not very revealing. I doubt > >> a non qemu developer can use it for something else that reporting a bug. > >> > >> Consequently if there are non-bug ways one can use the hint and state them. > >> Your example with the misconfigured target, by the way, is IMHO also be due > >> to a bug of the management software IMHO. > >> > >> To sum it up: IMHO the message provided by a failing _EQUAL is to ugly > >> and Qemuspeak to be presented to an user-user in non-bug cases. Agree? > >> Disagree? > > > > Disagree. > > > > I don't mind giving field names etc; they make it easy for us as > > developers to track down what's happening, but also sometimes they help > > endusers work around a prolem or see where the problem is; of course > > that varies depending on the field name, but some of our names are > > reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in > > vmmouse.c). They're also pretty good if two end users hit the same > > problem they can see the same error message in a bug report. > > > > We often have customer-facing support people look at logs before they > > get as far as us developers; if we have bugs that are > > 'if it's a failing BLAH device complaining about the BAR field' > > then this fixes it, then that helps them find workarounds/fixes quickly > > even if they don't understand what the BAR field is. > > > > You seem to forget, that I'm not proposing omitting this information, > but extending it with something civilized so one can distinguish between > an assert failed should have never happened situation an a as good as > reasonable error handling for an expected error scenario. IMHO the current > EQUAL looks more like the former (assert) and less like the later (error > reporting for an expected error scenario). Agree? Dissagree? Yes, the current EQUAL is very terse; but we can't actually tell from the use which case it is; it'll all work nicely when people actually add the correct hint text in useful locations. > Having a field name is great! That's beyond discussion. > > I see, my 'sum it up' above was a bit unfortunate: it sounds like I'm > against the inclusion of technical info and not against a lack of non > technical info. Sorry for that! No, that's fine. Dave > >> > >>> > >>> (I also worry we have a lot f macros for each size; > >>> EQUAL, EQUAL_V, EQUAL_V_HINT but I don't know of a better answer for > >>> that) > >>> > >> > >> If we are going to drop the default hint ('Bug?!' or whatever) then > >> I think we could just add an extra NULL hint to each existing _EQUAL > >> usage, re-purpose EQUAL, and forget about introducing new _HINT macros. > >> > >> What to you think? > > > > Yes, that would be a lot simpler; and there aren't that many > > VMSTATE*EQUAL* macros in use. > > > > I still have not given up on the discussion above. Will do depending > on the outcome. > > Regards, > Halil > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/07/2017 07:10 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >> >> >> On 06/07/2017 02:01 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>> >>>> >>>> On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: >>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: >>>>>> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL >>>>>> so the hint states the assertion probably failed due to a bug. Introduce >>>>>> _EQUAL_HINT for specifying a context specific hint. >>>>>> >>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>>> >>>>> I'd prefer not to print 'Bug!?' by default - they already get the >>>>> message telling them something didn't match and the migration fails. >>>>> There are none-bug ways of this happening, e.g. a user starting a VM on >>>>> the source and destination with different configs. >>>> >>>> I admit, my objective with 'Bug!?' was to provoke. My train of thought is >>>> to encourage the programmer to think about and document the circumstances >>>> under which such an assertion is supposed to fail (and against which it >>>> is supposed to guard). >>>> >>>> I do not know how skillful are our users but a 4 != 5 then maybe a name >>>> of a vmstate field is probably quite scary and not very revealing. I doubt >>>> a non qemu developer can use it for something else that reporting a bug. >>>> >>>> Consequently if there are non-bug ways one can use the hint and state them. >>>> Your example with the misconfigured target, by the way, is IMHO also be due >>>> to a bug of the management software IMHO. >>>> >>>> To sum it up: IMHO the message provided by a failing _EQUAL is to ugly >>>> and Qemuspeak to be presented to an user-user in non-bug cases. Agree? >>>> Disagree? >>> >>> Disagree. >>> >>> I don't mind giving field names etc; they make it easy for us as >>> developers to track down what's happening, but also sometimes they help >>> endusers work around a prolem or see where the problem is; of course >>> that varies depending on the field name, but some of our names are >>> reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in >>> vmmouse.c). They're also pretty good if two end users hit the same >>> problem they can see the same error message in a bug report. >>> >>> We often have customer-facing support people look at logs before they >>> get as far as us developers; if we have bugs that are >>> 'if it's a failing BLAH device complaining about the BAR field' >>> then this fixes it, then that helps them find workarounds/fixes quickly >>> even if they don't understand what the BAR field is. >>> >> >> You seem to forget, that I'm not proposing omitting this information, >> but extending it with something civilized so one can distinguish between >> an assert failed should have never happened situation an a as good as >> reasonable error handling for an expected error scenario. IMHO the current >> EQUAL looks more like the former (assert) and less like the later (error >> reporting for an expected error scenario). Agree? Dissagree? > > Yes, the current EQUAL is very terse; but we can't actually tell from > the use which case it is; it'll all work nicely when people actually add > the correct hint text in useful locations. > You are right. Since Juan also requested the adding an extra param to the original macros variant I will go with that. I shied away form it in the first place because I did not want to bother the users of the macros without clarifying with the migration gurus how the new interface should look like. Thanks a lot! Regards, Halil >> Having a field name is great! That's beyond discussion. >> >> I see, my 'sum it up' above was a bit unfortunate: it sounds like I'm >> against the inclusion of technical info and not against a lack of non >> technical info. Sorry for that! > > No, that's fine. >
* Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > > > On 06/07/2017 07:10 PM, Dr. David Alan Gilbert wrote: > > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >> > >> > >> On 06/07/2017 02:01 PM, Dr. David Alan Gilbert wrote: > >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >>>> > >>>> > >>>> On 06/07/2017 01:07 PM, Dr. David Alan Gilbert wrote: > >>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote: > >>>>>> Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL > >>>>>> so the hint states the assertion probably failed due to a bug. Introduce > >>>>>> _EQUAL_HINT for specifying a context specific hint. > >>>>>> > >>>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>>>> > >>>>> I'd prefer not to print 'Bug!?' by default - they already get the > >>>>> message telling them something didn't match and the migration fails. > >>>>> There are none-bug ways of this happening, e.g. a user starting a VM on > >>>>> the source and destination with different configs. > >>>> > >>>> I admit, my objective with 'Bug!?' was to provoke. My train of thought is > >>>> to encourage the programmer to think about and document the circumstances > >>>> under which such an assertion is supposed to fail (and against which it > >>>> is supposed to guard). > >>>> > >>>> I do not know how skillful are our users but a 4 != 5 then maybe a name > >>>> of a vmstate field is probably quite scary and not very revealing. I doubt > >>>> a non qemu developer can use it for something else that reporting a bug. > >>>> > >>>> Consequently if there are non-bug ways one can use the hint and state them. > >>>> Your example with the misconfigured target, by the way, is IMHO also be due > >>>> to a bug of the management software IMHO. > >>>> > >>>> To sum it up: IMHO the message provided by a failing _EQUAL is to ugly > >>>> and Qemuspeak to be presented to an user-user in non-bug cases. Agree? > >>>> Disagree? > >>> > >>> Disagree. > >>> > >>> I don't mind giving field names etc; they make it easy for us as > >>> developers to track down what's happening, but also sometimes they help > >>> endusers work around a prolem or see where the problem is; of course > >>> that varies depending on the field name, but some of our names are > >>> reasonable (e.g. there's a VMSTATE_INT32_EQUAL on 'queue_size' in > >>> vmmouse.c). They're also pretty good if two end users hit the same > >>> problem they can see the same error message in a bug report. > >>> > >>> We often have customer-facing support people look at logs before they > >>> get as far as us developers; if we have bugs that are > >>> 'if it's a failing BLAH device complaining about the BAR field' > >>> then this fixes it, then that helps them find workarounds/fixes quickly > >>> even if they don't understand what the BAR field is. > >>> > >> > >> You seem to forget, that I'm not proposing omitting this information, > >> but extending it with something civilized so one can distinguish between > >> an assert failed should have never happened situation an a as good as > >> reasonable error handling for an expected error scenario. IMHO the current > >> EQUAL looks more like the former (assert) and less like the later (error > >> reporting for an expected error scenario). Agree? Dissagree? > > > > Yes, the current EQUAL is very terse; but we can't actually tell from > > the use which case it is; it'll all work nicely when people actually add > > the correct hint text in useful locations. > > > > You are right. > > Since Juan also requested the adding an extra param to the original > macros variant I will go with that. > > I shied away form it in the first place because I did not want to > bother the users of the macros without clarifying with the migration > gurus how the new interface should look like. If you just make the existing callers pass NULL then that's fine. Dave > Thanks a lot! > > Regards, > Halil > > >> Having a field name is great! That's beyond discussion. > >> > >> I see, my 'sum it up' above was a bit unfortunate: it sounds like I'm > >> against the inclusion of technical info and not against a lack of non > >> technical info. Sorry for that! > > > > No, that's fine. > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index d90d9b12ca..ed1e1fd047 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -302,6 +302,18 @@ extern const VMStateInfo vmstate_info_qtailq; .offset = vmstate_offset_value(_state, _field, _type), \ } +#define VMSTATE_SINGLE_FULL(_field, _state, _test, _version, _info, \ + _type, _err_hint) { \ + .name = (stringify(_field)), \ + .err_hint = (_err_hint), \ + .version_id = (_version), \ + .field_exists = (_test), \ + .size = sizeof(_type), \ + .info = &(_info), \ + .flags = VMS_SINGLE, \ + .offset = vmstate_offset_value(_state, _field, _type), \ +} + /* Validate state using a boolean predicate. */ #define VMSTATE_VALIDATE(_name, _test) { \ .name = (_name), \ @@ -808,30 +820,60 @@ extern const VMStateInfo vmstate_info_qtailq; #define VMSTATE_UINT64(_f, _s) \ VMSTATE_UINT64_V(_f, _s, 0) +#define VMSTATE_UINT8_EQUAL_HINT(_f, _s, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_uint8_equal, \ + uint8_t, _err_hint) + #define VMSTATE_UINT8_EQUAL(_f, _s) \ - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint8_equal, uint8_t) + VMSTATE_UINT8_EQUAL_HINT(_f, _s, "Bug!?") + +#define VMSTATE_UINT16_EQUAL_HINT(_f, _s, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_uint16_equal, \ + uint16_t, _err_hint) #define VMSTATE_UINT16_EQUAL(_f, _s) \ - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_uint16_equal, uint16_t) + VMSTATE_UINT16_EQUAL_HINT(_f, _s, "Bug!?") + +#define VMSTATE_UINT16_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_uint16_equal, \ + uint16_t, _err_hint) #define VMSTATE_UINT16_EQUAL_V(_f, _s, _v) \ - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint16_equal, uint16_t) + VMSTATE_UINT16_EQUAL_V_HINT(_f, _s, _v, "Bug!?") + +#define VMSTATE_INT32_EQUAL_HINT(_f, _s, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, 0, vmstate_info_int32_equal, \ + int32_t, _err_hint) #define VMSTATE_INT32_EQUAL(_f, _s) \ - VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_equal, int32_t) + VMSTATE_INT32_EQUAL_HINT(_f, _s, "Bug!?") + +#define VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_uint32_equal, \ + uint32_t, _err_hint) #define VMSTATE_UINT32_EQUAL_V(_f, _s, _v) \ - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32_equal, uint32_t) + VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, _v, "Bug!?") #define VMSTATE_UINT32_EQUAL(_f, _s) \ VMSTATE_UINT32_EQUAL_V(_f, _s, 0) +#define VMSTATE_UINT32_EQUAL_HINT(_f, _s, _err_hint) \ + VMSTATE_UINT32_EQUAL_V_HINT(_f, _s, 0, _err_hint) + +#define VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, _v, _err_hint) \ + VMSTATE_SINGLE_FULL(_f, _s, NULL, _v, vmstate_info_int64_equal, \ + uint64_t, _err_hint) + #define VMSTATE_UINT64_EQUAL_V(_f, _s, _v) \ - VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint64_equal, uint64_t) + VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, _v, "Bug!?") #define VMSTATE_UINT64_EQUAL(_f, _s) \ VMSTATE_UINT64_EQUAL_V(_f, _s, 0) +#define VMSTATE_UINT64_EQUAL_HINT(_f, _s, _err_hint) \ + VMSTATE_UINT64_EQUAL_V_HINT(_f, _s, 0, _err_hint) + #define VMSTATE_INT32_POSITIVE_LE(_f, _s) \ VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
Verbose error reporting for the _EQUAL family. Modify the standard _EQUAL so the hint states the assertion probably failed due to a bug. Introduce _EQUAL_HINT for specifying a context specific hint. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- Keeping this separate for now because we may want something different here. E.g. no new macros and adding an extra NULL parameter for all pre-existing _EQUAL usages. --- include/migration/vmstate.h | 54 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-)