From nobody Mon Apr 29 13:18:53 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=virtuozzo.com ARC-Seal: i=1; a=rsa-sha256; t=1568811928; cv=none; d=zoho.com; s=zohoarc; b=Z20SewGqRlQ4OPxmJ2sJ/tiwrKFKFZwucqsMQmMLaJfTD68onom61+HI3md9OAxRdm1H/Jt2VOrYnfTJ4lS9T2Ao+PUhTJrvcZeZP97XBRigjc+rxhJ/Lap67K1CXxPBr1HTdft5FN/TXC8ellWe73bPTuxUVG8ddSI5TlnFTAY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com; s=zohoarc; t=1568811928; h=Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To:ARC-Authentication-Results; bh=QYEAMaUrnP61Hp4R7PCY5nQmawLLxsQktbPm861Eb+g=; b=QLbyBrYWPRnXREoJzsqAdCiW6pGQMzSH8VMEb7y0Xr6noyp/5/5pBT7nO4OOMfWXgA6gFcTTRh/KfTtuWdNMZAscs4+9mgV0+B4IrVC4KEI8km3kJ+kubFPVer9NAXKLy2cG9tl5FuROMfMnKDgoLYres9ST3SqgKuhg6ForaBo= ARC-Authentication-Results: i=1; mx.zoho.com; spf=pass (zoho.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail header.from= (p=none dis=none) header.from= Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 1568811927786655.5087723790723; Wed, 18 Sep 2019 06:05:27 -0700 (PDT) Received: from localhost ([::1]:58774 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iAZe4-0002H7-T8 for importer@patchew.org; Wed, 18 Sep 2019 09:05:20 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:58041) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iAZbn-0001Lz-JP for qemu-devel@nongnu.org; Wed, 18 Sep 2019 09:03:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iAZbk-0006GX-Rj for qemu-devel@nongnu.org; Wed, 18 Sep 2019 09:02:58 -0400 Received: from relay.sw.ru ([185.231.240.75]:36080) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iAZbk-0006G0-KJ; Wed, 18 Sep 2019 09:02:56 -0400 Received: from [10.94.3.0] (helo=kvm.qa.sw.ru) by relay.sw.ru with esmtp (Exim 4.92.2) (envelope-from ) id 1iAZbb-0000E4-1f; Wed, 18 Sep 2019 16:02:47 +0300 From: Vladimir Sementsov-Ogievskiy To: qemu-devel@nongnu.org Date: Wed, 18 Sep 2019 16:02:44 +0300 Message-Id: <20190918130244.24257-1-vsementsov@virtuozzo.com> X-Mailer: git-send-email 2.21.0 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 185.231.240.75 Subject: [Qemu-devel] [RFC] error: auto propagated local_err X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: fam@euphon.net, peter.maydell@linaro.org, mst@redhat.com, codyprime@gmail.com, mark.cave-ayland@ilande.co.uk, mdroth@linux.vnet.ibm.com, kraxel@redhat.com, mreitz@redhat.com, qemu-block@nongnu.org, quintela@redhat.com, david@redhat.com, armbru@redhat.com, pasic@linux.ibm.com, borntraeger@de.ibm.com, marcandre.lureau@redhat.com, rth@twiddle.net, farman@linux.ibm.com, groug@kaod.org, dgilbert@redhat.com, alex.williamson@redhat.com, qemu-arm@nongnu.org, stefanha@redhat.com, jsnow@redhat.com, david@gibson.dropbear.id.au, kwolf@redhat.com, vsementsov@virtuozzo.com, berrange@redhat.com, cohuck@redhat.com, qemu-s390x@nongnu.org, sundeep.lkml@gmail.com, qemu-ppc@nongnu.org, pbonzini@redhat.com Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" Content-Type: text/plain; charset="utf-8" Hi all! Here is a proposal (three of them, actually) of auto propagation for local_err, to not call error_propagate on every exit point, when we deal with local_err. It also may help make Greg's series[1] about error_append_hint smaller. See definitions and examples below. I'm cc-ing to this RFC everyone from series[1] CC list, as if we like it, the idea will touch same code (and may be more). [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html Signed-off-by: Vladimir Sementsov-Ogievskiy --- include/qapi/error.h | 102 +++++++++++++++++++++++++++++++++++++++++++ block.c | 63 ++++++++++++-------------- block/backup.c | 8 +++- block/gluster.c | 7 +++ 4 files changed, 144 insertions(+), 36 deletions(-) diff --git a/include/qapi/error.h b/include/qapi/error.h index 3f95141a01..083e061014 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -322,6 +322,108 @@ void error_set_internal(Error **errp, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(6, 7); =20 +typedef struct ErrorPropagator { + Error **errp; + Error *local_err; +} ErrorPropagator; + +static inline void error_propagator_cleanup(ErrorPropagator *prop) +{ + if (prop->local_err) { + error_propagate(prop->errp, prop->local_err); + } +} + +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup= ); + +/* + * ErrorPropagationPair + * + * [Error *local_err, Error **errp] + * + * First element is local_err, second is original errp, which is propagati= on + * target. Yes, errp has a bit another type, so it should be converted. + * + * ErrorPropagationPair may be used as errp, which points to local_err, + * as it's type is compatible. + */ +typedef Error *ErrorPropagationPair[2]; + +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *ar= r) +{ + Error *local_err =3D (*arr)[0]; + Error **errp =3D (Error **)(*arr)[1]; + + if (local_err) { + error_propagate(errp, local_err); + } +} + +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationPair, + error_propagation_pair_cleanup); + +/* + * DEF_AUTO_ERRP + * + * Define auto_errp variable, which may be used instead of errp, and + * *auto_errp may be safely checked to be zero or not, and may be safely + * used for error_append_hint(). auto_errp is automatically propagated + * to errp at function exit. + */ +#define DEF_AUTO_ERRP(auto_errp, errp) \ + g_auto(ErrorPropagationPair) (auto_errp) =3D {NULL, (Error *)(errp)} + + +/* + * Another variant: + * Pros: + * - normal structure instead of cheating with array + * - we can directly use errp, if it's not NULL and don't point to + * error_abort or error_fatal + * Cons: + * - we need to define two variables instead of one + */ +typedef struct ErrorPropagationStruct { + Error *local_err; + Error **errp; +} ErrorPropagationStruct; + +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct= *prop) +{ + if (prop->local_err) { + error_propagate(prop->errp, prop->local_err); + } +} + +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagationStruct, + error_propagation_struct_cleanup); + +#define DEF_AUTO_ERRP_V2(auto_errp, errp) \ + g_auto(ErrorPropagationStruct) (__auto_errp_prop) =3D {.errp =3D (errp= )}; \ + Error **auto_errp =3D \ + ((errp) =3D=3D NULL || *(errp) =3D=3D error_abort || *(errp) =3D= =3D error_fatal) ? \ + &__auto_errp_prop.local_err : \ + (errp); + +/* + * Third variant: + * Pros: + * - simpler movement for functions which don't have local_err yet + * the only thing to do is to call one macro at function start. + * This extremely simplifies Greg's series + * Cons: + * - looks like errp shadowing.. Still seems safe. + * - must be after all definitions of local variables and before any + * code. + * - like v2, several statements in one open macro + */ +#define MAKE_ERRP_SAFE(errp) \ +g_auto(ErrorPropagationStruct) (__auto_errp_prop) =3D {.errp =3D (errp)}; \ +if ((errp) =3D=3D NULL || *(errp) =3D=3D error_abort || *(errp) =3D=3D err= or_fatal) { \ + (errp) =3D &__auto_errp_prop.local_err; \ +} + + /* * Special error destination to abort on error. * See error_setg() and error_propagate() for details. diff --git a/block.c b/block.c index 5944124845..5253663329 100644 --- a/block.c +++ b/block.c @@ -1275,12 +1275,11 @@ static int bdrv_open_driver(BlockDriverState *bs, B= lockDriver *drv, const char *node_name, QDict *options, int open_flags, Error **errp) { - Error *local_err =3D NULL; + DEF_AUTO_ERRP_V2(auto_errp, errp); int i, ret; =20 - bdrv_assign_node_name(bs, node_name, &local_err); - if (local_err) { - error_propagate(errp, local_err); + bdrv_assign_node_name(bs, node_name, auto_errp); + if (*auto_errp) { return -EINVAL; } =20 @@ -1290,20 +1289,21 @@ static int bdrv_open_driver(BlockDriverState *bs, B= lockDriver *drv, =20 if (drv->bdrv_file_open) { assert(!drv->bdrv_needs_filename || bs->filename[0]); - ret =3D drv->bdrv_file_open(bs, options, open_flags, &local_err); + ret =3D drv->bdrv_file_open(bs, options, open_flags, auto_errp); } else if (drv->bdrv_open) { - ret =3D drv->bdrv_open(bs, options, open_flags, &local_err); + ret =3D drv->bdrv_open(bs, options, open_flags, auto_errp); } else { ret =3D 0; } =20 if (ret < 0) { - if (local_err) { - error_propagate(errp, local_err); - } else if (bs->filename[0]) { - error_setg_errno(errp, -ret, "Could not open '%s'", bs->filena= me); - } else { - error_setg_errno(errp, -ret, "Could not open image"); + if (!*auto_errp) { + if (bs->filename[0]) { + error_setg_errno(errp, -ret, "Could not open '%s'", + bs->filename); + } else { + error_setg_errno(errp, -ret, "Could not open image"); + } } goto open_failed; } @@ -1314,9 +1314,8 @@ static int bdrv_open_driver(BlockDriverState *bs, Blo= ckDriver *drv, return ret; } =20 - bdrv_refresh_limits(bs, &local_err); - if (local_err) { - error_propagate(errp, local_err); + bdrv_refresh_limits(bs, auto_errp); + if (*auto_errp) { return -EINVAL; } =20 @@ -4238,17 +4237,17 @@ out: void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp) { - Error *local_err =3D NULL; + g_auto(ErrorPropagator) prop =3D {.errp =3D errp}; =20 - bdrv_set_backing_hd(bs_new, bs_top, &local_err); - if (local_err) { - error_propagate(errp, local_err); + errp =3D &prop.local_err; + + bdrv_set_backing_hd(bs_new, bs_top, errp); + if (*errp) { goto out; } =20 - bdrv_replace_node(bs_top, bs_new, &local_err); - if (local_err) { - error_propagate(errp, local_err); + bdrv_replace_node(bs_top, bs_new, errp); + if (*errp) { bdrv_set_backing_hd(bs_new, NULL, &error_abort); goto out; } @@ -5309,9 +5308,9 @@ void bdrv_init_with_whitelist(void) static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp) { + DEF_AUTO_ERRP(auto_errp, errp); BdrvChild *child, *parent; uint64_t perm, shared_perm; - Error *local_err =3D NULL; int ret; BdrvDirtyBitmap *bm; =20 @@ -5324,9 +5323,8 @@ static void coroutine_fn bdrv_co_invalidate_cache(Blo= ckDriverState *bs, } =20 QLIST_FOREACH(child, &bs->children, next) { - bdrv_co_invalidate_cache(child->bs, &local_err); - if (local_err) { - error_propagate(errp, local_err); + bdrv_co_invalidate_cache(child->bs, auto_errp); + if (*auto_errp) { return; } } @@ -5346,19 +5344,17 @@ static void coroutine_fn bdrv_co_invalidate_cache(B= lockDriverState *bs, */ bs->open_flags &=3D ~BDRV_O_INACTIVE; bdrv_get_cumulative_perm(bs, &perm, &shared_perm); - ret =3D bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &loca= l_err); + ret =3D bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, auto_= errp); if (ret < 0) { bs->open_flags |=3D BDRV_O_INACTIVE; - error_propagate(errp, local_err); return; } bdrv_set_perm(bs, perm, shared_perm); =20 if (bs->drv->bdrv_co_invalidate_cache) { - bs->drv->bdrv_co_invalidate_cache(bs, &local_err); - if (local_err) { + bs->drv->bdrv_co_invalidate_cache(bs, auto_errp); + if (*auto_errp) { bs->open_flags |=3D BDRV_O_INACTIVE; - error_propagate(errp, local_err); return; } } @@ -5378,10 +5374,9 @@ static void coroutine_fn bdrv_co_invalidate_cache(Bl= ockDriverState *bs, =20 QLIST_FOREACH(parent, &bs->parents, next_parent) { if (parent->role->activate) { - parent->role->activate(parent, &local_err); - if (local_err) { + parent->role->activate(parent, auto_errp); + if (*auto_errp) { bs->open_flags |=3D BDRV_O_INACTIVE; - error_propagate(errp, local_err); return; } } diff --git a/block/backup.c b/block/backup.c index 89f7f89200..462dea4fbb 100644 --- a/block/backup.c +++ b/block/backup.c @@ -583,6 +583,10 @@ static const BlockJobDriver backup_job_driver =3D { static int64_t backup_calculate_cluster_size(BlockDriverState *target, Error **errp) { + /* + * Example of using DEF_AUTO_ERRP to make error_append_hint safe + */ + DEF_AUTO_ERRP(auto_errp, errp); int ret; BlockDriverInfo bdi; =20 @@ -602,10 +606,10 @@ static int64_t backup_calculate_cluster_size(BlockDri= verState *target, BACKUP_CLUSTER_SIZE_DEFAULT); return BACKUP_CLUSTER_SIZE_DEFAULT; } else if (ret < 0 && !target->backing) { - error_setg_errno(errp, -ret, + error_setg_errno(auto_errp, -ret, "Couldn't determine the cluster size of the target image, " "which has no backing file"); - error_append_hint(errp, + error_append_hint(auto_errp, "Aborting, since this may create an unusable destination image= \n"); return ret; } else if (ret < 0 && target->backing) { diff --git a/block/gluster.c b/block/gluster.c index 64028b2cba..799a2dbeca 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -695,6 +695,13 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *= gconf, QDict *options, Error **errp) { int ret; + /* + * Example of using MAKE_ERRP_SAFE to make error_append_hint safe. We + * only need to add one macro call. Note, it must be placed exactly af= ter + * all local variables defenition. + */ + MAKE_ERRP_SAFE(errp); + if (filename) { ret =3D qemu_gluster_parse_uri(gconf, filename); if (ret < 0) { --=20 2.21.0