From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41940) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7QBr-0006lA-IZ for qemu-devel@nongnu.org; Fri, 31 Aug 2012 08:19:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7QBh-0004rj-QI for qemu-devel@nongnu.org; Fri, 31 Aug 2012 08:19:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26964) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7QBh-0004rd-GS for qemu-devel@nongnu.org; Fri, 31 Aug 2012 08:19:01 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7VCJ0sR003259 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 31 Aug 2012 08:19:00 -0400 Message-ID: <5040ABAF.1040508@redhat.com> Date: Fri, 31 Aug 2012 14:18:55 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <3222678753b33b3a95096767745a5cf313c7ca1b.1345016001.git.phrdina@redhat.com> <20120830114701.7f5c1413@doriath.home> <87ipbz1srl.fsf@blackfin.pond.sub.org> In-Reply-To: <87ipbz1srl.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit 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: Markus Armbruster Cc: Pavel Hrdina , qemu-devel@nongnu.org, Luiz Capitulino Am 31.08.2012 08:26, schrieb Markus Armbruster: > 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. I'd rather not remove existing -errno returns if there's not good reason why we can't provide them (or can't do so easily at least). Kevin