Eric Blake <ebl...@redhat.com> writes: > On 12/17/2015 09:49 AM, Markus Armbruster wrote: >> While there, tighten its assertion. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> include/qapi/error.h | 19 +++++++++++++++++-- >> util/error.c | 2 +- >> 2 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index b2362a5..048d947 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -18,6 +18,15 @@ >> * 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 we want to allow/encourage a trailing dot in the hint? I'm fine if > we don't - but once we document its usage, we should probably then be > consistent with what we document; qemu-option.c used a trailing dot, > qdev-monitor.c did not.
I'll fix this example. >> + * >> + * Do *not* contract this to >> + * error_setg(&err, "invalid quark\n" >> + * "Valid quarks are up, down, strange, charm, top, bottom"); >> + * >> * Report an error to stderr: >> * error_report_err(err); >> * This frees the error object. >> @@ -26,6 +35,7 @@ >> * const char *msg = error_get_pretty(err); >> * do with msg what needs to be done... >> * error_free(err); >> + * Note that this loses hints added with error_append_hint(). >> * >> * Handle an error without reporting it (just for completeness): >> * error_free(err); >> @@ -128,6 +138,7 @@ ErrorClass error_get_class(const Error *err); >> * If @errp is anything else, *@errp must be NULL. >> * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its >> * human-readable error message is made from printf-style @fmt, ... >> + * The resulting message should not contain newlines. > > Should we also discourage trailing punctuation? Yes. How to best phrase it? > Should we also mention no need for leading 'error: ' prefix? Unlike the other anti-patterns, this one doesn't seem frequent in new code. >> */ >> #define error_setg(errp, fmt, ...) \ >> error_setg_internal((errp), __FILE__, __LINE__, __func__, \ >> @@ -184,7 +195,11 @@ void error_propagate(Error **dst_errp, Error >> *local_err); >> >> /** >> * Append a printf-style human-readable explanation to an existing error. >> - * May be called multiple times, and safe if @errp is NULL. >> + * @errp may be NULL, but neither &error_fatal nor &error_abort. > > As a native speaker, 'not' sounds better than 'neither' in that > sentence. But I think both choices are grammatically correct. You mean "not &error_abort 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, ...) >> GCC_FMT_ATTR(2, 3); >> @@ -218,7 +233,7 @@ void error_free_or_abort(Error **errp); >> /* >> * Convenience function to error_report() and free @err. >> */ >> -void error_report_err(Error *); >> +void error_report_err(Error *err); >> >> /* >> * Just like error_setg(), except you get to specify the error class. >> diff --git a/util/error.c b/util/error.c >> index 9b27c45..ebfb74b 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -132,7 +132,7 @@ void error_append_hint(Error **errp, const char *fmt, >> ...) >> return; >> } >> err = *errp; >> - assert(err && errp != &error_abort); >> + assert(err && errp != &error_abort && errp != &error_fatal); > > Otherwise looks reasonable. > >> >> if (!err->hint) { >> err->hint = g_string_new(NULL); >> Thanks!