From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38246) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMBId-00017O-1U for qemu-devel@nongnu.org; Fri, 13 Feb 2015 03:08:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YMBIZ-0008PR-QV for qemu-devel@nongnu.org; Fri, 13 Feb 2015 03:08:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YMBIZ-0008PN-Hq for qemu-devel@nongnu.org; Fri, 13 Feb 2015 03:08:27 -0500 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1D88QpL004164 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 13 Feb 2015 03:08:26 -0500 From: Markus Armbruster References: <1423748040-3448-1-git-send-email-armbru@redhat.com> <1423748040-3448-2-git-send-email-armbru@redhat.com> <54DCC635.3010405@redhat.com> Date: Fri, 13 Feb 2015 09:08:24 +0100 In-Reply-To: <54DCC635.3010405@redhat.com> (Eric Blake's message of "Thu, 12 Feb 2015 08:26:45 -0700") Message-ID: <87wq3mutc7.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 01/10] error: New convenience function error_report_err() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org Eric Blake 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 >> --- >> 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 Thanks!