Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Greg Kurz]

On Thu, 19 Sep 2019 09:28:11 +0000
Vladimir Sementsov-Ogievskiy <address@hidden> wrote:

> 19.09.2019 11:59, Greg Kurz wrote:
> > On Wed, 18 Sep 2019 16:02:44 +0300
> > Vladimir Sementsov-Ogievskiy <address@hidden> 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 <address@hidden>
> >> ---
> >>   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) {
> > 
> 
>