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 <arm...@redhat.com>
> ---
> 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
