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

[Eric Blake]

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.

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.

But I can also live without the assert, so:

Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature