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

[Greg Kurz]

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.

> +    }
> +}
> +
> +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) {