[V8,3/8] util: add error_append()

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/qapi/error.h |    6 ++++++
 util/error.c         |   21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

On Fri, Jan 3, 2014 at 1:08 PM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  include/qapi/error.h |    6 ++++++
>  util/error.c         |   21 +++++++++++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 7d4c696..bcfb724 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -30,6 +30,12 @@ typedef struct Error Error;
>  void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>
>  /**
> + * Append message to err if err != NULL && *err != NULL. "\n" will be inserted
> + * after old message.
> + */
> +void error_append(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> +
> +/**
>   * Set an indirect pointer to an error given a ErrorClass value and a
>   * printf-style human message, followed by a strerror() string if
>   * @os_error is not zero.
> diff --git a/util/error.c b/util/error.c
> index 3ee362a..64bbb2d 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -46,6 +46,27 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      errno = saved_errno;
>  }
>
> +void error_append(Error **err, const char *fmt, ...)

errp

> +{
> +    va_list ap;
> +    gchar *msg, *msg_old;
> +
> +    if (!err || !*err) {

Should appending to an unset error really be a nop? You should just
set the error as normal in this case (error_set()?).

This avoids callers having to:

if (error_is_set(&err)) {
   error_append(&err);
} else {
   error_set(&err);
}

so they can just append if they want appending.

Generally speaking, appending to nothing should give you something.

Regards,
Peter

> +        return;
> +    }
> +
> +    va_start(ap, fmt);
> +    msg = g_strdup_vprintf(fmt, ap);
> +    va_end(ap);
> +
> +    msg_old = (*err)->msg;
> +    (*err)->msg = g_strdup_printf("%s\n%s", msg_old, msg);
> +
> +    g_free(msg);
> +    g_free(msg_old);
> +
> +}
> +
>  void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>                       const char *fmt, ...)
>  {
> --
> 1.7.1
>
>

于 2014/1/3 23:32, Peter Crosthwaite 写道:
> On Fri, Jan 3, 2014 at 1:08 PM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   include/qapi/error.h |    6 ++++++
>>   util/error.c         |   21 +++++++++++++++++++++
>>   2 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 7d4c696..bcfb724 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -30,6 +30,12 @@ typedef struct Error Error;
>>   void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>>
>>   /**
>> + * Append message to err if err != NULL && *err != NULL. "\n" will be inserted
>> + * after old message.
>> + */
>> +void error_append(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>> +
>> +/**
>>    * Set an indirect pointer to an error given a ErrorClass value and a
>>    * printf-style human message, followed by a strerror() string if
>>    * @os_error is not zero.
>> diff --git a/util/error.c b/util/error.c
>> index 3ee362a..64bbb2d 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -46,6 +46,27 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>       errno = saved_errno;
>>   }
>>
>> +void error_append(Error **err, const char *fmt, ...)
>
> errp
>
>> +{
>> +    va_list ap;
>> +    gchar *msg, *msg_old;
>> +
>> +    if (!err || !*err) {
>
> Should appending to an unset error really be a nop? You should just
> set the error as normal in this case (error_set()?).
>
> This avoids callers having to:
>
> if (error_is_set(&err)) {
>     error_append(&err);
> } else {
>     error_set(&err);
> }
>
> so they can just append if they want appending.
>
> Generally speaking, appending to nothing should give you something.
>
> Regards,
> Peter
>

   Make sense, will fix as above, thanks for review.

>> +        return;
>> +    }
>> +
>> +    va_start(ap, fmt);
>> +    msg = g_strdup_vprintf(fmt, ap);
>> +    va_end(ap);
>> +
>> +    msg_old = (*err)->msg;
>> +    (*err)->msg = g_strdup_printf("%s\n%s", msg_old, msg);
>> +
>> +    g_free(msg);
>> +    g_free(msg_old);
>> +
>> +}
>> +
>>   void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>>                        const char *fmt, ...)
>>   {
>> --
>> 1.7.1
>>
>>
>