From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47748) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7Kgl-0003Mh-7H for qemu-devel@nongnu.org; Fri, 31 Aug 2012 02:26:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7Kgk-0004d5-3v for qemu-devel@nongnu.org; Fri, 31 Aug 2012 02:26:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52913) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7Kgj-0004co-RD for qemu-devel@nongnu.org; Fri, 31 Aug 2012 02:26:42 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7V6Qexa025506 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 31 Aug 2012 02:26:40 -0400 From: Markus Armbruster References: <3222678753b33b3a95096767745a5cf313c7ca1b.1345016001.git.phrdina@redhat.com> <20120830114701.7f5c1413@doriath.home> Date: Fri, 31 Aug 2012 08:26:38 +0200 In-Reply-To: <20120830114701.7f5c1413@doriath.home> (Luiz Capitulino's message of "Thu, 30 Aug 2012 11:47:01 -0300") Message-ID: <87ipbz1srl.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 02/18] block: add error parameter to bdrv_snapshot_create() and related functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , Pavel Hrdina , qemu-devel@nongnu.org Luiz Capitulino writes: > On Wed, 15 Aug 2012 09:41:43 +0200 > Pavel Hrdina wrote: > >> Signed-off-by: Pavel Hrdina >> --- >> block.c | 25 +++++++++++++++++-------- >> block.h | 3 ++- >> block/qcow2-snapshot.c | 9 ++++++++- >> block/qcow2.h | 4 +++- >> block/rbd.c | 20 ++++++++++++++------ >> block/sheepdog.c | 17 +++++++++-------- >> block_int.h | 3 ++- >> qemu-img.c | 2 +- >> savevm.c | 2 +- >> 9 files changed, 57 insertions(+), 28 deletions(-) >> >> diff --git a/block.c b/block.c >> index 016858b..8bc49b7 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2661,16 +2661,25 @@ BlockDriverState *bdrv_snapshots(void) >> } >> >> int bdrv_snapshot_create(BlockDriverState *bs, >> - QEMUSnapshotInfo *sn_info) >> + QEMUSnapshotInfo *sn_info, >> + Error **errp) >> { >> BlockDriver *drv = bs->drv; >> - if (!drv) >> - return -ENOMEDIUM; >> - if (drv->bdrv_snapshot_create) >> - return drv->bdrv_snapshot_create(bs, sn_info); >> - if (bs->file) >> - return bdrv_snapshot_create(bs->file, sn_info); >> - return -ENOTSUP; >> + int ret; >> + >> + if (!drv) { >> + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs)); > > We should only use QERR_ macros for the errors listed in the ErrorClass enum > (except GenericError), all other errors should generally use error_setg(), like > this: > > error_setg(errp, "device '%s' has no medium); > >> + ret = -ENOMEDIUM; > > And, usually, we should get rid of errno propagation. There are two cases here: The block layer consistently[*] uses -errno return values. Its consistency is valuable, and I'm a bit reluctant to break it. Maybe a new rule "returns -errno, except when it has an Error ** argument" could work. I'd like to hear Kevin's advice on this. > > 1. errno is propagated up so that upper layers can print a decent error > message to the user. > > In this case, it's safe to eliminate errno. error_setg() will store a > decent message already and the Error object can be propagated up. > > 2. errno is propagated up so that upper layers can distinguish among > error causes and take different actions accordingly. > > Doesn't seem to be the case of bdrv_snapshot_create() (ie. errno is only > used to communicate the error to the user). However, I'm pretty sure that > such usage exists in qemu and the error API will break it, as most of our > errors are generic. > > I see two solutions to this problem: > > A. Add specific errors to ErrorClass. I don't like this very much, > as it's possible that such errors are going to be useful only internally. Let's not reinvent errno, poorly. > B. Add two new functions: > > void error_sete(Error **err, ErrorClass err_class, int errno, const char *fmt, ...); > int error_get_errno(const Error **err); > > So that we can maintain errno when it's used to communicate > error cause among functions. Better. What's ErrorClass doing there? [...] [*] Almost; it's still QEMU after all.