On Thu, 19 Sep 2019 09:28:11 +0000 Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> wrote:
> 19.09.2019 11:59, Greg Kurz wrote: > > 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. > > Hmm, for example, qmp_eject, which error_free or error_propagate.. > We can leave such cases as is, not many of them. Or introduce > safe_errp_free(Error **errp), which will zero pointer after freeing. > Maybe even turning error_free() to take an Error ** ? It looks safe to zero out a dangling pointer. Of course the API change would need to be propagated to all error_* functions that explicitly call error_free(). > > > >> + } > >> +} > >> + > >> +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) { > > > >
