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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2020-12-10 at 13:54 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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.

This is a good point, but it is not something I changed.
I prefer this to be changed in a separate patch.


> 
> 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))

The idea behind this was to reduce code duplication in the callers of this function,
and this is why this function doesn't even have the 'errp' parameter.
I'll think about it.

> 
> 
> 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..

Will fix.
> 
> >   /**
> >    * 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 {
> > 

Thanks a lot for the review!

Best regards,
	Maxim Levitsky

> 
>