Re: [Qemu-devel] [PATCH v2 05/23] error: Improve documentation around error_append_hint()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> While there, tighten its assertion.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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, address@hidden 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!