On Wed, 18 Sep 2019 16:02:44 +0300 Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> 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. > This will depend on whether we reach a consensus soon enough (soft freeze for 4.2 is 2019-10-29). Otherwise, I think my series should be merged as is, and auto-propagation to be delayed to 4.3. > 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). > When we have a good auto-propagation mechanism available, I guess this can be beneficial for most of the code, not only the places where we add hints (or prepend something, see below). > [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(-) > > 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); We also have error_propagate_prepend(), which is basically a variant of error_prepend()+error_propagate() that can cope with &error_fatal. This was introduced by Markus in commit 4b5766488fd3, for similar reasons that motivated my series. It has ~30 users in the tree. There's no such thing a generic cleanup function with a printf-like interface, so all of these should be converted back to error_prepend() if we go for auto-propagation. Aside from propagation, one can sometime choose to call things like error_free() or error_free_or_abort()... Auto-propagation should detect that and not call error_propagate() in this case. > + } > +} > + > +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 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. > + */ > +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); > + } > +} > + > +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)} > + > + > +/* > + * 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) = {.errp = (errp)}; \ > + Error **auto_errp = \ > + ((errp) == NULL || *(errp) == error_abort || *(errp) == 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) = {.errp = (errp)}; \ > +if ((errp) == NULL || *(errp) == error_abort || *(errp) == error_fatal) { \ > + (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 > @@ -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); > 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) { > 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); > + } 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}; > > - bdrv_set_backing_hd(bs_new, bs_top, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + errp = &prop.local_err; > + > + bdrv_set_backing_hd(bs_new, bs_top, errp); > + if (*errp) { > 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; > } > @@ -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 = 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); > 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. Note, it must be placed exactly after > + * all local variables defenition. > + */ > + MAKE_ERRP_SAFE(errp); > + > if (filename) { > ret = qemu_gluster_parse_uri(gconf, filename); > if (ret < 0) {