Re: [PATCH v5 2/4] block: add bdrv_co_delete_file_noerr

[Vladimir Sementsov-Ogievskiy]

09.12.2020 23:33, Maxim Levitsky wrote:
> This function wraps bdrv_co_delete_file for the common case of removing a file,
> which was just created by format driver, on an error condition.
>
> It hides the -ENOTSUPP error, and reports all other errors otherwise.

I've looked at original commit added this logic, and didn't find exactly, why 
should we hide it..

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   block.c               | 24 ++++++++++++++++++++++++
>   include/block/block.h |  1 +
>   2 files changed, 25 insertions(+)
>
> diff --git a/block.c b/block.c
> index f1cedac362..5d35ba2fb8 100644
> --- a/block.c
> +++ b/block.c
> @@ -704,6 +704,30 @@ int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, 
> Error **errp)
>       return ret;
>   }
>   
> +void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs)
> +{
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    if (!bs) {
> +        return;
> +    }
> +
> +    ret = bdrv_co_delete_file(bs, &local_err);
> +    /*
> +     * ENOTSUP will happen if the block driver doesn't support
> +     * the 'bdrv_co_delete_file' interface. This is a predictable
> +     * scenario and shouldn't be reported back to the user.
> +     */

It's not predictable for user: user doesn't know internal processes and 
interfaces.

The problem: we've left extra file on failure scenario and can't remove it. We 
want to warn the user that there is a wrong file left. Why not to warn, when 
the removement is unsupported internally by the driver?

I think, it's better to report it in any case.

Another reason: less code and logic on failure case. Optimizing failure 
scenarios in different manner is seldom a good thing to do.

And finally: why to report the error at all? If we have errp parameter, it's better to 
add the info to it using error_append.. something like error_append(errp, " (also 
failed to remove unfinished file %s: %s)", file_name, error_get_pretty(local_err))


Probably I'm making mountains out of molehills. It should work, so take my r-b 
anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Side note: I'd not split patches 02 and 03: this way it would be simpler to see 
the code movement.

> +    if (ret == -ENOTSUP) {
> +        error_free(local_err);
> +    } else if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +}
> +
> +
> +

three empty lines..

>   /**
>    * Try to get @bs's logical and physical block size.
>    * On success, store them in @bsz struct and return 0.
> diff --git a/include/block/block.h b/include/block/block.h
> index c9d7c58765..af03022723 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,6 +428,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, 
> BlockDriverState *base,
>                                 Error **errp);
>   void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState 
> *base);
>   int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
> +void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs);
>   
>   
>   typedef struct BdrvCheckResult {


--
Best regards,
Vladimir