Eric Blake <ebl...@redhat.com> 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 <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.
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 <ebl...@redhat.com> Thanks!