[RFC] error: auto propagated local_err
diff mbox series

Message ID 20190918130244.24257-1-vsementsov@virtuozzo.com
State New
Headers show
Series
  • [RFC] error: auto propagated local_err
Related show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 18, 2019, 1:02 p.m. UTC
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.

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).

[1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)

Comments

Eric Blake Sept. 18, 2019, 5:10 p.m. UTC | #1
On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy 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.
> 
> 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).

It might have been nice to do a patch series of one proposal per patch,
rather than cramming three in one, if only to see...

> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)

...relative diffstat sizing differences between them.

Of course, adding the framework requires growth in error.h.  The real
question, then, is how many lines do we save elsewhere by getting rid of
boilerplate that is replaced by auto cleanup, and how many lines are
mechanically churned to change variable names.  And how much of that
churn can itself be automated with Coccinelle.

But I think the idea is worth pursuing!

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

Technically, you don't need the 'if' here, since error_propogate() works
just fine when prop->local_err is NULL.

> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +

Looks reasonable.

> +/*
> + * 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.

s/it's/its/  ("it's" is only appropriate if "it is" can be substituted)

> + */
> +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);
> +    }
> +}

I don't like the type-punning.

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

Again, more type-punning.  Ends up declaring the equivalent of 'Error
*auto_errp[2]' which devolves to 'Error **auto_errp'.

So, using this macro requires me to pass in the name of my local
variable (which auto-propagates when it goes out of scope) and the errp
parameter.  Can we shortcut by declaring that the variable it propagates
to MUST be named 'errp' rather than having that as a parameter name
(doing so would force us to be consistent at using an Error **errp
parameter).

> +
> +
> +/*
> + * 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;

No real difference from ErrorPropagator above.

> +
> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct *prop)
> +{
> +    if (prop->local_err) {
> +        error_propagate(prop->errp, prop->local_err);
> +    }

Another useless if.

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


Not written to take a trailing semicolon in the caller.

Less type-punning, and and tries to reuse errp where possible.

> +
> +/*
> + * 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.

Why?  I see no reason why it can't be hoisted earlier than other
declarations, and the only reason to not sink it after earlier code that
doesn't touch errp would be our coding standards that frowns on
declaration after code.

> + *     - like v2, several statements in one open macro

Not a drawback in my mind.

> + */
> +#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; \
> +}

Not written to take a trailing semicolon in the caller.

You could even set __auto_errp_prop unconditionally rather than trying
to reuse incoming errp (the difference being that error_propagate() gets
called more frequently).

This is actually quite cool.  And if you get rid of your insistence that
it must occur after other variable declarations, you could instead
easily automate that any function that has a parameter 'Error **errp'
then has a MAKE_ERRP_SAFE(errp); as the first line of its function body
(that becomes something that you could grep for, rather than having to
use the smarts of coccinelle).

Or if we want to enforce consistency on the parameter naming, even go with:

#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
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

So the above was the proposed implementations (I'm leaning towards
option 3); the below is a demonstration of their use.

> @@ -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);

This is option 2. Basically, you replace any declaration of local_err to
instead use the magic macro...

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

then s/local_err/*auto_errp/ and delete resulting &* pairs and
error_propagate() calls in the rest of the function.  Coccinelle could
make this type of conversion easy.

>          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);

You reflowed the logic a bit here, but it is still worth pointing out
that you still call error_setg*(errp) as before (no changing to
auto_errp, although that would also have worked).

> +            } 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};

This is option 1.  It's rather open-coded, rather than having a nice
wrapper macro.

>  
> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    errp = &prop.local_err;

And by re-assigning errp manually,

> +
> +    bdrv_set_backing_hd(bs_new, bs_top, errp);
> +    if (*errp) {

you can now safely use it in the rest of the function without worrying
about NULL.

>          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;

Not too bad (we'd probably want a coccinelle script to prove that you
don't dereference *errp without first using the magic reassignment).

>      }
> @@ -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);

Option 1 again, but this time with a macro.

>      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);

Option 1 again.  Requires renaming errp to auto_errp in the rest of the
function.

>      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.

Option 3.  By far my favorite, whether or not we decide to take a macro
parameter for the variable name to protect, or hard-code for consistency
that 'errp' must be in scope when it is used.

> Note, it must be placed exactly after
> +     * all local variables defenition.

definition

Why do you think this limitation is necessary?  Do you have an actual
code sequence that misbehaves if the compiler-unwind cleanup is
performed in a different order when the macro is used before other
variable declarations?

> +     */
> +    MAKE_ERRP_SAFE(errp);
> +
>      if (filename) {
>          ret = qemu_gluster_parse_uri(gconf, filename);
>          if (ret < 0) {
> 

This is sweet - you can safely use '*errp' in the rest of the function,
without having to remember a second error name - while the caller can
still pass NULL or error_abort as desired.

And I still think we can probably get Coccinelle to help make the
conversions, both of using this macro in any function that has an Error
**errp parameter, as well as getting rid of local_err declarations and
error_propagate() calls rendered redundant once this macro is used.
Vladimir Sementsov-Ogievskiy Sept. 18, 2019, 5:46 p.m. UTC | #2
18.09.2019 20:10, Eric Blake wrote:
> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy 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.
>>
>> 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).
> 
> It might have been nice to do a patch series of one proposal per patch,
> rather than cramming three in one, if only to see...
> 
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)
> 
> ...relative diffstat sizing differences between them.
> 
> Of course, adding the framework requires growth in error.h.  The real
> question, then, is how many lines do we save elsewhere by getting rid of
> boilerplate that is replaced by auto cleanup, and how many lines are
> mechanically churned to change variable names.  And how much of that
> churn can itself be automated with Coccinelle.
> 
> But I think the idea is worth pursuing!
> 
>>
>> 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);
>> +    }
> 
> Technically, you don't need the 'if' here, since error_propogate() works
> just fine when prop->local_err is NULL.
> 
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
> 
> Looks reasonable.
> 
>> +/*
>> + * 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.
> 
> s/it's/its/  ("it's" is only appropriate if "it is" can be substituted)
> 
>> + */
>> +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);
>> +    }
>> +}
> 
> I don't like the type-punning.
> 
>> +
>> +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)}
> 
> Again, more type-punning.  Ends up declaring the equivalent of 'Error
> *auto_errp[2]' which devolves to 'Error **auto_errp'.
> 
> So, using this macro requires me to pass in the name of my local
> variable (which auto-propagates when it goes out of scope) and the errp
> parameter.  Can we shortcut by declaring that the variable it propagates
> to MUST be named 'errp' rather than having that as a parameter name
> (doing so would force us to be consistent at using an Error **errp
> parameter).
> 
>> +
>> +
>> +/*
>> + * 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;
> 
> No real difference from ErrorPropagator above.

Sorry, I just forget to remove ErrorPropagator.

> 
>> +
>> +static inline void error_propagation_struct_cleanup(ErrorPropagationStruct *prop)
>> +{
>> +    if (prop->local_err) {
>> +        error_propagate(prop->errp, prop->local_err);
>> +    }
> 
> Another useless if.
> 
>> +}
>> +
>> +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);
> 
> 
> Not written to take a trailing semicolon in the caller.
> 
> Less type-punning, and and tries to reuse errp where possible.
> 
>> +
>> +/*
>> + * 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.
> 
> Why?  I see no reason why it can't be hoisted earlier than other
> declarations, and the only reason to not sink it after earlier code that
> doesn't touch errp would be our coding standards that frowns on
> declaration after code.

Hmm, I thought compiler would warn about mixing code and definitions.
Seems that gcc don't care, so it's OK.

> 
>> + *     - like v2, several statements in one open macro
> 
> Not a drawback in my mind.
> 
>> + */
>> +#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; \
>> +}
> 
> Not written to take a trailing semicolon in the caller.
> 
> You could even set __auto_errp_prop unconditionally rather than trying
> to reuse incoming errp (the difference being that error_propagate() gets
> called more frequently).
> 
> This is actually quite cool. 

I glad to see that you like my favorite variant)

>And if you get rid of your insistence that
> it must occur after other variable declarations, you could instead
> easily automate that any function that has a parameter 'Error **errp'
> then has a MAKE_ERRP_SAFE(errp); as the first line of its function body
> (that becomes something that you could grep for, rather than having to
> use the smarts of coccinelle).
> 
> Or if we want to enforce consistency on the parameter naming, even go with:
> 
> #define MAKE_ERRP_SAFE() \
> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
> errp = &__auto_errp_prop.local_err
> 

I am for

>> +
>> +
>>   /*
>>    * 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
> 
> So the above was the proposed implementations (I'm leaning towards
> option 3); the below is a demonstration of their use.
> 
>> @@ -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);
> 
> This is option 2. Basically, you replace any declaration of local_err to
> instead use the magic macro...
> 
>>       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) {
> 
> then s/local_err/*auto_errp/ and delete resulting &* pairs and
> error_propagate() calls in the rest of the function.  Coccinelle could
> make this type of conversion easy.
> 
>>           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);
> 
> You reflowed the logic a bit here, but it is still worth pointing out
> that you still call error_setg*(errp) as before (no changing to
> auto_errp, although that would also have worked).
> 
>> +            } 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};
> 
> This is option 1.  It's rather open-coded, rather than having a nice
> wrapper macro.
> 
>>   
>> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> +    errp = &prop.local_err;
> 
> And by re-assigning errp manually,
> 
>> +
>> +    bdrv_set_backing_hd(bs_new, bs_top, errp);
>> +    if (*errp) {
> 
> you can now safely use it in the rest of the function without worrying
> about NULL.
> 
>>           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;
> 
> Not too bad (we'd probably want a coccinelle script to prove that you
> don't dereference *errp without first using the magic reassignment).
> 
>>       }
>> @@ -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);
> 
> Option 1 again, but this time with a macro.
> 
>>       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);
> 
> Option 1 again.  Requires renaming errp to auto_errp in the rest of the
> function.
> 
>>       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.
> 
> Option 3.  By far my favorite, whether or not we decide to take a macro
> parameter for the variable name to protect, or hard-code for consistency
> that 'errp' must be in scope when it is used.
> 
>> Note, it must be placed exactly after
>> +     * all local variables defenition.
> 
> definition
> 
> Why do you think this limitation is necessary?  Do you have an actual
> code sequence that misbehaves if the compiler-unwind cleanup is
> performed in a different order when the macro is used before other
> variable declarations?

No I just thought that this is a requirement to not mix definitions with code,
I was wrong and that's good.

> 
>> +     */
>> +    MAKE_ERRP_SAFE(errp);
>> +
>>       if (filename) {
>>           ret = qemu_gluster_parse_uri(gconf, filename);
>>           if (ret < 0) {
>>
> 
> This is sweet - you can safely use '*errp' in the rest of the function,
> without having to remember a second error name - while the caller can
> still pass NULL or error_abort as desired.
> 
> And I still think we can probably get Coccinelle to help make the
> conversions, both of using this macro in any function that has an Error
> **errp parameter, as well as getting rid of local_err declarations and
> error_propagate() calls rendered redundant once this macro is used.
> 

Thanks! And sorry for dirty draft.
Eric Blake Sept. 18, 2019, 6:05 p.m. UTC | #3
On 9/18/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:

>>> +/*
>>> + * 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.
>>
>> Why?  I see no reason why it can't be hoisted earlier than other
>> declarations, and the only reason to not sink it after earlier code that
>> doesn't touch errp would be our coding standards that frowns on
>> declaration after code.
> 
> Hmm, I thought compiler would warn about mixing code and definitions.
> Seems that gcc don't care, so it's OK.

C89 required all definitions before code, but that's historical.
Meanwhile, we require a compiler that supports C99 as well as at least
the __attribute__((cleanup)) extension (gcc and clang qualify, nothing
else really does, but no one has been complaining).  And C99 requires
compiler support for intermixing definitions (in part because c++ did it
first, then gcc allowed that as an extension in C89); so it's been
permissible to intermix code and declarations for more than 20 years
now.  The only reason we don't do it more is because of habits and
aesthetics, rather than necessity.

>>
>> This is actually quite cool. 
> 
> I glad to see that you like my favorite variant)
> 
>> And if you get rid of your insistence that
>> it must occur after other variable declarations, you could instead
>> easily automate that any function that has a parameter 'Error **errp'
>> then has a MAKE_ERRP_SAFE(errp); as the first line of its function body
>> (that becomes something that you could grep for, rather than having to
>> use the smarts of coccinelle).
>>
>> Or if we want to enforce consistency on the parameter naming, even go with:
>>
>> #define MAKE_ERRP_SAFE() \
>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>> errp = &__auto_errp_prop.local_err
>>
> 
> I am for

So now to wait for other comments.


>>
>>> +     */
>>> +    MAKE_ERRP_SAFE(errp);
>>> +
>>>       if (filename) {
>>>           ret = qemu_gluster_parse_uri(gconf, filename);
>>>           if (ret < 0) {
>>>
>>
>> This is sweet - you can safely use '*errp' in the rest of the function,
>> without having to remember a second error name - while the caller can
>> still pass NULL or error_abort as desired.
>>
>> And I still think we can probably get Coccinelle to help make the
>> conversions, both of using this macro in any function that has an Error
>> **errp parameter, as well as getting rid of local_err declarations and
>> error_propagate() calls rendered redundant once this macro is used.
>>
> 
> Thanks! And sorry for dirty draft.

It was titled RFC, after all :)
Eric Blake Sept. 18, 2019, 6:32 p.m. UTC | #4
On 9/18/19 1:05 PM, Eric Blake wrote:

>>> #define MAKE_ERRP_SAFE() \
>>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>>> errp = &__auto_errp_prop.local_err
>>>

I tried to see if this could be done with just a single declaration
line, as in:

typedef struct ErrorPropagator {
    Error **errp;
    Error *local_err;
} ErrorPropagator;

#define MAKE_ERRP_SAFE() \
  g_auto(ErrorPropagator) __auto_errp_prop = { \
    errp, ((errp = &__auto_errp_prop.local_err), NULL) }

But sadly, C17 paragraph 6.7.9P23 states:

"The evaluations of the initialization list expressions are
indeterminately sequenced with respect to one another and thus the order
in which any side effects occur is unspecified."

which does not bode well for the assignment to __auto_errp_prop.errp.
All changes to errp would have to be within the same initializer.  Maybe:

#define MAKE_ERRP_SAFE() \
  g_auto(ErrorPropagator) __auto_errp_prop = { \
    .local_err = (__auto_errp_prop.err = errp, \
        (errp = &__auto_errp_prop.local_err), NULL) }

but by the time you get that complicated, just using a statement is
easier to read.
no-reply@patchew.org Sept. 19, 2019, 1:54 a.m. UTC | #5
Patchew URL: https://patchew.org/QEMU/20190918130244.24257-1-vsementsov@virtuozzo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [RFC] error: auto propagated local_err
Message-id: 20190918130244.24257-1-vsementsov@virtuozzo.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
16e8874 error: auto propagated local_err

=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#291: FILE: include/qapi/error.h:391:
+static inline void error_propagation_struct_cleanup(ErrorPropagationStruct *prop)

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#301: FILE: include/qapi/error.h:401:
+#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);

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#320: FILE: include/qapi/error.h:420:
+#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; \
+}

total: 2 errors, 1 warnings, 277 lines checked

Commit 16e8874947e7 (error: auto propagated local_err) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190918130244.24257-1-vsementsov@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 6:47 a.m. UTC | #6
18.09.2019 21:32, Eric Blake wrote:
> On 9/18/19 1:05 PM, Eric Blake wrote:
> 
>>>> #define MAKE_ERRP_SAFE() \
>>>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>>>> errp = &__auto_errp_prop.local_err
>>>>
> 
> I tried to see if this could be done with just a single declaration
> line, as in:
> 
> typedef struct ErrorPropagator {
>      Error **errp;
>      Error *local_err;
> } ErrorPropagator;
> 
> #define MAKE_ERRP_SAFE() \
>    g_auto(ErrorPropagator) __auto_errp_prop = { \
>      errp, ((errp = &__auto_errp_prop.local_err), NULL) }
> 
> But sadly, C17 paragraph 6.7.9P23 states:
> 
> "The evaluations of the initialization list expressions are
> indeterminately sequenced with respect to one another and thus the order
> in which any side effects occur is unspecified."
> 
> which does not bode well for the assignment to __auto_errp_prop.errp.
> All changes to errp would have to be within the same initializer.  Maybe:
> 
> #define MAKE_ERRP_SAFE() \
>    g_auto(ErrorPropagator) __auto_errp_prop = { \
>      .local_err = (__auto_errp_prop.err = errp, \
>          (errp = &__auto_errp_prop.local_err), NULL) }

Is it guaranteed that .errp will not be initialized to NULL after evaluating of
.local_err initializer?

> 
> but by the time you get that complicated, just using a statement is
> easier to read.
>
David Hildenbrand Sept. 19, 2019, 7:32 a.m. UTC | #7
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
> 
> 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).
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
> +    }
> +}
> +
> +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; \
> +}


Using that idea, what about something like this:

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8bfb6684cb..043ad69f8b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
     return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
 }
 
+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);
+    }
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+#define DEFINE_LOCAL_ERRP(_errp) \
+g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \
+Error **local_errp = &__auto_errp_prop.local_err
+
 static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
                               Error **errp)
 {
+    DEFINE_LOCAL_ERRP(errp);
     S390CPU *cpu = S390_CPU(object_new(typename));
-    Error *err = NULL;
 
-    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
-    if (err != NULL) {
+    object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp);
+    if (*local_errp != NULL) {
         goto out;
     }
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+    object_property_set_bool(OBJECT(cpu), true, "realized", local_errp);
 
 out:
     object_unref(OBJECT(cpu));
-    if (err) {
-        error_propagate(errp, err);
+    if (*local_errp) {
         cpu = NULL;
     }
     return cpu;
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 7:41 a.m. UTC | #8
19.09.2019 10:32, David Hildenbrand wrote:
> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>
>> 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).
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
>> +    }
>> +}
>> +
>> +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; \
>> +}
> 
> 
> Using that idea, what about something like this:
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8bfb6684cb..043ad69f8b 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>       return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
>   }
>   
> +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);
> +    }
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +#define DEFINE_LOCAL_ERRP(_errp) \
> +g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \
> +Error **local_errp = &__auto_errp_prop.local_err
> +
>   static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>                                 Error **errp)
>   {
> +    DEFINE_LOCAL_ERRP(errp);
>       S390CPU *cpu = S390_CPU(object_new(typename));
> -    Error *err = NULL;
>   
> -    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
> -    if (err != NULL) {
> +    object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp);
> +    if (*local_errp != NULL) {
>           goto out;
>       }
> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", local_errp);
>   
>   out:
>       object_unref(OBJECT(cpu));
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (*local_errp) {
>           cpu = NULL;
>       }
>       return cpu;
> 
> 

So it's DEF_AUTO_ERRP_V2 with first parameter hardcoded to be local_errp.
I still prefer MAKE_ERRP_SAFE(), to not introduce extra variables.
David Hildenbrand Sept. 19, 2019, 7:53 a.m. UTC | #9
On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:32, David Hildenbrand wrote:
>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>
>>> 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).
>>>
>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
>>> +    }
>>> +}
>>> +
>>> +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; \
>>> +}
>>
>>
>> Using that idea, what about something like this:
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 8bfb6684cb..043ad69f8b 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>       return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
>>   }
>>   
>> +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);
>> +    }
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>> +#define DEFINE_LOCAL_ERRP(_errp) \
>> +g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \
>> +Error **local_errp = &__auto_errp_prop.local_err
>> +
>>   static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>>                                 Error **errp)
>>   {
>> +    DEFINE_LOCAL_ERRP(errp);
>>       S390CPU *cpu = S390_CPU(object_new(typename));
>> -    Error *err = NULL;
>>   
>> -    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
>> -    if (err != NULL) {
>> +    object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp);
>> +    if (*local_errp != NULL) {
>>           goto out;
>>       }
>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>> +    object_property_set_bool(OBJECT(cpu), true, "realized", local_errp);
>>   
>>   out:
>>       object_unref(OBJECT(cpu));
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (*local_errp) {
>>           cpu = NULL;
>>       }
>>       return cpu;
>>
>>
> 
> So it's DEF_AUTO_ERRP_V2 with first parameter hardcoded to be local_errp.
> I still prefer MAKE_ERRP_SAFE(), to not introduce extra variables.
> 

I lost track of the different approaches ;)

The local variable will most probably optimized out by the compiler. I
dislike MAKE_ERRP_SAFE(), as it mixes defining a new variable with code.
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 8:20 a.m. UTC | #10
19.09.2019 10:53, David Hildenbrand wrote:
> On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
>> 19.09.2019 10:32, David Hildenbrand wrote:
>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>>
>>>> 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).
>>>>
>>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
>>>> +    }
>>>> +}
>>>> +
>>>> +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; \
>>>> +}
>>>
>>>
>>> Using that idea, what about something like this:
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 8bfb6684cb..043ad69f8b 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>>        return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
>>>    }
>>>    
>>> +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);
>>> +    }
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>>> +
>>> +#define DEFINE_LOCAL_ERRP(_errp) \
>>> +g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \
>>> +Error **local_errp = &__auto_errp_prop.local_err
>>> +
>>>    static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>>>                                  Error **errp)
>>>    {
>>> +    DEFINE_LOCAL_ERRP(errp);
>>>        S390CPU *cpu = S390_CPU(object_new(typename));
>>> -    Error *err = NULL;
>>>    
>>> -    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
>>> -    if (err != NULL) {
>>> +    object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp);
>>> +    if (*local_errp != NULL) {
>>>            goto out;
>>>        }
>>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>>> +    object_property_set_bool(OBJECT(cpu), true, "realized", local_errp);
>>>    
>>>    out:
>>>        object_unref(OBJECT(cpu));
>>> -    if (err) {
>>> -        error_propagate(errp, err);
>>> +    if (*local_errp) {
>>>            cpu = NULL;
>>>        }
>>>        return cpu;
>>>
>>>
>>
>> So it's DEF_AUTO_ERRP_V2 with first parameter hardcoded to be local_errp.
>> I still prefer MAKE_ERRP_SAFE(), to not introduce extra variables.
>>
> 
> I lost track of the different approaches ;)
> 
> The local variable will most probably optimized out by the compiler. I
> dislike MAKE_ERRP_SAFE(), as it mixes defining a new variable with code.
> 

But it makes Greg's series extremely simple: just add MAKE_ERRP_SAFE() to some
functions. And as Eric explains, mixing code and definitions is not a problem.

Still, we can do like this:

#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = errp}; \
Error **__local_errp_unused __attribute__ ((unused)) = (errp = &__auto_errp_prop.local_err)

Which are two valid definitions.
David Hildenbrand Sept. 19, 2019, 8:23 a.m. UTC | #11
On 19.09.19 10:20, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 10:53, David Hildenbrand wrote:
>> On 19.09.19 09:41, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.09.2019 10:32, David Hildenbrand wrote:
>>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>>>
>>>>> 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).
>>>>>
>>>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +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; \
>>>>> +}
>>>>
>>>>
>>>> Using that idea, what about something like this:
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 8bfb6684cb..043ad69f8b 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -58,22 +58,42 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>>>        return S390_CPU(ms->possible_cpus->cpus[cpu_addr].cpu);
>>>>    }
>>>>    
>>>> +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);
>>>> +    }
>>>> +}
>>>> +
>>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>>>> +
>>>> +#define DEFINE_LOCAL_ERRP(_errp) \
>>>> +g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = (_errp)}; \
>>>> +Error **local_errp = &__auto_errp_prop.local_err
>>>> +
>>>>    static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
>>>>                                  Error **errp)
>>>>    {
>>>> +    DEFINE_LOCAL_ERRP(errp);
>>>>        S390CPU *cpu = S390_CPU(object_new(typename));
>>>> -    Error *err = NULL;
>>>>    
>>>> -    object_property_set_int(OBJECT(cpu), core_id, "core-id", &err);
>>>> -    if (err != NULL) {
>>>> +    object_property_set_int(OBJECT(cpu), core_id, "core-id", local_errp);
>>>> +    if (*local_errp != NULL) {
>>>>            goto out;
>>>>        }
>>>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>>>> +    object_property_set_bool(OBJECT(cpu), true, "realized", local_errp);
>>>>    
>>>>    out:
>>>>        object_unref(OBJECT(cpu));
>>>> -    if (err) {
>>>> -        error_propagate(errp, err);
>>>> +    if (*local_errp) {
>>>>            cpu = NULL;
>>>>        }
>>>>        return cpu;
>>>>
>>>>
>>>
>>> So it's DEF_AUTO_ERRP_V2 with first parameter hardcoded to be local_errp.
>>> I still prefer MAKE_ERRP_SAFE(), to not introduce extra variables.
>>>
>>
>> I lost track of the different approaches ;)
>>
>> The local variable will most probably optimized out by the compiler. I
>> dislike MAKE_ERRP_SAFE(), as it mixes defining a new variable with code.
>>
> 
> But it makes Greg's series extremely simple: just add MAKE_ERRP_SAFE() to some
> functions. And as Eric explains, mixing code and definitions is not a problem.

I still dislike it ;)

> 
> Still, we can do like this:
> 
> #define MAKE_ERRP_SAFE() \
> g_auto(ErrorPropagator) (__auto_errp_prop) = {.errp = errp}; \
> Error **__local_errp_unused __attribute__ ((unused)) = (errp = &__auto_errp_prop.local_err)
> 
> Which are two valid definitions.

Yeah, I would prefer something like that!
Greg Kurz Sept. 19, 2019, 8:59 a.m. UTC | #12
On Wed, 18 Sep 2019 16:02:44 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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) {
Max Reitz Sept. 19, 2019, 8:59 a.m. UTC | #13
On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
> 
> 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).
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)

If the combination of “if (local_err) { error_propagate(...); ... }” is
what’s cumbersome, can’t this be done simpler by adding an
error_propagate() variant with a return value?

i.e.

bool has_error_then_propagate(Error **errp, Error *err)
{
    if (!err) {
        return false;
    }
    error_propagate(errp, err);
    return true;
}

And then turn all instances of

if (local_err) {
    error_propagate(errp, local_err);
    ...
}

into

if (has_error_then_propagate(errp, local_err)) {
    ...
}

?

Max
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 9:14 a.m. UTC | #14
19.09.2019 11:59, Max Reitz wrote:
> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>
>> 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).
>>
>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)
> 
> If the combination of “if (local_err) { error_propagate(...); ... }” is
> what’s cumbersome, can’t this be done simpler by adding an
> error_propagate() variant with a return value?
> 
> i.e.
> 
> bool has_error_then_propagate(Error **errp, Error *err)
> {
>      if (!err) {
>          return false;
>      }
>      error_propagate(errp, err);
>      return true;
> }
> 
> And then turn all instances of
> 
> if (local_err) {
>      error_propagate(errp, local_err);
>      ...
> }
> 
> into
> 
> if (has_error_then_propagate(errp, local_err)) {
>      ...
> }
> 
> ?
> 
> Max
> 

No, originally cumbersome is introducing local_err in a lot of new places by
Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
instead (in each function where we need local_err).

Also, auto-propagation seems correct thing to do, which fits good into
g_auto concept, so even without any macro it just allows to drop several error_propagate
calls (or may be several goto statements to do one error_propagate call) into
one definitions. It's the same story like with g_autofree vs g_free.
Kevin Wolf Sept. 19, 2019, 9:17 a.m. UTC | #15
Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> > + */
> > +#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; \
> > +}
> 
> Not written to take a trailing semicolon in the caller.
> 
> You could even set __auto_errp_prop unconditionally rather than trying
> to reuse incoming errp (the difference being that error_propagate() gets
> called more frequently).

I think this difference is actually a problem.

When debugging things, I hate error_propagate(). It means that the Error
(specifically its fields src/func/line) points to the outermost
error_propagate() rather than the place where the error really happened.
It also makes error_abort completely useless because at the point where
the process gets aborted, the interesting information is already lost.

So I'd really like to restrict the use of error_propagate() to places
where it's absolutely necessary. Unless, of course, you can fix these
practical problems that error_propagate() causes for debugging.

In fact, in the context of Greg's series, I think we really only need to
support hints for error_fatal, which are cases that users are supposed
to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
are things that are never supposed to happen. A good stack trace is more
important there than adding a hint to the message.

Kevin
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 9:28 a.m. UTC | #16
19.09.2019 11:59, Greg Kurz wrote:
> On Wed, 18 Sep 2019 16:02:44 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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) {
>
Max Reitz Sept. 19, 2019, 9:33 a.m. UTC | #17
On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 11:59, Max Reitz wrote:
>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>
>>> 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).
>>>
>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)
>>
>> If the combination of “if (local_err) { error_propagate(...); ... }” is
>> what’s cumbersome, can’t this be done simpler by adding an
>> error_propagate() variant with a return value?
>>
>> i.e.
>>
>> bool has_error_then_propagate(Error **errp, Error *err)
>> {
>>      if (!err) {
>>          return false;
>>      }
>>      error_propagate(errp, err);
>>      return true;
>> }
>>
>> And then turn all instances of
>>
>> if (local_err) {
>>      error_propagate(errp, local_err);
>>      ...
>> }
>>
>> into
>>
>> if (has_error_then_propagate(errp, local_err)) {
>>      ...
>> }
>>
>> ?
>>
>> Max
>>
> 
> No, originally cumbersome is introducing local_err in a lot of new places by
> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
> instead (in each function where we need local_err).

Does it need more than one local_err per function?

Because if it didn’t, I’d find one “Error *local_err;” simpler than one
macro incantation.

(It has the same LoC, and it makes code readers ask the same question:
“Why do we need it?”  Which has the same answer for both; but one is
immediately readable code, whereas the other is a macro.)

> Also, auto-propagation seems correct thing to do, which fits good into
> g_auto concept, so even without any macro it just allows to drop several error_propagate
> calls (or may be several goto statements to do one error_propagate call) into
> one definitions. It's the same story like with g_autofree vs g_free.

I don’t see the advantage here.  You have to do the if () statement
anyway, so it isn’t like you’re saving any LoC.  In addition, I
personally don’t find code hidden through __attribute__((cleanup))
easier to understand than explicitly written code.

It isn’t like I don’t like __attribute__((cleanup)).  But it does count
under “magic” in my book, and thus I’d avoid it if it doesn’t bring
actual advantages.  (It does bring actual advantages for things like
auto-freeing memory, auto-closing FDs, or zeroing secret buffers before
freeing them.  In all those cases, you save LoC, and you prevent
accidental leakage.  I don’t quite see such advantages here, but I may
well be mistaken.)


Now Kevin has given an actual advantage, which is that local_err
complicates debugging.  I’ve never had that problem myself, but that
would indeed be an advantage that may warrant some magic.

Max
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 9:47 a.m. UTC | #18
19.09.2019 12:17, Kevin Wolf wrote:
> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> + */
>>> +#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; \
>>> +}
>>
>> Not written to take a trailing semicolon in the caller.
>>
>> You could even set __auto_errp_prop unconditionally rather than trying
>> to reuse incoming errp (the difference being that error_propagate() gets
>> called more frequently).
> 
> I think this difference is actually a problem.
> 
> When debugging things, I hate error_propagate(). It means that the Error
> (specifically its fields src/func/line) points to the outermost
> error_propagate() rather than the place where the error really happened.

Hmm, never tested it, but looking at the code I can't understand how can that
be. src/func/line are set in error_setg.. and in error_propagate() we just
set the errp of the caller, src/func/line unchanged.

Still, I see that these useful fields are printed only for error_abort, for
which we usually have coredump, which provides a lot more information.

> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.

Aha, understand this point, error_abort just don't work as desired, if
we swap it by local_err. And we can fix it by using macro: never create
local_err for error_abort, let it abort exactly on error_setg.

> 
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
> 
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.
> 

Agreed
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 10:03 a.m. UTC | #19
19.09.2019 12:33, Max Reitz wrote:
> On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
>> 19.09.2019 11:59, Max Reitz wrote:
>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>>
>>>> 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).
>>>>
>>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)
>>>
>>> If the combination of “if (local_err) { error_propagate(...); ... }” is
>>> what’s cumbersome, can’t this be done simpler by adding an
>>> error_propagate() variant with a return value?
>>>
>>> i.e.
>>>
>>> bool has_error_then_propagate(Error **errp, Error *err)
>>> {
>>>       if (!err) {
>>>           return false;
>>>       }
>>>       error_propagate(errp, err);
>>>       return true;
>>> }
>>>
>>> And then turn all instances of
>>>
>>> if (local_err) {
>>>       error_propagate(errp, local_err);
>>>       ...
>>> }
>>>
>>> into
>>>
>>> if (has_error_then_propagate(errp, local_err)) {
>>>       ...
>>> }
>>>
>>> ?
>>>
>>> Max
>>>
>>
>> No, originally cumbersome is introducing local_err in a lot of new places by
>> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
>> instead (in each function where we need local_err).
> 
> Does it need more than one local_err per function?
> 
> Because if it didn’t, I’d find one “Error *local_err;” simpler than one
> macro incantation.
> 
> (It has the same LoC, and it makes code readers ask the same question:
> “Why do we need it?”  Which has the same answer for both; but one is
> immediately readable code, whereas the other is a macro.)

Not the same, you didn't count error_propagate

And your example don't match Greg's case, there no "if (local_err)" in it,
just "error_append_hint(errp)", which don't work for error_fatal and error_abort
(Yes, I agree with Kevin, that it should work only for error_fatal)

> 
>> Also, auto-propagation seems correct thing to do, which fits good into
>> g_auto concept, so even without any macro it just allows to drop several error_propagate
>> calls (or may be several goto statements to do one error_propagate call) into
>> one definitions. It's the same story like with g_autofree vs g_free.
> 
> I don’t see the advantage here.  You have to do the if () statement
> anyway, so it isn’t like you’re saving any LoC.  In addition, I
> personally don’t find code hidden through __attribute__((cleanup))
> easier to understand than explicitly written code.
> 
> It isn’t like I don’t like __attribute__((cleanup)).  But it does count
> under “magic” in my book, and thus I’d avoid it if it doesn’t bring
> actual advantages.  (It does bring actual advantages for things like
> auto-freeing memory, auto-closing FDs, or zeroing secret buffers before
> freeing them.  In all those cases, you save LoC, and you prevent
> accidental leakage.  I don’t quite see such advantages here, but I may
> well be mistaken.)
> 
> 
> Now Kevin has given an actual advantage, which is that local_err
> complicates debugging.  I’ve never had that problem myself, but that
> would indeed be an advantage that may warrant some magic.
> 
> Max
>
Greg Kurz Sept. 19, 2019, 10:08 a.m. UTC | #20
On Thu, 19 Sep 2019 09:28:11 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 19.09.2019 11:59, Greg Kurz wrote:
> > On Wed, 18 Sep 2019 16:02:44 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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) {
> > 
> 
>
Daniel P. Berrangé Sept. 19, 2019, 10:09 a.m. UTC | #21
On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote:
> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> > On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > + */
> > > +#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; \
> > > +}
> > 
> > Not written to take a trailing semicolon in the caller.
> > 
> > You could even set __auto_errp_prop unconditionally rather than trying
> > to reuse incoming errp (the difference being that error_propagate() gets
> > called more frequently).
> 
> I think this difference is actually a problem.
> 
> When debugging things, I hate error_propagate(). It means that the Error
> (specifically its fields src/func/line) points to the outermost
> error_propagate() rather than the place where the error really happened.
> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.

Yeah, that is very frustrating. For personal development you can eventually
figure it out, but with customer support requests, all you get is the
stack trace and you've no core file to investigate, so you're stuck.
IOW, as a general principle, we should strive to minimize the usage
of error propagate.

The key reason why we typically need the propagate calls, is because
we allow the passed in Error **errp parameter to be NULL, while at the
same time we want to inspect it to see if its contents are non-NULL
to detect failure. I venture to suggest that this is flawed API
design on our part. We should not need to inspect the error object
to see if a method call fails - the return value can be used for
this purpose.

IOW, instead of this pattern with typically 'void' methods

     extern void somemethod(Error **errp);

     void thismethod(Error **errp) {
        Error *local_err = NULL;
	...
        somemethod(&local_err);
        if (local_err) {
	    error_propagate(errp, local_err);
	    return;
	}
        ...
     }

We should be preferring

     extern int somemethod(Error **errp);

     int thismethod(Error **errp) {
	...
        if (somemethod(errp) < 0) {
	    return -1;
	}
        ...
     }

ie only using the Error object to bubble up the *details* of
the error, not as the mechanism for finding if an error occurred.

There is of course a downside with this approach, in that a
method might set 'errp' but return 0, or not set 'errp' but
return -1. I think this is outweighed by the simpler code
pattern overall though.


Regards,
Daniel
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 10:21 a.m. UTC | #22
19.09.2019 13:09, Daniel P. Berrangé wrote:
> On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote:
>> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> + */
>>>> +#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; \
>>>> +}
>>>
>>> Not written to take a trailing semicolon in the caller.
>>>
>>> You could even set __auto_errp_prop unconditionally rather than trying
>>> to reuse incoming errp (the difference being that error_propagate() gets
>>> called more frequently).
>>
>> I think this difference is actually a problem.
>>
>> When debugging things, I hate error_propagate(). It means that the Error
>> (specifically its fields src/func/line) points to the outermost
>> error_propagate() rather than the place where the error really happened.
>> It also makes error_abort completely useless because at the point where
>> the process gets aborted, the interesting information is already lost.
> 
> Yeah, that is very frustrating. For personal development you can eventually
> figure it out, but with customer support requests, all you get is the
> stack trace and you've no core file to investigate, so you're stuck.
> IOW, as a general principle, we should strive to minimize the usage
> of error propagate.
> 
> The key reason why we typically need the propagate calls, is because
> we allow the passed in Error **errp parameter to be NULL, while at the
> same time we want to inspect it to see if its contents are non-NULL
> to detect failure. I venture to suggest that this is flawed API
> design on our part. We should not need to inspect the error object
> to see if a method call fails - the return value can be used for
> this purpose.
> 
> IOW, instead of this pattern with typically 'void' methods
> 
>       extern void somemethod(Error **errp);
> 
>       void thismethod(Error **errp) {
>          Error *local_err = NULL;
> 	...
>          somemethod(&local_err);
>          if (local_err) {
> 	    error_propagate(errp, local_err);
> 	    return;
> 	}
>          ...
>       }
> 
> We should be preferring
> 
>       extern int somemethod(Error **errp);
> 
>       int thismethod(Error **errp) {
> 	...
>          if (somemethod(errp) < 0) {
> 	    return -1;
> 	}
>          ...
>       }
> 
> ie only using the Error object to bubble up the *details* of
> the error, not as the mechanism for finding if an error occurred.
> 
> There is of course a downside with this approach, in that a
> method might set 'errp' but return 0, or not set 'errp' but
> return -1. I think this is outweighed by the simpler code
> pattern overall though.
> 
> 

Agree. But seems that such change may be only done by hand.. Hmm, on the other hand,
why not. May be I'll try do it for some parts of block layer.

Still, error_append_hint needs local_err in case of error_fatal.
Max Reitz Sept. 19, 2019, 11:52 a.m. UTC | #23
On 19.09.19 12:03, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 12:33, Max Reitz wrote:
>> On 19.09.19 11:14, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.09.2019 11:59, Max Reitz wrote:
>>>> On 18.09.19 15:02, Vladimir Sementsov-Ogievskiy 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.
>>>>>
>>>>> 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).
>>>>>
>>>>> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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(-)
>>>>
>>>> If the combination of “if (local_err) { error_propagate(...); ... }” is
>>>> what’s cumbersome, can’t this be done simpler by adding an
>>>> error_propagate() variant with a return value?
>>>>
>>>> i.e.
>>>>
>>>> bool has_error_then_propagate(Error **errp, Error *err)
>>>> {
>>>>       if (!err) {
>>>>           return false;
>>>>       }
>>>>       error_propagate(errp, err);
>>>>       return true;
>>>> }
>>>>
>>>> And then turn all instances of
>>>>
>>>> if (local_err) {
>>>>       error_propagate(errp, local_err);
>>>>       ...
>>>> }
>>>>
>>>> into
>>>>
>>>> if (has_error_then_propagate(errp, local_err)) {
>>>>       ...
>>>> }
>>>>
>>>> ?
>>>>
>>>> Max
>>>>
>>>
>>> No, originally cumbersome is introducing local_err in a lot of new places by
>>> Greg's series. MAKE_ERRP_SAFE macro makes it as simple as one macro call
>>> instead (in each function where we need local_err).
>>
>> Does it need more than one local_err per function?
>>
>> Because if it didn’t, I’d find one “Error *local_err;” simpler than one
>> macro incantation.
>>
>> (It has the same LoC, and it makes code readers ask the same question:
>> “Why do we need it?”  Which has the same answer for both; but one is
>> immediately readable code, whereas the other is a macro.)
> 
> Not the same, you didn't count error_propagate

I did, because it would part of the if () statement in my proposal.

> And your example don't match Greg's case, there no "if (local_err)" in it,

Ah, right, I see.  OK then.

> just "error_append_hint(errp)", which don't work for error_fatal and error_abort
> (Yes, I agree with Kevin, that it should work only for error_fatal)

True.

[...]

>> Now Kevin has given an actual advantage, which is that local_err
>> complicates debugging.  I’ve never had that problem myself, but that
>> would indeed be an advantage that may warrant some magic.

Although after some more consideration I realized this probably cannot
be achieved with this series, actually.

Max
Daniel P. Berrangé Sept. 19, 2019, 11:55 a.m. UTC | #24
On Thu, Sep 19, 2019 at 10:21:44AM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 13:09, Daniel P. Berrangé wrote:
> > On Thu, Sep 19, 2019 at 11:17:20AM +0200, Kevin Wolf wrote:
> >> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> >>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>>> + */
> >>>> +#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; \
> >>>> +}
> >>>
> >>> Not written to take a trailing semicolon in the caller.
> >>>
> >>> You could even set __auto_errp_prop unconditionally rather than trying
> >>> to reuse incoming errp (the difference being that error_propagate() gets
> >>> called more frequently).
> >>
> >> I think this difference is actually a problem.
> >>
> >> When debugging things, I hate error_propagate(). It means that the Error
> >> (specifically its fields src/func/line) points to the outermost
> >> error_propagate() rather than the place where the error really happened.
> >> It also makes error_abort completely useless because at the point where
> >> the process gets aborted, the interesting information is already lost.
> > 
> > Yeah, that is very frustrating. For personal development you can eventually
> > figure it out, but with customer support requests, all you get is the
> > stack trace and you've no core file to investigate, so you're stuck.
> > IOW, as a general principle, we should strive to minimize the usage
> > of error propagate.
> > 
> > The key reason why we typically need the propagate calls, is because
> > we allow the passed in Error **errp parameter to be NULL, while at the
> > same time we want to inspect it to see if its contents are non-NULL
> > to detect failure. I venture to suggest that this is flawed API
> > design on our part. We should not need to inspect the error object
> > to see if a method call fails - the return value can be used for
> > this purpose.
> > 
> > IOW, instead of this pattern with typically 'void' methods
> > 
> >       extern void somemethod(Error **errp);
> > 
> >       void thismethod(Error **errp) {
> >          Error *local_err = NULL;
> > 	...
> >          somemethod(&local_err);
> >          if (local_err) {
> > 	    error_propagate(errp, local_err);
> > 	    return;
> > 	}
> >          ...
> >       }
> > 
> > We should be preferring
> > 
> >       extern int somemethod(Error **errp);
> > 
> >       int thismethod(Error **errp) {
> > 	...
> >          if (somemethod(errp) < 0) {
> > 	    return -1;
> > 	}
> >          ...
> >       }
> > 
> > ie only using the Error object to bubble up the *details* of
> > the error, not as the mechanism for finding if an error occurred.
> > 
> > There is of course a downside with this approach, in that a
> > method might set 'errp' but return 0, or not set 'errp' but
> > return -1. I think this is outweighed by the simpler code
> > pattern overall though.
> > 
> > 
> 
> Agree. But seems that such change may be only done by hand.. Hmm, on the other hand,
> why not. May be I'll try do it for some parts of block layer.
> 
> Still, error_append_hint needs local_err in case of error_fatal.

Yep, fortunately that usage is comparatively rare vs use of error_propagate
in general.

To be clear I don't have any objections to your overall idea of using
attribute cleanup to simplify error propagation. As you say, it can
still be useful for the error_append_hint, even if we use the code
pattern I suggest more widely to eliminate propagation where possible.

Regards,
Daniel
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, noon UTC | #25
19.09.2019 12:17, Kevin Wolf wrote:
> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> + */
>>> +#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; \
>>> +}
>>
>> Not written to take a trailing semicolon in the caller.
>>
>> You could even set __auto_errp_prop unconditionally rather than trying
>> to reuse incoming errp (the difference being that error_propagate() gets
>> called more frequently).
> 
> I think this difference is actually a problem.
> 
> When debugging things, I hate error_propagate(). It means that the Error
> (specifically its fields src/func/line) points to the outermost
> error_propagate() rather than the place where the error really happened.
> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.
> 
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
> 
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.
> 

Interesting, that to handle error_append_hint problem, we don't need to
create local_err in case of errp==NULL either..

So, possibly, we need the following steps:

1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == error_fatal" in the if condition
2. rebase Greg's series on it, to fix hints for fatal errors
3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == NULL" in the if condition
4. convert all (almost all) local_err usage to use MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
    fix problem with error_abort (and also drop a lot of calls of error_propagate)
5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop MAKE_ERRP_SAFE_FOR_DEREFERENCE()
    magic, together with dereferencing.
Kevin Wolf Sept. 19, 2019, 1:03 p.m. UTC | #26
Am 19.09.2019 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.09.2019 12:17, Kevin Wolf wrote:
> > Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> >> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>> + */
> >>> +#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; \
> >>> +}
> >>
> >> Not written to take a trailing semicolon in the caller.
> >>
> >> You could even set __auto_errp_prop unconditionally rather than trying
> >> to reuse incoming errp (the difference being that error_propagate() gets
> >> called more frequently).
> > 
> > I think this difference is actually a problem.
> > 
> > When debugging things, I hate error_propagate(). It means that the Error
> > (specifically its fields src/func/line) points to the outermost
> > error_propagate() rather than the place where the error really happened.
> > It also makes error_abort completely useless because at the point where
> > the process gets aborted, the interesting information is already lost.
> > 
> > So I'd really like to restrict the use of error_propagate() to places
> > where it's absolutely necessary. Unless, of course, you can fix these
> > practical problems that error_propagate() causes for debugging.
> > 
> > In fact, in the context of Greg's series, I think we really only need to
> > support hints for error_fatal, which are cases that users are supposed
> > to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> > are things that are never supposed to happen. A good stack trace is more
> > important there than adding a hint to the message.
> > 
> 
> Interesting, that to handle error_append_hint problem, we don't need to
> create local_err in case of errp==NULL either..
> 
> So, possibly, we need the following steps:
> 
> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == error_fatal" in the if condition
> 2. rebase Greg's series on it, to fix hints for fatal errors
> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == NULL" in the if condition
> 4. convert all (almost all) local_err usage to use MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
>     fix problem with error_abort (and also drop a lot of calls of error_propagate)
> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop MAKE_ERRP_SAFE_FOR_DEREFERENCE()
>     magic, together with dereferencing.

Long macro names, but as the parameter will always only be "errp", it
fits easily on a line, so this is fine.

I think I like this plan.

Kevin
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 1:17 p.m. UTC | #27
19.09.2019 16:03, Kevin Wolf wrote:
> Am 19.09.2019 um 14:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 19.09.2019 12:17, Kevin Wolf wrote:
>>> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>>>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> + */
>>>>> +#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; \
>>>>> +}
>>>>
>>>> Not written to take a trailing semicolon in the caller.
>>>>
>>>> You could even set __auto_errp_prop unconditionally rather than trying
>>>> to reuse incoming errp (the difference being that error_propagate() gets
>>>> called more frequently).
>>>
>>> I think this difference is actually a problem.
>>>
>>> When debugging things, I hate error_propagate(). It means that the Error
>>> (specifically its fields src/func/line) points to the outermost
>>> error_propagate() rather than the place where the error really happened.
>>> It also makes error_abort completely useless because at the point where
>>> the process gets aborted, the interesting information is already lost.
>>>
>>> So I'd really like to restrict the use of error_propagate() to places
>>> where it's absolutely necessary. Unless, of course, you can fix these
>>> practical problems that error_propagate() causes for debugging.
>>>
>>> In fact, in the context of Greg's series, I think we really only need to
>>> support hints for error_fatal, which are cases that users are supposed
>>> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
>>> are things that are never supposed to happen. A good stack trace is more
>>> important there than adding a hint to the message.
>>>
>>
>> Interesting, that to handle error_append_hint problem, we don't need to
>> create local_err in case of errp==NULL either..
>>
>> So, possibly, we need the following steps:
>>
>> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == error_fatal" in the if condition
>> 2. rebase Greg's series on it, to fix hints for fatal errors
>> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == NULL" in the if condition
>> 4. convert all (almost all) local_err usage to use MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
>>      fix problem with error_abort (and also drop a lot of calls of error_propagate)
>> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop MAKE_ERRP_SAFE_FOR_DEREFERENCE()
>>      magic, together with dereferencing.
> 
> Long macro names, but as the parameter will always only be "errp", it
> fits easily on a line, so this is fine.

Yes, I wanted to stress their meaning in plan..

Other variants, I can imagine:

MAKE_ERRP_SAFE_FOR_DEREFERENCE
WRAP_ERRP_FOR_DEREFERENCE
WRAP_NULL_ERRP

MAKE_ERRP_SAFE_FOR_HINT
WRAP_ERRP_FOR_HINT
WRAP_FATAL_ERRP


> 
> I think I like this plan.
> 
> Kevin
>
Eric Blake Sept. 19, 2019, 1:29 p.m. UTC | #28
On 9/19/19 1:47 AM, Vladimir Sementsov-Ogievskiy wrote:

>> "The evaluations of the initialization list expressions are
>> indeterminately sequenced with respect to one another and thus the order
>> in which any side effects occur is unspecified."
>>
>> which does not bode well for the assignment to __auto_errp_prop.errp.
>> All changes to errp would have to be within the same initializer.  Maybe:
>>
>> #define MAKE_ERRP_SAFE() \
>>    g_auto(ErrorPropagator) __auto_errp_prop = { \
>>      .local_err = (__auto_errp_prop.err = errp, \
>>          (errp = &__auto_errp_prop.local_err), NULL) }
> 
> Is it guaranteed that .errp will not be initialized to NULL after evaluating of
> .local_err initializer?

Probably not.

> 
>>
>> but by the time you get that complicated, just using a statement is
>> easier to read.

Either two declarations (the second being an unused dummy variable
declared solely for its initializer's side-effects) or a declaration and
a statement are the only sane ways I can see to provide guaranteed
ordering.  It's hidden behind a macro, so I don't care which.
Eric Blake Sept. 19, 2019, 1:40 p.m. UTC | #29
On 9/19/19 4:17 AM, Kevin Wolf wrote:
> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> + */
>>> +#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; \
>>> +}
>>
>> Not written to take a trailing semicolon in the caller.
>>
>> You could even set __auto_errp_prop unconditionally rather than trying
>> to reuse incoming errp (the difference being that error_propagate() gets
>> called more frequently).
> 
> I think this difference is actually a problem.
> 
> When debugging things, I hate error_propagate(). It means that the Error
> (specifically its fields src/func/line) points to the outermost
> error_propagate() rather than the place where the error really happened.
> It also makes error_abort completely useless because at the point where
> the process gets aborted, the interesting information is already lost.

Okay, based on that, I see the following desirable semantics:

Caller: one of 4 calling paradigms:

pass errp=NULL (we don't care about failures)
pass errp=&error_abort (we want to abort() as soon as possible as close
to the real problem as possible)
pass errp=&error_fatal (we want to exit(), but only after collecting as
much information as possible)
pass errp = anything else (we are collecting an error for other reasons,
we may report it or let the caller decide or ...)

Callee: we want a SINGLE paradigm:

func (Error **errp)
{
    MAKE_ERRP_SAFE();

    now we can pass errp to any child function, test '*errp', or do
anything else, and we DON'T have to call error_propagate.

I think that means we need:

#define MAKE_ERRP_SAFE() \
  g_auto(...) __auto_errp = { .errp = errp }; \
  do { \
    if (!errp || errp == &error_fatal) { errp = &__auto_errp.local; } \
  } while (0)

So back to the caller semantics:

if the caller passed NULL, we've redirected errp locally so that we can
use *errp at will; the auto-cleanup will free our local error.

if the caller passed &error_abort, we keep errp unchanged.  *errp tests
will never trigger, because we'll have already aborted in the child on
the original errp, giving developers the best stack trace.

if the caller passed &error_fatal, we redirect errp.  auto-cleanup will
then error_propagate that back to the caller, producing as much nice
information as possible.

if the caller passed anything else, we keep errp unchanged, so no extra
error_propagate in the mix.

> 
> So I'd really like to restrict the use of error_propagate() to places
> where it's absolutely necessary. Unless, of course, you can fix these
> practical problems that error_propagate() causes for debugging.
> 
> In fact, in the context of Greg's series, I think we really only need to
> support hints for error_fatal, which are cases that users are supposed
> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> are things that are never supposed to happen. A good stack trace is more
> important there than adding a hint to the message.

We also want to handle the caller passing NULL, so that we no longer
have to introduce 'Error *local_error = NULL' everywhere.
Eric Blake Sept. 19, 2019, 1:44 p.m. UTC | #30
On 9/19/19 8:03 AM, Kevin Wolf wrote:

>>
>> Interesting, that to handle error_append_hint problem, we don't need to
>> create local_err in case of errp==NULL either..
>>
>> So, possibly, we need the following steps:
>>
>> 1. implement MAKE_ERRP_SAFE_FOR_HINT (which only leave "*(errp) == error_fatal" in the if condition
>> 2. rebase Greg's series on it, to fix hints for fatal errors
>> 3. implement MAKE_ERRP_SAFE_FOR_DEREFERENCE (which only leave "(errp) == NULL" in the if condition
>> 4. convert all (almost all) local_err usage to use MAKE_ERRP_SAFE_FOR_DEREFERENCE, which will
>>     fix problem with error_abort (and also drop a lot of calls of error_propagate)

Why do we need two macros?  A single MAKE_ERRP_SAFE that covers both
NULL and &error_fatal at the same time is sufficient.  You can then
always use append_hint and/or *errp as you wish, and the
error_propagate() call is only used in two situations: caller passed
NULL (so we free the error as ignored), caller passed &error_fatal (so
the hint got appended before we propagate it to flag a more useful message).

>> 5. merely convert "void func(.., errp)" to "int func(.., errp)" and drop MAKE_ERRP_SAFE_FOR_DEREFERENCE()
>>     magic, together with dereferencing.

That's orthogonal, but may also be useful cleanups.  I don't think we
need to drop the macro, though.

> 
> Long macro names, but as the parameter will always only be "errp", it
> fits easily on a line, so this is fine.
> 
> I think I like this plan.
> 
> Kevin
>
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 2:13 p.m. UTC | #31
19.09.2019 16:40, Eric Blake wrote:
> On 9/19/19 4:17 AM, Kevin Wolf wrote:
>> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> + */
>>>> +#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; \
>>>> +}
>>>
>>> Not written to take a trailing semicolon in the caller.
>>>
>>> You could even set __auto_errp_prop unconditionally rather than trying
>>> to reuse incoming errp (the difference being that error_propagate() gets
>>> called more frequently).
>>
>> I think this difference is actually a problem.
>>
>> When debugging things, I hate error_propagate(). It means that the Error
>> (specifically its fields src/func/line) points to the outermost
>> error_propagate() rather than the place where the error really happened.
>> It also makes error_abort completely useless because at the point where
>> the process gets aborted, the interesting information is already lost.
> 
> Okay, based on that, I see the following desirable semantics:
> 
> Caller: one of 4 calling paradigms:
> 
> pass errp=NULL (we don't care about failures)
> pass errp=&error_abort (we want to abort() as soon as possible as close
> to the real problem as possible)
> pass errp=&error_fatal (we want to exit(), but only after collecting as
> much information as possible)
> pass errp = anything else (we are collecting an error for other reasons,
> we may report it or let the caller decide or ...)
> 
> Callee: we want a SINGLE paradigm:
> 
> func (Error **errp)
> {
>      MAKE_ERRP_SAFE();
> 
>      now we can pass errp to any child function, test '*errp', or do
> anything else, and we DON'T have to call error_propagate.
> 
> I think that means we need:
> 
> #define MAKE_ERRP_SAFE() \
>    g_auto(...) __auto_errp = { .errp = errp }; \
>    do { \
>      if (!errp || errp == &error_fatal) { errp = &__auto_errp.local; } \
>    } while (0)
> 
> So back to the caller semantics:
> 
> if the caller passed NULL, we've redirected errp locally so that we can
> use *errp at will; the auto-cleanup will free our local error.
> 
> if the caller passed &error_abort, we keep errp unchanged.  *errp tests
> will never trigger, because we'll have already aborted in the child on
> the original errp, giving developers the best stack trace.
> 
> if the caller passed &error_fatal, we redirect errp.  auto-cleanup will
> then error_propagate that back to the caller, producing as much nice
> information as possible.
> 
> if the caller passed anything else, we keep errp unchanged, so no extra
> error_propagate in the mix.
> 
>>
>> So I'd really like to restrict the use of error_propagate() to places
>> where it's absolutely necessary. Unless, of course, you can fix these
>> practical problems that error_propagate() causes for debugging.
>>
>> In fact, in the context of Greg's series, I think we really only need to
>> support hints for error_fatal, which are cases that users are supposed
>> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
>> are things that are never supposed to happen. A good stack trace is more
>> important there than adding a hint to the message.
> 
> We also want to handle the caller passing NULL, so that we no longer
> have to introduce 'Error *local_error = NULL' everywhere.
> 

With my plan of two different macro, I at least messed the case when we need
both dereferencing and hints, which means third macro, or one macro with parameters,
saying what to wrap.

And my aim was to follow the idea of "do propagation only if it really necessary in this case".

But may be you are right, and we shouldn't care so much.

1. What is bad, if we wrap NULL, when we only want to use hints?
Seems nothing. Some extra actions on error path, but who cares about it?

2. What is bad, if we wrap error_fatal, when we only want to dereference, and don't use hints?
Seems nothing again, on error path we will return from higher level, and a bit of extra work, but nothing worse..

So I tend to agree. But honestly, I didn't understand first part of Kevin's paragraph against propagation,
so, may be he have more reasons to minimize number of cases when we propagate.

To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
Kevin, is something bad in propagation, when it not related to error_abort?
Eric Blake Sept. 19, 2019, 2:26 p.m. UTC | #32
On 9/19/19 9:13 AM, Vladimir Sementsov-Ogievskiy wrote:

> 
> With my plan of two different macro, I at least messed the case when we need
> both dereferencing and hints, which means third macro, or one macro with parameters,
> saying what to wrap.
> 
> And my aim was to follow the idea of "do propagation only if it really necessary in this case".
> 
> But may be you are right, and we shouldn't care so much.
> 
> 1. What is bad, if we wrap NULL, when we only want to use hints?
> Seems nothing. Some extra actions on error path, but who cares about it?
> 
> 2. What is bad, if we wrap error_fatal, when we only want to dereference, and don't use hints?
> Seems nothing again, on error path we will return from higher level, and a bit of extra work, but nothing worse..
> 
> So I tend to agree. But honestly, I didn't understand first part of Kevin's paragraph against propagation,
> so, may be he have more reasons to minimize number of cases when we propagate.

Here's the problem.

error_propagate(&error_abort, ...)

produces an abort() with a stack trace tied to your CURRENT location,
not the location at which the error was first detected.  We DO copy the
function name and line number from the original detection, but we have
moved on since that point in time, so the backtrace does not match the
reported location in the error struct.  So for error_abort, you want to
abort as soon as possible, without any intermediate error_propagate - we
use this to flag programmer errors, and want to make life easy for the
programmer.

But for error_propagate(&error_fatal, ...), we WANT to delay things to
as late as possible, in order to gather up as many additional
append_hints as possible, before finally telling the user their problem.
 In that case, we don't care about the stack trace or even the function
and line number of the failure, but about giving the user as much as we
can to help them fix their command-line problem.

> 
> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
> Kevin, is something bad in propagation, when it not related to error_abort?

error_propagate() serves two purposes:

It is a crutch to make it easier to paper over passing NULL as errp (in
that case, we substitute a non-NULL pointer, then we can use 'if
(local_error)', and error_propagate() then frees local_error).  Hiding
this crutch behind a macro is desirable (less boilerplate).

It helps transfer from one error to another.  In this form, we are fine
using it for error_fatal (because we WANT the transfer to happen as late
as possible, because once we transfer the program exits so we can't add
any more hints), but not for error_abort (because we don't want to lose
context by transferring), and wasteful at other times (write directly to
the original error, instead of having to transfer).  So again, hiding it
in a macro means we no longer have to call it directly, but only in the
places where it helps.
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 2:30 p.m. UTC | #33
19.09.2019 17:12, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 16:40, Eric Blake wrote:
>> On 9/19/19 4:17 AM, Kevin Wolf wrote:
>>> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
>>>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> + */
>>>>> +#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; \
>>>>> +}
>>>>
>>>> Not written to take a trailing semicolon in the caller.
>>>>
>>>> You could even set __auto_errp_prop unconditionally rather than trying
>>>> to reuse incoming errp (the difference being that error_propagate() gets
>>>> called more frequently).
>>>
>>> I think this difference is actually a problem.
>>>
>>> When debugging things, I hate error_propagate(). It means that the Error
>>> (specifically its fields src/func/line) points to the outermost
>>> error_propagate() rather than the place where the error really happened.
>>> It also makes error_abort completely useless because at the point where
>>> the process gets aborted, the interesting information is already lost.
>>
>> Okay, based on that, I see the following desirable semantics:
>>
>> Caller: one of 4 calling paradigms:
>>
>> pass errp=NULL (we don't care about failures)
>> pass errp=&error_abort (we want to abort() as soon as possible as close
>> to the real problem as possible)
>> pass errp=&error_fatal (we want to exit(), but only after collecting as
>> much information as possible)
>> pass errp = anything else (we are collecting an error for other reasons,
>> we may report it or let the caller decide or ...)
>>
>> Callee: we want a SINGLE paradigm:
>>
>> func (Error **errp)
>> {
>>      MAKE_ERRP_SAFE();
>>
>>      now we can pass errp to any child function, test '*errp', or do
>> anything else, and we DON'T have to call error_propagate.
>>
>> I think that means we need:
>>
>> #define MAKE_ERRP_SAFE() \
>>    g_auto(...) __auto_errp = { .errp = errp }; \
>>    do { \
>>      if (!errp || errp == &error_fatal) { errp = &__auto_errp.local; } \
>>    } while (0)
>>
>> So back to the caller semantics:
>>
>> if the caller passed NULL, we've redirected errp locally so that we can
>> use *errp at will; the auto-cleanup will free our local error.
>>
>> if the caller passed &error_abort, we keep errp unchanged.  *errp tests
>> will never trigger, because we'll have already aborted in the child on
>> the original errp, giving developers the best stack trace.
>>
>> if the caller passed &error_fatal, we redirect errp.  auto-cleanup will
>> then error_propagate that back to the caller, producing as much nice
>> information as possible.
>>
>> if the caller passed anything else, we keep errp unchanged, so no extra
>> error_propagate in the mix.
>>
>>>
>>> So I'd really like to restrict the use of error_propagate() to places
>>> where it's absolutely necessary. Unless, of course, you can fix these
>>> practical problems that error_propagate() causes for debugging.
>>>
>>> In fact, in the context of Greg's series, I think we really only need to
>>> support hints for error_fatal, which are cases that users are supposed
>>> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
>>> are things that are never supposed to happen. A good stack trace is more
>>> important there than adding a hint to the message.
>>
>> We also want to handle the caller passing NULL, so that we no longer
>> have to introduce 'Error *local_error = NULL' everywhere.
>>
> 
> With my plan of two different macro, I at least messed the case when we need
> both dereferencing and hints, which means third macro, or one macro with parameters,
> saying what to wrap.
> 
> And my aim was to follow the idea of "do propagation only if it really necessary in this case".
> 
> But may be you are right, and we shouldn't care so much.
> 
> 1. What is bad, if we wrap NULL, when we only want to use hints?
> Seems nothing. Some extra actions on error path, but who cares about it?
> 
> 2. What is bad, if we wrap error_fatal, when we only want to dereference, and don't use hints?
> Seems nothing again, on error path we will return from higher level, and a bit of extra work, but nothing worse..
> 
> So I tend to agree. But honestly, I didn't understand first part of Kevin's paragraph against propagation,
> so, may be he have more reasons to minimize number of cases when we propagate.
> 
> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
> Kevin, is something bad in propagation, when it not related to error_abort?
> 
> 

Or, even, we may use MAKE_ERRP_SAFE on every function, which have Error **errp argument, even if we neither
dereference it nor append hints? Is it what you propose by "SINGLE paradigm"? It's of course simpler to script,
to check in checkpatch and to maintain.
Kevin Wolf Sept. 19, 2019, 2:34 p.m. UTC | #34
Am 19.09.2019 um 16:13 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 19.09.2019 16:40, Eric Blake wrote:
> > On 9/19/19 4:17 AM, Kevin Wolf wrote:
> >> Am 18.09.2019 um 19:10 hat Eric Blake geschrieben:
> >>> On 9/18/19 8:02 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>>> + */
> >>>> +#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; \
> >>>> +}
> >>>
> >>> Not written to take a trailing semicolon in the caller.
> >>>
> >>> You could even set __auto_errp_prop unconditionally rather than trying
> >>> to reuse incoming errp (the difference being that error_propagate() gets
> >>> called more frequently).
> >>
> >> I think this difference is actually a problem.
> >>
> >> When debugging things, I hate error_propagate(). It means that the Error
> >> (specifically its fields src/func/line) points to the outermost
> >> error_propagate() rather than the place where the error really happened.
> >> It also makes error_abort completely useless because at the point where
> >> the process gets aborted, the interesting information is already lost.
> > 
> > Okay, based on that, I see the following desirable semantics:
> > 
> > Caller: one of 4 calling paradigms:
> > 
> > pass errp=NULL (we don't care about failures)
> > pass errp=&error_abort (we want to abort() as soon as possible as close
> > to the real problem as possible)
> > pass errp=&error_fatal (we want to exit(), but only after collecting as
> > much information as possible)
> > pass errp = anything else (we are collecting an error for other reasons,
> > we may report it or let the caller decide or ...)
> > 
> > Callee: we want a SINGLE paradigm:
> > 
> > func (Error **errp)
> > {
> >      MAKE_ERRP_SAFE();
> > 
> >      now we can pass errp to any child function, test '*errp', or do
> > anything else, and we DON'T have to call error_propagate.
> > 
> > I think that means we need:
> > 
> > #define MAKE_ERRP_SAFE() \
> >    g_auto(...) __auto_errp = { .errp = errp }; \
> >    do { \
> >      if (!errp || errp == &error_fatal) { errp = &__auto_errp.local; } \
> >    } while (0)
> > 
> > So back to the caller semantics:
> > 
> > if the caller passed NULL, we've redirected errp locally so that we can
> > use *errp at will; the auto-cleanup will free our local error.
> > 
> > if the caller passed &error_abort, we keep errp unchanged.  *errp tests
> > will never trigger, because we'll have already aborted in the child on
> > the original errp, giving developers the best stack trace.
> > 
> > if the caller passed &error_fatal, we redirect errp.  auto-cleanup will
> > then error_propagate that back to the caller, producing as much nice
> > information as possible.
> > 
> > if the caller passed anything else, we keep errp unchanged, so no extra
> > error_propagate in the mix.
> > 
> >>
> >> So I'd really like to restrict the use of error_propagate() to places
> >> where it's absolutely necessary. Unless, of course, you can fix these
> >> practical problems that error_propagate() causes for debugging.
> >>
> >> In fact, in the context of Greg's series, I think we really only need to
> >> support hints for error_fatal, which are cases that users are supposed
> >> to see. We should exclude error_abort in MAKE_ERRP_SAFE() because these
> >> are things that are never supposed to happen. A good stack trace is more
> >> important there than adding a hint to the message.
> > 
> > We also want to handle the caller passing NULL, so that we no longer
> > have to introduce 'Error *local_error = NULL' everywhere.
> > 
> 
> With my plan of two different macro, I at least messed the case when we need
> both dereferencing and hints, which means third macro, or one macro with parameters,
> saying what to wrap.
> 
> And my aim was to follow the idea of "do propagation only if it really necessary in this case".
> 
> But may be you are right, and we shouldn't care so much.
> 
> 1. What is bad, if we wrap NULL, when we only want to use hints?
> Seems nothing. Some extra actions on error path, but who cares about it?
> 
> 2. What is bad, if we wrap error_fatal, when we only want to dereference, and don't use hints?
> Seems nothing again, on error path we will return from higher level, and a bit of extra work, but nothing worse..
> 
> So I tend to agree. But honestly, I didn't understand first part of Kevin's paragraph against propagation,
> so, may be he have more reasons to minimize number of cases when we propagate.

I think my concerns were really only about error_abort and "normal"
non-NULL errp losing some information about the origin of the error. And
from this thread, it seems that I misremebered and the normal one is
actually supposed to just work.

In any case, wrapping NULL and error_fatal should be fine, so I agree
that a single macro should do.

> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?

Hm, I think it's more obviously correct if done at the top, but I also
can't see any reason why using it only in a block wouldn't work. So I'd
put it at the top just as a matter of style.

> Kevin, is something bad in propagation, when it not related to error_abort?

Probably not, unless I didn't misremember, but we misread the code.

Kevin
Eric Blake Sept. 19, 2019, 2:44 p.m. UTC | #35
On 9/19/19 9:30 AM, Vladimir Sementsov-Ogievskiy wrote:

>>
>> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
>> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
>> Kevin, is something bad in propagation, when it not related to error_abort?
>>
>>
> 
> Or, even, we may use MAKE_ERRP_SAFE on every function, which have Error **errp argument, even if we neither
> dereference it nor append hints? Is it what you propose by "SINGLE paradigm"? It's of course simpler to script,
> to check in checkpatch and to maintain.

Yes. The simpler we make the rules, and the less boilerplate it entails,
the more likely that we can:

a) avoid exceptions and corner cases that cost review time
b) automate the conversion into complying with the rule
c) codify those rules in syntax check to ensure they are followed
post-conversion

ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
**errp parameter is dirt-simple to explain.  It has no performance
penalty if the user passed in a normal error or error_abort (the cost of
an 'if' hidden in the macro is probably negligible compared to
everything else we do), and has no semantic penalty if the user passed
in NULL or error_fatal (we now get the behavior we want with less
boilerplate).

Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
can I omit it?' does not provide the same simplicity.
Daniel P. Berrangé Sept. 19, 2019, 2:49 p.m. UTC | #36
On Thu, Sep 19, 2019 at 09:44:14AM -0500, Eric Blake wrote:
> On 9/19/19 9:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> >>
> >> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
> >> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
> >> Kevin, is something bad in propagation, when it not related to error_abort?
> >>
> >>
> > 
> > Or, even, we may use MAKE_ERRP_SAFE on every function, which have Error **errp argument, even if we neither
> > dereference it nor append hints? Is it what you propose by "SINGLE paradigm"? It's of course simpler to script,
> > to check in checkpatch and to maintain.
> 
> Yes. The simpler we make the rules, and the less boilerplate it entails,
> the more likely that we can:
> 
> a) avoid exceptions and corner cases that cost review time
> b) automate the conversion into complying with the rule
> c) codify those rules in syntax check to ensure they are followed
> post-conversion
> 
> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
> **errp parameter is dirt-simple to explain.  It has no performance
> penalty if the user passed in a normal error or error_abort (the cost of
> an 'if' hidden in the macro is probably negligible compared to
> everything else we do), and has no semantic penalty if the user passed
> in NULL or error_fatal (we now get the behavior we want with less
> boilerplate).
> 
> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
> can I omit it?' does not provide the same simplicity.

The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
really know what its doing without looking at it, and this is QEMU
custom concept so one more thing to learn for new contributors.

While I think it is a nice trick, personally I think we would be better
off if we simply used a code pattern which does not require de-referencing
'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
of all our methods using Error **errp.

There are lots of neat things we could do with auto-cleanup functions we
I think we need to be wary of hiding too much cleverness behind macros
when doing so overall.

Regards,
Daniel
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 3:12 p.m. UTC | #37
19.09.2019 17:44, Eric Blake wrote:
> On 9/19/19 9:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>
>>> To the same topic, of minimization: should we always call MAKE_ERRP_SAFE at function top, or only
>>> in block, where it is needed (assume, we dereference it only inside some "if" or "while"?
>>> Kevin, is something bad in propagation, when it not related to error_abort?
>>>
>>>
>>
>> Or, even, we may use MAKE_ERRP_SAFE on every function, which have Error **errp argument, even if we neither
>> dereference it nor append hints? Is it what you propose by "SINGLE paradigm"? It's of course simpler to script,
>> to check in checkpatch and to maintain.
> 
> Yes. The simpler we make the rules, and the less boilerplate it entails,
> the more likely that we can:
> 
> a) avoid exceptions and corner cases that cost review time
> b) automate the conversion into complying with the rule
> c) codify those rules in syntax check to ensure they are followed
> post-conversion
> 
> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
> **errp parameter is dirt-simple to explain.  It has no performance
> penalty if the user passed in a normal error or error_abort (the cost of
> an 'if' hidden in the macro is probably negligible compared to
> everything else we do), and has no semantic penalty if the user passed
> in NULL or error_fatal (we now get the behavior we want with less
> boilerplate).
> 
> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
> can I omit it?' does not provide the same simplicity.
> 

Interesting: it's another story, but with this macro used in each errp-function we can collect
the whole call-stack of the error int Error object, and report it.
It may be not good for end-user, but very useful for testing.

Or is it bad idea? Anyway, I often have the problem: I have some error reported, and need to
understand where was it from.. git grep helps, but backtrace would be a lot better.
Eric Blake Sept. 19, 2019, 3:24 p.m. UTC | #38
On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:

>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
>> **errp parameter is dirt-simple to explain.  It has no performance
>> penalty if the user passed in a normal error or error_abort (the cost of
>> an 'if' hidden in the macro is probably negligible compared to
>> everything else we do), and has no semantic penalty if the user passed
>> in NULL or error_fatal (we now get the behavior we want with less
>> boilerplate).
>>
>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
>> can I omit it?' does not provide the same simplicity.
> 
> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
> really know what its doing without looking at it, and this is QEMU
> custom concept so one more thing to learn for new contributors.
> 
> While I think it is a nice trick, personally I think we would be better
> off if we simply used a code pattern which does not require de-referencing
> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
> of all our methods using Error **errp.

If 100% of our callsites use the macro, then new contributors will
quickly learn by observation alone that the macro usage must be
important on any new function taking Error **errp, whether or not they
actually read the macro to see what it does.  If only 5% of our
callsites use the macro, it's harder to argue that a new user will pick
up on the nuances by observation alone (presumably, our docs would also
spell it out, but we know that not everyone reads those...).

However, if we can automate syntax checks to reach a near-100% accuracy,
we don't HAVE to worry about whether a new programmer picks up on the
nuances by observation, because they will instead pick up the nuances by
CI rejection messages.  This is true for _either_ style:

100% use of the macro: CI message would be "you added a method with a
parameter 'Error **errp' but forgot to use MAKE_ERRP_SAFE"

use of the macro only where necessary (namely, functions that contain
'*errp' and/or 'error_append_hint'): CI message would either be "your
function body requires MAKE_ERRP_SAFE but you forgot it" or "your
function body does not require MAKE_ERRP_SAFE but you forgot to remove
it".  And this would work even for experienced committers editing
existing functions (such as ongoing work to convert away from 'void
child(errp); if (*errp)' and towards 'if (int child(errp) < 0)').

Writing the CI engine for the first case is easy, writing it for the
second is a bit harder, but still seems tractable (since, for any
function with an 'Error **errp' parameter, it should be easy to scan the
function body for instances of '*errp' or 'error_append_hint', as well
as to scan whether MAKE_ERRP_SAFE was present or absent accordingly).

> There are lots of neat things we could do with auto-cleanup functions we
> I think we need to be wary of hiding too much cleverness behind macros
> when doing so overall.

The benefit of getting rid of the 'Error *local_err = NULL; ...
error_propagate()' boilerplate is worth the cleverness, in my opinion,
but especially if also accompanied by CI coverage that we abide by our
new rules.

I'd really like to hear Markus' opinion on the matter, as Error maintainer.
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 3:37 p.m. UTC | #39
19.09.2019 18:24, Eric Blake wrote:
> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:
> 
>>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
>>> **errp parameter is dirt-simple to explain.  It has no performance
>>> penalty if the user passed in a normal error or error_abort (the cost of
>>> an 'if' hidden in the macro is probably negligible compared to
>>> everything else we do), and has no semantic penalty if the user passed
>>> in NULL or error_fatal (we now get the behavior we want with less
>>> boilerplate).
>>>
>>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
>>> can I omit it?' does not provide the same simplicity.
>>
>> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
>> really know what its doing without looking at it, and this is QEMU
>> custom concept so one more thing to learn for new contributors.
>>
>> While I think it is a nice trick, personally I think we would be better
>> off if we simply used a code pattern which does not require de-referencing
>> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
>> of all our methods using Error **errp.
> 
> If 100% of our callsites use the macro, then new contributors will
> quickly learn by observation alone that the macro usage must be
> important on any new function taking Error **errp, whether or not they
> actually read the macro to see what it does.  If only 5% of our
> callsites use the macro, it's harder to argue that a new user will pick
> up on the nuances by observation alone (presumably, our docs would also
> spell it out, but we know that not everyone reads those...).

When I was a new contributor, it was not simple to understand, when and why we
need to use local_err, keeping in mind that (this is not only reason, but also
a consequence) we have places where local_err is used for no reason.

> 
> However, if we can automate syntax checks to reach a near-100% accuracy,
> we don't HAVE to worry about whether a new programmer picks up on the
> nuances by observation, because they will instead pick up the nuances by
> CI rejection messages.  This is true for _either_ style:
> 
> 100% use of the macro: CI message would be "you added a method with a
> parameter 'Error **errp' but forgot to use MAKE_ERRP_SAFE"
> 
> use of the macro only where necessary (namely, functions that contain
> '*errp' and/or 'error_append_hint'): CI message would either be "your
> function body requires MAKE_ERRP_SAFE but you forgot it" or "your
> function body does not require MAKE_ERRP_SAFE but you forgot to remove
> it".  And this would work even for experienced committers editing
> existing functions (such as ongoing work to convert away from 'void
> child(errp); if (*errp)' and towards 'if (int child(errp) < 0)').
> 
> Writing the CI engine for the first case is easy, writing it for the
> second is a bit harder, but still seems tractable (since, for any
> function with an 'Error **errp' parameter, it should be easy to scan the
> function body for instances of '*errp' or 'error_append_hint', as well
> as to scan whether MAKE_ERRP_SAFE was present or absent accordingly).
> 
>> There are lots of neat things we could do with auto-cleanup functions we
>> I think we need to be wary of hiding too much cleverness behind macros
>> when doing so overall.
> 
> The benefit of getting rid of the 'Error *local_err = NULL; ...
> error_propagate()' boilerplate is worth the cleverness, in my opinion,
> but especially if also accompanied by CI coverage that we abide by our
> new rules.
> 
> I'd really like to hear Markus' opinion on the matter, as Error maintainer.
>
Daniel P. Berrangé Sept. 19, 2019, 3:50 p.m. UTC | #40
On Thu, Sep 19, 2019 at 10:24:20AM -0500, Eric Blake wrote:
> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:
> 
> >> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
> >> **errp parameter is dirt-simple to explain.  It has no performance
> >> penalty if the user passed in a normal error or error_abort (the cost of
> >> an 'if' hidden in the macro is probably negligible compared to
> >> everything else we do), and has no semantic penalty if the user passed
> >> in NULL or error_fatal (we now get the behavior we want with less
> >> boilerplate).
> >>
> >> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
> >> can I omit it?' does not provide the same simplicity.
> > 
> > The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
> > really know what its doing without looking at it, and this is QEMU
> > custom concept so one more thing to learn for new contributors.
> > 
> > While I think it is a nice trick, personally I think we would be better
> > off if we simply used a code pattern which does not require de-referencing
> > 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
> > of all our methods using Error **errp.
> 
> If 100% of our callsites use the macro, then new contributors will
> quickly learn by observation alone that the macro usage must be
> important on any new function taking Error **errp, whether or not they
> actually read the macro to see what it does.  If only 5% of our
> callsites use the macro, it's harder to argue that a new user will pick
> up on the nuances by observation alone (presumably, our docs would also
> spell it out, but we know that not everyone reads those...).

To get some slightly less made-up stats, I did some searching:

  - 4408  methods with an 'errp' parameter declared

     git grep 'Error \*\*errp'|  wc -l

  - 696 methods with an 'Error *local' declared (what other names
    do we use for this?)

     git grep 'Error \*local' | wc -l

  - 1243 methods with an 'errp' parameter which have void
    return value (fuzzy number - my matching is quite crude)

     git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l

  - 11 methods using error_append_hint with a local_err

     git grep append_hint | grep local | wc -l


This suggests to me, that if we used the 'return 0 / return -1' convention
to indicate failure for the methods which currently return void, we could
potentially only have 11 cases where a local error object is genuinely
needed. 

If my numbers are at all accurate, I still believe we're better off
changing the return type and eliminating essentially all use of local
error variables, as void funcs/local error objects appear to be the
minority coding pattern.


> > There are lots of neat things we could do with auto-cleanup functions we
> > I think we need to be wary of hiding too much cleverness behind macros
> > when doing so overall.
> 
> The benefit of getting rid of the 'Error *local_err = NULL; ...
> error_propagate()' boilerplate is worth the cleverness, in my opinion,
> but especially if also accompanied by CI coverage that we abide by our
> new rules.

At least we're both wanting to eliminate the local error + propagation.
The difference is whether we're genuinely eliminating it, or just hiding
it from view via auto-cleanup & macro magic

Regards,
Daniel
Vladimir Sementsov-Ogievskiy Sept. 19, 2019, 4:16 p.m. UTC | #41
19.09.2019 18:50, Daniel P. Berrangé wrote:
> On Thu, Sep 19, 2019 at 10:24:20AM -0500, Eric Blake wrote:
>> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:
>>
>>>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
>>>> **errp parameter is dirt-simple to explain.  It has no performance
>>>> penalty if the user passed in a normal error or error_abort (the cost of
>>>> an 'if' hidden in the macro is probably negligible compared to
>>>> everything else we do), and has no semantic penalty if the user passed
>>>> in NULL or error_fatal (we now get the behavior we want with less
>>>> boilerplate).
>>>>
>>>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
>>>> can I omit it?' does not provide the same simplicity.
>>>
>>> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
>>> really know what its doing without looking at it, and this is QEMU
>>> custom concept so one more thing to learn for new contributors.
>>>
>>> While I think it is a nice trick, personally I think we would be better
>>> off if we simply used a code pattern which does not require de-referencing
>>> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
>>> of all our methods using Error **errp.
>>
>> If 100% of our callsites use the macro, then new contributors will
>> quickly learn by observation alone that the macro usage must be
>> important on any new function taking Error **errp, whether or not they
>> actually read the macro to see what it does.  If only 5% of our
>> callsites use the macro, it's harder to argue that a new user will pick
>> up on the nuances by observation alone (presumably, our docs would also
>> spell it out, but we know that not everyone reads those...).
> 
> To get some slightly less made-up stats, I did some searching:
> 
>    - 4408  methods with an 'errp' parameter declared
> 
>       git grep 'Error \*\*errp'|  wc -l
> 
>    - 696 methods with an 'Error *local' declared (what other names
>      do we use for this?)
> 
>       git grep 'Error \*local' | wc -l
> 
>    - 1243 methods with an 'errp' parameter which have void
>      return value (fuzzy number - my matching is quite crude)
> 
>       git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l
> 
>    - 11 methods using error_append_hint with a local_err
> 
>       git grep append_hint | grep local | wc -l

why do you count only with local? Greg's series is here to bring local to all
functions with append_hint:

# git grep append_hint | wc -l
85

(still, some functions may append_hint several times, so the number should be less)
and file list is big:

# git grep -l append_hint
block.c
block/backup.c
block/dirty-bitmap.c
block/file-posix.c
block/gluster.c
block/qcow.c
block/qcow2.c
block/vhdx-log.c
block/vpc.c
chardev/spice.c
hw/9pfs/9p-local.c
hw/9pfs/9p-proxy.c
hw/arm/msf2-soc.c
hw/arm/virt.c
hw/audio/intel-hda.c
hw/intc/arm_gicv3_kvm.c
hw/misc/msf2-sysreg.c
hw/pci-bridge/pci_bridge_dev.c
hw/pci-bridge/pcie_root_port.c
hw/ppc/mac_newworld.c
hw/ppc/spapr.c
hw/ppc/spapr_irq.c
hw/ppc/spapr_pci.c
hw/rdma/vmw/pvrdma_main.c
hw/s390x/s390-ccw.c
hw/scsi/megasas.c
hw/scsi/mptsas.c
hw/scsi/scsi-disk.c
hw/scsi/scsi-generic.c
hw/usb/ccid-card-emulated.c
hw/usb/hcd-xhci.c
hw/vfio/common.c
hw/vfio/pci-quirks.c
hw/vfio/pci.c
hw/virtio/virtio-pci.c
hw/xen/xen_pt.c
hw/xen/xen_pt_config_init.c
include/qapi/error.h
migration/migration.c
nbd/client.c
nbd/server.c
qdev-monitor.c
target/arm/cpu64.c
target/ppc/kvm.c
util/error.c
util/qemu-option.c
util/qemu-sockets.c


Also, conversion to use macro everywhere may be done (seems so) by coccinelle script.
But conversion which you mean, only by hand I think. Converting 1243 methods by hand
is a huge task..


I think there are three consistent ways:

1. Use macro everywhere
2. Drop error_append_hint
3. Drop error_fatal


> 
> 
> This suggests to me, that if we used the 'return 0 / return -1' convention
> to indicate failure for the methods which currently return void, we could
> potentially only have 11 cases where a local error object is genuinely
> needed.
> 
> If my numbers are at all accurate, I still believe we're better off
> changing the return type and eliminating essentially all use of local
> error variables, as void funcs/local error objects appear to be the
> minority coding pattern.
> 
> 
>>> There are lots of neat things we could do with auto-cleanup functions we
>>> I think we need to be wary of hiding too much cleverness behind macros
>>> when doing so overall.
>>
>> The benefit of getting rid of the 'Error *local_err = NULL; ...
>> error_propagate()' boilerplate is worth the cleverness, in my opinion,
>> but especially if also accompanied by CI coverage that we abide by our
>> new rules.
> 
> At least we're both wanting to eliminate the local error + propagation.
> The difference is whether we're genuinely eliminating it, or just hiding
> it from view via auto-cleanup & macro magic
> 
> Regards,
> Daniel
>
Daniel P. Berrangé Sept. 19, 2019, 4:51 p.m. UTC | #42
On Thu, Sep 19, 2019 at 04:16:25PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 19.09.2019 18:50, Daniel P. Berrangé wrote:
> > On Thu, Sep 19, 2019 at 10:24:20AM -0500, Eric Blake wrote:
> >> On 9/19/19 9:49 AM, Daniel P. Berrangé wrote:
> >>
> >>>> ALWAYS using MAKE_ERRP_SAFE() on entry to any function that has an Error
> >>>> **errp parameter is dirt-simple to explain.  It has no performance
> >>>> penalty if the user passed in a normal error or error_abort (the cost of
> >>>> an 'if' hidden in the macro is probably negligible compared to
> >>>> everything else we do), and has no semantic penalty if the user passed
> >>>> in NULL or error_fatal (we now get the behavior we want with less
> >>>> boilerplate).
> >>>>
> >>>> Having to think 'does this method require me to use MAKE_ERRP_SAFE, or
> >>>> can I omit it?' does not provide the same simplicity.
> >>>
> >>> The flipside is that MAKE_ERRP_SAFE hides a bunch of logic, so you don't
> >>> really know what its doing without looking at it, and this is QEMU
> >>> custom concept so one more thing to learn for new contributors.
> >>>
> >>> While I think it is a nice trick, personally I think we would be better
> >>> off if we simply used a code pattern which does not require de-referencing
> >>> 'errp' at all, aside from exceptional cases. IOW, no added macro in 95%
> >>> of all our methods using Error **errp.
> >>
> >> If 100% of our callsites use the macro, then new contributors will
> >> quickly learn by observation alone that the macro usage must be
> >> important on any new function taking Error **errp, whether or not they
> >> actually read the macro to see what it does.  If only 5% of our
> >> callsites use the macro, it's harder to argue that a new user will pick
> >> up on the nuances by observation alone (presumably, our docs would also
> >> spell it out, but we know that not everyone reads those...).
> > 
> > To get some slightly less made-up stats, I did some searching:
> > 
> >    - 4408  methods with an 'errp' parameter declared
> > 
> >       git grep 'Error \*\*errp'|  wc -l
> > 
> >    - 696 methods with an 'Error *local' declared (what other names
> >      do we use for this?)
> > 
> >       git grep 'Error \*local' | wc -l
> > 
> >    - 1243 methods with an 'errp' parameter which have void
> >      return value (fuzzy number - my matching is quite crude)
> > 
> >       git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l
> > 
> >    - 11 methods using error_append_hint with a local_err
> > 
> >       git grep append_hint | grep local | wc -l
> 
> why do you count only with local? Greg's series is here to bring local to all
> functions with append_hint:

I hadn't noticed the scope of Greg's series :-)

> 
> # git grep append_hint | wc -l
> 85



> Also, conversion to use macro everywhere may be done (seems so) by coccinelle script.
> But conversion which you mean, only by hand I think. Converting 1243 methods by hand
> is a huge task..

Yeah, it would be a non-negligible amount of work.

> I think there are three consistent ways:
> 
> 1. Use macro everywhere
> 2. Drop error_append_hint
> 3. Drop error_fatal

Regards,
Daniel
Vladimir Sementsov-Ogievskiy Sept. 20, 2019, 11:46 a.m. UTC | #43
18.09.2019 16:02, Vladimir Sementsov-Ogievskiy 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.
> 
> 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).
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03449.html
> 


OK, seems a kind of consensus is here, and looks like

#define MAKE_ERRP_SAFE() \
g_auto(ErrorPropagationStruct) __auto_errp_prop = {.errp = errp}; \
Error **__local_errp_unused __attribute__ ((unused)) = \
     (errp = (errp == NULL || *errp == error_fatal ? \
              &__auto_errp_prop.local_error : errp))


[I also suggest to call it ERRP_FUNCTION_BEGIN, in case we'll enhance it
in future (for example bring call stack into Error)]


And MAKE_ERRP_SAFE assumed to be used in all errp-functions.


Now, there are still several things to discuss:


1. Some functions want to do error_free(local_err), error_report_err(local_err), or
like this, when they decide that error should not be propagated.

What to do with them? I suggest to make corresponding functions

error_free_errp, error_report_errp, warn_report_ferrp, etc, with Error **errp parameter,
which calls corresponding  Error* function and then set pointer to 0. Then our propagation
cleanup will do nothing, as desired.


2. Some functions tends to error_free(*errp) or error_report_err(*errp). So, they don't
use errp to return error, but instead they want to handle errp somehow:
vnc_client_io_error
   - is used only in two places to trace errp, so it may be inlined

hmp_handle_error
   - is a wrapper used in more than 80 places, to do error_reportf_err(*errp, "Error: ");
(hmm, bad that it doesn't set *errp to ZERO after it)

what do do with it? inline too?

or, maybe rename errp parameter to "Error **filled_errp", to not match our criteria?

(api function error_free_or_abort is with same behavior)

Just bugs:

3. Wow, fit_load_fdt() just error_free(*errp) in wrong way! It must set then *errp = NULL,
but it doesn't do it. And qmp_query_cpu_def() is correct example of this thing -
these functions definitely wants error_free_errp function. But qmp_query_cpu_def()
incorrectly dereference errp (it may be NULL!)

4. A lot of functions in target/s390x/cpu_models.c just dereference errp to check error
also:
build_guest_fsinfo_for_virtual_device
legacy_acpi_cpu_plug_cb
sclp_events_bus_realize
memory_device_get_free_addr
ipmi_isa_realize
isa_ipmi_bt_realize
legacy_acpi_cpu_plug_cb

5. file_ram_alloc has funny patter to check error:
     if (mem_prealloc) {
         os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
         if (errp && *errp) {
             qemu_ram_munmap(fd, area, memory);
             return NULL;
         }
     }


Seems nothing interesting more. I used this coccinelle script
@@
identifier fn;
@@

  fn(..., Error **errp)
  {
      <...
*    *errp
      ...>
  }

to search by this commend:

git grep -l 'Error \*\*errp' | xargs grep -l '[^*]\*errp' | xargs spatch --sp-file script
Eric Blake Sept. 20, 2019, 12:58 p.m. UTC | #44
On 9/19/19 10:50 AM, Daniel P. Berrangé wrote:

> To get some slightly less made-up stats, I did some searching:
> 
>   - 4408  methods with an 'errp' parameter declared
> 
>      git grep 'Error \*\*errp'|  wc -l
> 
>   - 696 methods with an 'Error *local' declared (what other names
>     do we use for this?)
> 
>      git grep 'Error \*local' | wc -l

Covers 'local' and 'local_err'.  We also have:

git grep 'Error \*err\b' | wc -l
553

So 1249 local errors.

> 
>   - 1243 methods with an 'errp' parameter which have void
>     return value (fuzzy number - my matching is quite crude)
> 
>      git grep 'Error \*\*errp'|  grep -E '(^| )void' | wc -l
> 
>   - 11 methods using error_append_hint with a local_err
> 
>      git grep append_hint | grep local | wc -l
> 
> 
> This suggests to me, that if we used the 'return 0 / return -1' convention
> to indicate failure for the methods which currently return void, we could
> potentially only have 11 cases where a local error object is genuinely
> needed. 
> 
> If my numbers are at all accurate, I still believe we're better off
> changing the return type and eliminating essentially all use of local
> error variables, as void funcs/local error objects appear to be the
> minority coding pattern.

When relying on a return value, you do have to check whether a negative
return value happens if and only if errp is set; that's something that's
harder for code grepping to spot.  I'm not opposed to that coding
pattern, just pointing out that it requires some semantic analysis,
while MAKE_ERRP_SAFE() coupled with checking *errp is easier to prove at
a glance that the check for whether an error happened is 1:1 associated
with whether an error is reported.

> 
> 
>>> There are lots of neat things we could do with auto-cleanup functions we
>>> I think we need to be wary of hiding too much cleverness behind macros
>>> when doing so overall.
>>
>> The benefit of getting rid of the 'Error *local_err = NULL; ...
>> error_propagate()' boilerplate is worth the cleverness, in my opinion,
>> but especially if also accompanied by CI coverage that we abide by our
>> new rules.
> 
> At least we're both wanting to eliminate the local error + propagation.
> The difference is whether we're genuinely eliminating it, or just hiding
> it from view via auto-cleanup & macro magic
> 
> Regards,
> Daniel
>

Patch
diff mbox series

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