[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 01/10] error: New convenience function error_

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2 01/10] error: New convenience function error_report_err()
Date: Fri, 13 Feb 2015 09:08:24 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 02/12/2015 06:33 AM, Markus Armbruster wrote:
>> I've typed error_report("%s", error_get_pretty(ERR)) too many times
>> already, and I've fixed too many instances of qerror_report_err(ERR)
>> to error_report("%s", error_get_pretty(ERR)) as well.  Capture the
>> pattern in a convenience function.
>> Since it's almost invariably followed by error_free(), stuff that into
>> the convenience function as well.
>> The next patch will put it to use.
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  include/qapi/error.h | 5 +++++
>>  util/error.c         | 6 ++++++
>>  2 files changed, 11 insertions(+)
>> +++ b/util/error.c
>> @@ -152,6 +152,12 @@ const char *error_get_pretty(Error *err)
>>      return err->msg;
>>  }
>> +void error_report_err(Error *err)
>> +{
>> +    error_report("%s", error_get_pretty(err));
>> +    error_free(err);
> When I read v1, I wondered if it would make sense to allow:
> Error *local_err = NULL;
> error_report_err(local_err);
> as a no-op, so that calling code can unconditionally use this function
> rather than always burying it inside an 'if (problem)'.  But in
> reviewing the rest of the patches, I wasn't sure it would save very many
> lines, and it also seems like it would be a bit more confusing to see a
> call to an error report function when there is no error to report.

I like my cleanup functions to work unconditionally, like free() does.
But error_report_err() isn't just cleanup, it's called for its very
visible side effect.  Calling it unconditionally would be confusing

> So in the opposite direction of thought, I wonder if you should add:
> assert(err);
> and enforce that this function is only ever used on real error messages,
> especially since error_get_pretty segfaults if called on no error.

I wouldn't mind, but I'm reluctant to respin just for that.

> But I can also live without the assert, so:
> Reviewed-by: Eric Blake <address@hidden>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]