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.
>
>> + }
>> +}
>> +
>> +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) {
>
--
Best regards,
Vladimir
