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