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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   include/qapi/error.h | 102 +++++++++++++++++++++++++++++++++++++++++++
>>   block.c              |  63 ++++++++++++--------------
>>   block/backup.c       |   8 +++-
>>   block/gluster.c      |   7 +++
>>   4 files changed, 144 insertions(+), 36 deletions(-)
> 
> ...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.

-- 
Best regards,
Vladimir