Halil Pasic <pa...@linux.vnet.ibm.com> writes: > On 07/03/2017 03:52 PM, Markus Armbruster wrote: >> Halil Pasic <pa...@linux.vnet.ibm.com> writes: >> >>> On 06/30/2017 04:54 PM, Eric Blake wrote: >>>> On 06/30/2017 09:41 AM, Halil Pasic wrote: >>>>>>> 'This' basically boils down to the question and >>>>>>> 'Why aren't hints reported in QMP context?' >>>>>> >>>>>> QMP is supposed to be machine-parseable. Hints are supposed to be >>>>>> human-readable. If you have a machine managing the monitor, the hint >>>>>> adds nothing but bandwidth consumption, because machine should not be >>>>>> parsing the human portion of the error message in the first place (as it >>>>>> is, libvirt already just logs the human-readable portion of a message, >>>>>> and bases its actions solely on the machine-stable portions of an error >>>>>> reply: namely, whether an error was sent at all, and occasionally, what >>>>>> error class was used for that error - there's no guarantee a human will >>>>>> be reading the log, though). [...] >>>>> From prior experiences I'm more used to think about error messages as >>>>> something meant for human consumption, and expressing things expected to >>>>> be relevant for some kind of client code in a different way (optimized >>>>> for machine consumption). >>>>> >>>>> If however the error message ain't part of the machine relevant portion, >>>>> then the same argument applies as to the 'hint', and I don't see the >>>>> reason for handling hints differently. Do you agree with my >>>>> argumentation? >>>> >>>> Indeed, it may not hurt to start passing the hints over the wire (errors >>>> would then consume more bandwidth, but errors are not the hot path). >>>> And I'm not necessarily opposed to that change, so much as trying to >>>> document why it is not currently the case. At the same time, I probably >>>> won't be the one writing a path to populate the hint information into >>>> the QMP error, as I don't have any reason to use the hint when >>>> controlling libvirt (except maybe for logging, but there, the hint is >>>> not going to help the end user, because it's not the end-user's fault >>>> that libvirt used the API wrong to get a hint in the first place). >>> >>> For me both human readable things make sense only for error reporting >>> (effectively logging). Error.msg should IMHO be different, than Error.hint. >>> The existence of an error should be indicated by the Error object. >> >> Consider this one from qemu-option.c: >> >> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, >> "a non-negative number below 2^64"); >> error_append_hint(errp, "Optional suffix k, M, G, T, P or E means" >> " kilo-, mega-, giga-, tera-, peta-\n" >> "and exabytes, respectively.\n"); >> >> The hint is helpful for a human command line or HMP user. It's actively >> misleading in QMP. > > I agree. > >> Totally fine, it's how the "hint" feature is meant >> to be used. >> > > Was not aware. > >> If we have errors that can't be adequately explained in a single error >> message, we may need a way to add more explanation. error_append_hint() >> isn't. >> > > Was not aware. Using hint in this very situation was suggested by Connie, > and I assumed she is long enough with the project to know... > > In fact looking at include/qapi/error.h: > """ > /* > * Error reporting system loosely patterned after Glib's GError. > * > * Create an error: > * error_setg(&err, "situation normal, all fouled up"); > * > * Create an error and add additional explanation: > * error_setg(&err, "invalid quark"); > * error_append_hint(&err, "Valid quarks are up, down, strange, " > * "charm, top, bottom.\n"); > * > * Do *not* contract this to > * error_setg(&err, "invalid quark\n" > * "Valid quarks are up, down, strange, charm, top, bottom."); > """ > > my understanding was and is still the exact opposite of what you say: > error_append_hint is for adding more explanation. > > Furthermore > """ > /* > * Append a printf-style human-readable explanation to an existing error. > * @errp may be NULL, but not &error_fatal or &error_abort. > * Trivially the case if you call it only after error_setg() or > * error_propagate(). > * May be called multiple times. The resulting hint should end with a > * newline. > */ > void error_append_hint(Error **errp, const char *fmt, ...) > """ > > Assuming that error_append_hint() isn't for adding more explanation, > IMHO the doc does not adequately explain what it is for.
You're right, it doesn't. > I have also failed to find any hint in qapi/error.h which is AFAIU > documenting the error api about this human-readable explanation > appended to an existing error by error_append_hint() is to be discarded > if the error is reported in QMP context. > > Am I reading the api doc incorrectly, or did the documentation and > de-facto api diverge (behavior)? I added documentation after I inherited this subsystem, in response to recurring questions on proper use of the interface. I failed to fully capture the hint feature's intent. I'll post a patch. >>>>>> If something absolutely must be reported, then it is not a hint, and >>>>>> shouldn't be using the hint mechanism. >> >> Exactly. >> > > Perfectly fine with me provided the apidoc tells me clearly what the hint is > for, and what it is not for. > >>>>> I find it hard to formulate criteria for 'must be reported'. I'm afraid >>>>> this is backwards logic: since the hint may not be reported everything >>>>> that needs to be reported is not a hint. This is a valid approach of >>>>> course, but then I think some modifications to the comments in error.h >>>>> would not hurt. And maybe something with verbose would be more >>>>> expressive name. >>>>> >>>>> I hope all this makes some sense and ain't pure waste of time... >>>> >>>> No, it never hurts to question whether the design is optimal, and it's >>>> better to question first to know whether it is even worth patching >>>> things to behave differently, rather than spending time patching it only >>>> to have a maintainer clarify that the patch can't be accepted because of >>>> some design constraint. So I still hope Markus will chime in. >>>> >>> >>> For this patch I went with Dave's proposal so I have no acute interest >>> in changing this. >>> >>> Conceptually, for me it really boils down to the question: Is it reasonable >>> to assume that we are interested in what went wrong (error message)? >>> >>> If yes, we are good as is. If no, we should not drop hint in QMP context. >>> >>> Thanks for your time. I think we provided Markus with enough input to >>> make his call :). >> >> I had a quick peek at the patch that triggered this discussion. What >> problem are you trying to solve? According to your cover letter, it's >> "to specify a hint for the case a vmstate equal assertion". How is >> nicer assertion failures related to QMP? Am I confused? > > > The problem is solved by d2164ad ("vmstate: error hint for failed equal > checks", 2017-06-23). The way the commit uses error_report() and error_printf() looks good to me. > The assertions ain't assertions in sense of the C programming > language. Maybe calling these 'checks' instead of 'assertions' in the > cover letter (like in the subject) would have been better. If one of > these 'assertions' fail qemu is supposed to abort the initiated load > (migration), state the reason, and terminate normally. In this sense these > 'assertions' are similar to the assertions in our unit tests (those fail > a test, and similarly to these do not terminate the program). > > The problem I was trying to solve is that the message generated by these > checks looked something like "5 != 4" which is OK if the check is never > supposed to fail, but not satisfactory for something we have to live > with. > > Sorry for the confusion. No problem :)