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

[Markus Armbruster]

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
indeed.

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

Thanks!