Re: [Qemu-devel] [PATCH v2 1/3] block: introducing 'bdrv_co_delete_file' interface

[Daniel Henrique Barboza]

On 3/25/19 9:10 AM, Kevin Wolf wrote:
> Am 22.03.2019 um 18:52 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 LUKS driver. The implementation is provided
> > in a new 'bdrv_co_delete_file_generic' function inside block.c. This
> > function is made public in case other block drivers wants to
> > support this cleanup interface as well.
> >
> > Suggested-by: Daniel P. Berrangé <address@hidden>
> > Signed-off-by: Daniel Henrique Barboza <address@hidden>
> This is still wrong. Consider a LUKS image that is accessed via http://
> rather than in a local file.
>
> Instead of the "generic" implementation in block.c (which isn't actually
> generic, but very specific to local files), this needs to be the
> implementation for the file driver in block/file-posix.c.

I'll move the code there in the next version.

> The call of bdrv_co_delete_file() must then be in the error path of the
> same function that called bdrv_create_file(), such as
> block_crypto_co_create_opts_luks() in your specific example.


That makes sense for the problem that I'm aiming to fix.

However, since we're moving the code to file-posix.c, keeping the
bdrv_co_delete_file() in img_create will fix the issue for all other
formats that ends up using the file driver as well. ATM I have no
evidence that this error might happen with anything but LUKS,
but if/when it does, the code can handle it.

Now, making a counter-argument for what I just wrote, there are
more callers of bdrv_create besides img_create:

- bdrv_append_temp_snapshot
- bdrv_create_file
- bdrv_img_create
- enable_write_target (vvfat)
- img_convert
- img_dd


Moving the delete code from img_create to 
block_crypto_co_create_opts_luks(),
as you suggested,  will fix the case we know it can be problematic for these
callers as well. The drawback is that we will need to check inside LUKS if
the local file exists beforehand though.



Thanks,


dhb



> Kevin