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

2015-12-18 Thread Markus Armbruster 
Eric Blake  writes:

> On 12/17/2015 12:00 PM, Markus Armbruster wrote:
>
>> @@ -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?
>>>
>>> Maybe:
>>>
>>> The resulting message should be a single phrase, with no newline or
>>> trailing punctuation.
>> 
>> What about ending the message with an exclamation mark?
>
> Very few current users do that! An exclamation mark is still trailing
> punctuation! And I don't like shouting at users!
>
> :)

Okay, okay, I applied it %-)

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

2015-12-17 Thread Markus Armbruster 
While there, tighten its assertion.

Signed-off-by: Markus Armbruster 
---
 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(, "situation normal, all fouled up");
  *
+ * Create an error and add additional explanation:
+ * error_setg(, "invalid quark");
+ * error_append_hint(, "Valid quarks are up, down, strange, "
+ *   "charm, top, bottom\n");
+ *
+ * Do *not* contract this to
+ * error_setg(, "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.
  */
 #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 _fatal nor _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 != _abort);
+assert(err && errp != _abort && errp != _fatal);
 
 if (!err->hint) {
 err->hint = g_string_new(NULL);
-- 
2.4.3

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

2015-12-17 Thread Eric Blake 
On 12/17/2015 11:04 AM, Markus Armbruster wrote:

>>> + * Create an error and add additional explanation:
>>> + * error_setg(, "invalid quark");
>>> + * error_append_hint(, "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(, "invalid quark\n"
>>> + *"Valid quarks are up, down, strange, charm, top, 
>>> bottom");

Of course, if you put a trailing dot above, you'd also want it here in
the 'don't contract' section.


>>> @@ -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?

Maybe:

The resulting message should be a single phrase, with no newline or
trailing punctuation.

> 
>> Should we also mention no need for leading 'error: ' prefix?
> 
> Unlike the other anti-patterns, this one doesn't seem frequent in new
> code.

Fair enough.


>>>  /**
>>>   * 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 _fatal nor _abort.
>>
>> As a native speaker, 'not' sounds better than 'neither' in that
>> sentence.  But I think both choices are grammatically correct.
> 
> You mean "not _abort or _abort"?

Yes, that works:

@errp may be NULL, but not _fatal or _abort.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2015-12-17 Thread Eric Blake 
On 12/17/2015 09:49 AM, Markus Armbruster wrote:
> While there, tighten its assertion.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  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(, "situation normal, all fouled up");
>   *
> + * Create an error and add additional explanation:
> + * error_setg(, "invalid quark");
> + * error_append_hint(, "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.

> + *
> + * Do *not* contract this to
> + * error_setg(, "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?

Should we also mention no need for leading 'error: ' prefix?

>   */
>  #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 _fatal nor _abort.

As a native speaker, 'not' sounds better than 'neither' in that
sentence.  But I think both choices are grammatically correct.

> + * 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 != _abort);
> +assert(err && errp != _abort && errp != _fatal);

Otherwise looks reasonable.

>  
>  if (!err->hint) {
>  err->hint = g_string_new(NULL);
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2015-12-17 Thread Markus Armbruster 
Eric Blake  writes:

> On 12/17/2015 09:49 AM, Markus Armbruster wrote:
>> While there, tighten its assertion.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  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(, "situation normal, all fouled up");
>>   *
>> + * Create an error and add additional explanation:
>> + * error_setg(, "invalid quark");
>> + * error_append_hint(, "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(, "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 _fatal nor _abort.
>
> As a native speaker, 'not' sounds better than 'neither' in that
> sentence.  But I think both choices are grammatically correct.

You mean "not _abort or _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 != _abort);
>> +assert(err && errp != _abort && errp != _fatal);
>
> Otherwise looks reasonable.
>
>>  
>>  if (!err->hint) {
>>  err->hint = g_string_new(NULL);
>> 

Thanks!

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

2015-12-17 Thread Markus Armbruster 
Eric Blake  writes:

> On 12/17/2015 11:04 AM, Markus Armbruster wrote:
>
 + * Create an error and add additional explanation:
 + * error_setg(, "invalid quark");
 + * error_append_hint(, "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(, "invalid quark\n"
 + * "Valid quarks are up, down, strange, charm, top, bottom");
>
> Of course, if you put a trailing dot above, you'd also want it here in
> the 'don't contract' section.

Yes.

 @@ -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?
>
> Maybe:
>
> The resulting message should be a single phrase, with no newline or
> trailing punctuation.

What about ending the message with an exclamation mark?

>>> Should we also mention no need for leading 'error: ' prefix?
>> 
>> Unlike the other anti-patterns, this one doesn't seem frequent in new
>> code.
>
> Fair enough.
>
>
  /**
   * 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 _fatal nor _abort.
>>>
>>> As a native speaker, 'not' sounds better than 'neither' in that
>>> sentence.  But I think both choices are grammatically correct.
>> 
>> You mean "not _abort or _abort"?
>
> Yes, that works:
>
> @errp may be NULL, but not _fatal or _abort.

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

2015-12-17 Thread Eric Blake 
On 12/17/2015 12:00 PM, Markus Armbruster wrote:

> @@ -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?
>>
>> Maybe:
>>
>> The resulting message should be a single phrase, with no newline or
>> trailing punctuation.
> 
> What about ending the message with an exclamation mark?

Very few current users do that! An exclamation mark is still trailing
punctuation! And I don't like shouting at users!

:)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature