Re: [PATCH 1/2] block/export: Free ignored Error

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

22.04.2021 17:53, Max Reitz wrote:
> When invoking block-export-add with some iothread and
> fixed-iothread=false, and changing the node's iothread fails, the error
> is supposed to be ignored.
> 
> However, it is still stored in *errp, which is wrong.  If a second error
> occurs, the "*errp must be NULL" assertion in error_setv() fails:
> 
>    qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion
>    `*errp == NULL' failed.
> 
> So the error from bdrv_try_set_aio_context() must be freed when it is
> ignored.
> 
> Fixes: f51d23c80af73c95e0ce703ad06a300f1b3d63ef
>         ("block/export: add iothread and fixed-iothread options")
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/export/export.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/block/export/export.c b/block/export/export.c
> index fec7d9f738..ce5dd3e59b 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -68,6 +68,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
>   
>   BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>   {
> +    ERRP_GUARD();
>       bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;
>       const BlockExportDriver *drv;
>       BlockExport *exp = NULL;
> @@ -127,6 +128,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>               ctx = new_ctx;
>           } else if (fixed_iothread) {
>               goto fail;
> +        } else {
> +            error_free(*errp);
> +            *errp = NULL;
>           }
>       }
>   
> 

I don't think ERRP_GUARD is needed in this case: we don't need to handle errp somehow except for just free if it was set. So we can simply do:

} else if (errp) {
    error_free(*errp);
    *errp = NULL;
}

Let's only check that errp is really set on failure path of bdrv_try_set_aio_context():

bdrv_try_set_aio_context() fails iff bdrv_can_set_aio_context() fails, which in turn may fail iff bdrv_parent_can_set_aio_context() or bdrv_child_can_set_aio_context() fails.

bdrv_parent_can_set_aio_context() has two failure path, on first it set errp by hand, and on second it has assertion that errp is set.

bdrv_child_can_set_aio_context() may fail only if nested call to bdrv_can_set_aio_context() fails, so recursion is closed.


-- 
Best regards,
Vladimir