On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote: > 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).
It might have been nice to do a patch series of one proposal per patch, rather than cramming three in one, if only to see... > > [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > include/qapi/error.h | 102 +++++++++++++++++++++++++++++++++++++++++++ > block.c | 63 ++++++++++++-------------- > block/backup.c | 8 +++- > block/gluster.c | 7 +++ > 4 files changed, 144 insertions(+), 36 deletions(-) ...relative diffstat sizing differences between them. Of course, adding the framework requires growth in error.h. The real question, then, is how many lines do we save elsewhere by getting rid of boilerplate that is replaced by auto cleanup, and how many lines are mechanically churned to change variable names. And how much of that churn can itself be automated with Coccinelle. But I think the idea is worth pursuing! > > 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); > > +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); > + } Technically, you don't need the 'if' here, since error_propogate() works just fine when prop->local_err is NULL. > +} > + > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup); > + Looks reasonable. > +/* > + * ErrorPropagationPair > + * > + * [Error *local_err, Error **errp] > + * > + * First element is local_err, second is original errp, which is propagation > + * 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. s/it's/its/ ("it's" is only appropriate if "it is" can be substituted) > + */ > +typedef Error *ErrorPropagationPair[2]; > + > +static inline void error_propagation_pair_cleanup(ErrorPropagationPair *arr) > +{ > + Error *local_err = (*arr)[0]; > + Error **errp = (Error **)(*arr)[1]; > + > + if (local_err) { > + error_propagate(errp, local_err); > + } > +} I don't like the type-punning. > + > +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) = {NULL, (Error *)(errp)} Again, more type-punning. Ends up declaring the equivalent of 'Error *auto_errp[2]' which devolves to 'Error **auto_errp'. So, using this macro requires me to pass in the name of my local variable (which auto-propagates when it goes out of scope) and the errp parameter. Can we shortcut by declaring that the variable it propagates to MUST be named 'errp' rather than having that as a parameter name (doing so would force us to be consistent at using an Error **errp parameter). > + > + > +/* > + * 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; No real difference from ErrorPropagator above. > + > +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct > *prop) > +{ > + if (prop->local_err) { > + error_propagate(prop->errp, prop->local_err); > + } Another useless if. > +} > + > +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) = {.errp = (errp)}; \ > + Error **auto_errp = \ > + ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) > ? \ > + &__auto_errp_prop.local_err : \ > + (errp); Not written to take a trailing semicolon in the caller. Less type-punning, and and tries to reuse errp where possible. > + > +/* > + * 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. Why? I see no reason why it can't be hoisted earlier than other declarations, and the only reason to not sink it after earlier code that doesn't touch errp would be our coding standards that frowns on declaration after code. > + * - like v2, several statements in one open macro Not a drawback in my mind. > + */ > +#define MAKE_ERRP_SAFE(errp) \ > +g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = (errp)}; \ > +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { \ > + (errp) = &__auto_errp_prop.local_err; \ > +} Not written to take a trailing semicolon in the caller. You could even set __auto_errp_prop unconditionally rather than trying to reuse incoming errp (the difference being that error_propagate() gets called more frequently). This is actually quite cool. And if you get rid of your insistence that it must occur after other variable declarations, you could instead easily automate that any function that has a parameter 'Error **errp' then has a MAKE_ERRP_SAFE(errp); as the first line of its function body (that becomes something that you could grep for, rather than having to use the smarts of coccinelle). Or if we want to enforce consistency on the parameter naming, even go with: #define MAKE_ERRP_SAFE() \ g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \ errp = &__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 So the above was the proposed implementations (I'm leaning towards option 3); the below is a demonstration of their use. > @@ -1275,12 +1275,11 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > const char *node_name, QDict *options, > int open_flags, Error **errp) > { > - Error *local_err = NULL; > + DEF_AUTO_ERRP_V2(auto_errp, errp); This is option 2. Basically, you replace any declaration of local_err to instead use the magic macro... > int i, ret; > > - 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) { then s/local_err/*auto_errp/ and delete resulting &* pairs and error_propagate() calls in the rest of the function. Coccinelle could make this type of conversion easy. > return -EINVAL; > } > > @@ -1290,20 +1289,21 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > > if (drv->bdrv_file_open) { > assert(!drv->bdrv_needs_filename || bs->filename[0]); > - ret = drv->bdrv_file_open(bs, options, open_flags, &local_err); > + ret = drv->bdrv_file_open(bs, options, open_flags, auto_errp); > } else if (drv->bdrv_open) { > - ret = drv->bdrv_open(bs, options, open_flags, &local_err); > + ret = drv->bdrv_open(bs, options, open_flags, auto_errp); > } else { > ret = 0; > } > > 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->filename); > - } 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); You reflowed the logic a bit here, but it is still worth pointing out that you still call error_setg*(errp) as before (no changing to auto_errp, although that would also have worked). > + } else { > + error_setg_errno(errp, -ret, "Could not open image"); > + } > } > goto open_failed; > } > @@ -1314,9 +1314,8 @@ static int bdrv_open_driver(BlockDriverState *bs, > BlockDriver *drv, > return ret; > } > > - 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; > } > > @@ -4238,17 +4237,17 @@ out: > void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > Error **errp) > { > - Error *local_err = NULL; > + g_auto(ErrorPropagator) prop = {.errp = errp}; This is option 1. It's rather open-coded, rather than having a nice wrapper macro. > > - bdrv_set_backing_hd(bs_new, bs_top, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + errp = &prop.local_err; And by re-assigning errp manually, > + > + bdrv_set_backing_hd(bs_new, bs_top, errp); > + if (*errp) { you can now safely use it in the rest of the function without worrying about NULL. > goto out; > } > > - 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; Not too bad (we'd probably want a coccinelle script to prove that you don't dereference *errp without first using the magic reassignment). > } > @@ -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); Option 1 again, but this time with a macro. > BdrvChild *child, *parent; > uint64_t perm, shared_perm; > - Error *local_err = NULL; > int ret; > BdrvDirtyBitmap *bm; > > @@ -5324,9 +5323,8 @@ static void coroutine_fn > bdrv_co_invalidate_cache(BlockDriverState *bs, > } > > 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(BlockDriverState *bs, > */ > bs->open_flags &= ~BDRV_O_INACTIVE; > bdrv_get_cumulative_perm(bs, &perm, &shared_perm); > - ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, > &local_err); > + ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, > auto_errp); > if (ret < 0) { > bs->open_flags |= BDRV_O_INACTIVE; > - error_propagate(errp, local_err); > return; > } > bdrv_set_perm(bs, perm, shared_perm); > > 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 |= BDRV_O_INACTIVE; > - error_propagate(errp, local_err); > return; > } > } > @@ -5378,10 +5374,9 @@ static void coroutine_fn > bdrv_co_invalidate_cache(BlockDriverState *bs, > > 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 |= 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 = { > 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); Option 1 again. Requires renaming errp to auto_errp in the rest of the function. > int ret; > BlockDriverInfo bdi; > > @@ -602,10 +606,10 @@ static int64_t > backup_calculate_cluster_size(BlockDriverState *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. Option 3. By far my favorite, whether or not we decide to take a macro parameter for the variable name to protect, or hard-code for consistency that 'errp' must be in scope when it is used. > Note, it must be placed exactly after > + * all local variables defenition. definition Why do you think this limitation is necessary? Do you have an actual code sequence that misbehaves if the compiler-unwind cleanup is performed in a different order when the macro is used before other variable declarations? > + */ > + MAKE_ERRP_SAFE(errp); > + > if (filename) { > ret = qemu_gluster_parse_uri(gconf, filename); > if (ret < 0) { > This is sweet - you can safely use '*errp' in the rest of the function, without having to remember a second error name - while the caller can still pass NULL or error_abort as desired. And I still think we can probably get Coccinelle to help make the conversions, both of using this macro in any function that has an Error **errp parameter, as well as getting rid of local_err declarations and error_propagate() calls rendered redundant once this macro is used. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
