From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59666) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLWLW-0000Un-Oy for qemu-devel@nongnu.org; Wed, 11 Feb 2015 07:24:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLWLR-0000vS-OO for qemu-devel@nongnu.org; Wed, 11 Feb 2015 07:24:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLWLR-0000vK-HJ for qemu-devel@nongnu.org; Wed, 11 Feb 2015 07:24:41 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1BCOeGE006132 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 11 Feb 2015 07:24:40 -0500 From: Markus Armbruster References: <1423586055-4932-1-git-send-email-armbru@redhat.com> <1423586055-4932-2-git-send-email-armbru@redhat.com> <54DA3DD7.4050308@redhat.com> <20150211100407.GB5572@noname.str.redhat.com> Date: Wed, 11 Feb 2015 13:24:38 +0100 In-Reply-To: <20150211100407.GB5572@noname.str.redhat.com> (Kevin Wolf's message of "Wed, 11 Feb 2015 11:04:07 +0100") Message-ID: <87iof88wl5.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/9] error: New convenience function error_report_err() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org Kevin Wolf writes: > Am 10.02.2015 um 18:20 hat Eric Blake geschrieben: >> On 02/10/2015 09:34 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. >> > >> >> > @@ -2234,8 +2225,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> > >> > ret = do_sd_create(s, &new_vid, 1, &local_err); >> > if (ret < 0) { >> > - error_report("%s", error_get_pretty(local_err));; >> > - error_free(local_err); >> > + error_report_err(local_err); >> > error_report("failed to create inode for snapshot. %s", >> > strerror(errno)); >> >> Pre-existing bug, so maybe worth a separate patch. This looks fishy: >> are we guaranteed that errno is unchanged by error_report_err()? > > errno doesn't seem to contain anything meaningful here in the first > place, so I think that line should simply be removed. Either that, or combine the messages like this: error_report("failed to create inode for snapshot: %s", error_get_pretty(local_err)); Two error_report() in a row are suspect in general. Aside: do_sd_create() can't decide what to return on failure, negative value without meaning, or a negative error number. >> On the >> surface, error_vreport() and friends do NOT try to preserve errno; maybe >> your new function should guarantee that errno is not clobbered? (in >> libvirt, we've explicitly made error-reporting convenience functions >> document that they do not clobber errno, because it is much easier for >> callers to report an error then return an errno value without having to >> save errno locally) > > Isn't something going wrong if you report an error and pass it on at the > same time? I always thought that the error reporting should be at the > end of the error handling code. I guess that depends on the conventions used by the code. In QEMU, error reporting is generally an error information sink, i.e. no detailed information on the reported error is passed further up. >> > @@ -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); >> > +} >> > + >> >> If it were me, I'd split this patch in two, one that introduces the new >> function (with no clients), and the other which is a strict Coccinelle >> touchup to use it, so that readers don't have to hunt for the meat of >> the change. > > Yes, I thought the same. I can do that.