Re: [Qemu-devel] [PATCH v4 1/4] block: introducing 'bdrv_co_delete_file' interface

[Kevin Wolf <kwolf@redhat.com>]

Am 28.06.2019 um 21:45 hat Daniel Henrique Barboza geschrieben:
> Adding to Block Drivers the capability of being able to clean up
> its created files can be useful in certain situations. For the
> LUKS driver, for instance, a failure in one of its authentication
> steps can leave files in the host that weren't there before.
> 
> This patch adds the 'bdrv_co_delete_file' interface to block
> drivers and add it to the 'file' driver in file-posix.c. The
> implementation is given by 'raw_co_delete_file'. The helper
> 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
> at this moment, but it will be used inside LUKS in a later patch.
> Foreseeing this future use, let's put it in block.c and make it
> public.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  block.c                   | 11 +++++++++++
>  block/file-posix.c        | 28 ++++++++++++++++++++++++++++
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  6 ++++++
>  4 files changed, 46 insertions(+)
> 
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -309,6 +309,12 @@ struct BlockDriver {
>       */
>      int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
>  
> +    /*
> +     * Delete a local created file.
> +     */
> +    int coroutine_fn (*bdrv_co_delete_file)(const char *filename,
> +                                            Error **errp);

I wonder if it wouldn't make more sense to pass a BlockDriverState
instead of a filename. In the create options we usually have a BDS
around, so it should be possible to use.

The only case where it wouldn't work is if creating the image file
worked, but bdrv_open() fails. I think this is unlikely, and it's even
more unlikely that unlinking such a file would then work, so I wouldn't
see it as a problem.

The reason why I'm suggesting this is that we've spent a lot of time in
the past years to change open and create to an interface that doesn't
use only filenames, because filenames cover only part of the possibe
block devices.

So I'm kind of hesitant to add a new interface that can only use
filenames, while we know that filenames just don't quite cut it in the
general case - especially if using a BDS, which already has all the
information needed, is possible as an alternative.

>      /*
>       * Flushes all data that was already written to the OS all the way down to
>       * the disk (for example file-posix.c calls fsync()).
> -- 
> 2.20.1
> 
> diff --git a/block.c b/block.c
> index c139540f2b..6e2b0f528d 100644
> --- a/block.c
> +++ b/block.c
> @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
>  #endif
>  }
>  
> +/**
> + * Helper that checks if a given string represents a regular
> + * local file.
> + */
> +bool bdrv_path_is_regular_file(const char *path)
> +{
> +    struct stat st;
> +
> +    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
> +}
> +
>  /*
>   * Detect host devices. By convention, /dev/cdrom[N] is always
>   * recognized as a host CDROM.

This hunk isn't generic, it belong in file-posix.c

Kevin