Re: [RFC v5 067/126] block: introduce ERRP_AUTO_PROPAGATE

[Eric Blake]

On 10/11/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> If we want to add some info to errp (by error_prepend() or
> error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
> Otherwise, this info will not be added when errp == &fatal_err
> (the program will exit prior to the error_append_hint() or
> error_prepend() call).  Fix such cases.
>
> If we want to check error after errp-function call, we need to
> introduce local_err and than propagate it to errp. Instead, use
> ERRP_AUTO_PROPAGATE macro, benefits are:
> 1. No need of explicit error_propagate call
> 2. No need of explicit local_err variable: use errp directly
> 3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
>     &error_fatel, this means that we don't break error_abort
>     (we'll abort on error_set, not on error_propagate)

> Reported-by: Kevin Wolf <address@hidden>
> Reported-by: Greg Kurz <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---

>   17 files changed, 287 insertions(+), 381 deletions(-)

A bit large, but still manageable for manually checking.

> +++ b/block.c

> @@ -1565,6 +1558,7 @@ fail_opts:
>   
>   static QDict *parse_json_filename(const char *filename, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       QObject *options_obj;
>       QDict *options;
>       int ret;

Not shown in the diff, but this one is correct because of the use of 
error_prepend().

It does make me wonder if separating the error_prepend/error_hint vs. 
error_propagate into two separate cleanups (by incrementally amending 
the .cocci script between two series of generated patches) makes the 
overall review easier or harder.  But don't respin the series just 
because of me - wait for feedback from Markus as well on the best path 
forward.


> +++ b/block/backup.c
> @@ -583,6 +583,7 @@ static const BlockJobDriver backup_job_driver = {
>   static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>                                                Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       int ret;
>       BlockDriverInfo bdi;
>   

Here, the diff is correct because of error_append_hint.

> +++ b/block/dirty-bitmap.c
> @@ -237,6 +237,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap 
> *bitmap)
>   int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
>                               Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>           error_setg(errp, "Bitmap '%s' is currently in use by another"
>                      " operation and cannot be used", bitmap->name);

and here

> +++ b/block/qapi.c
> @@ -44,6 +44,7 @@
>   BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>                                           BlockDriverState *bs, Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       ImageInfo **p_image_info;
>       BlockDriverState *bs0;
>       BlockDeviceInfo *info;
> @@ -148,10 +149,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>       p_image_info = &info->image;
>       info->backing_file_depth = 0;
>       while (1) {
> -        Error *local_err = NULL;
> -        bdrv_query_image_info(bs0, p_image_info, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        bdrv_query_image_info(bs0, p_image_info, errp);
> +        if (*errp) {
>               qapi_free_BlockDeviceInfo(info);
>               return NULL;
>           }

I'm a bit wary of auto-propagate at the top level, but a local variable 
declared inside a loop - if the loop repeats more than once, we need to 
make sure that no subsequent iteration of the loop gets handed an 
already-set errp from an earlier iteration.  In this case, we're safe 
(the loop exits early due to return), but it's something to look out for 
in other patches.

> +++ b/hw/block/onenand.c

A bit odd to group this with the others.  But not the end of the world.

Reviewed-by: Eric Blake <address@hidden>

So far, however, I have not tried rerunning the Coccinelle script, or 
checking (perhaps by sed) whether there are missed opportunities in 
these files that Coccinelle missed.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org