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

[Kevin Wolf]

Am 25.03.2019 um 13:18 hat Daniel P. Berrangé geschrieben:
> On Mon, Mar 25, 2019 at 01:10:46PM +0100, 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.
> > 
> > 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.
> 
> I thought it was previously suggested that we did *not* want to
> put the calls in that kind of place, as it would lead us to delete
> files that the user had pre-created ?

We definitely don't want to put them in .bdrv_co_create (i.e. the
blockdev-create path) because there we only got an opened block node
passed and we can't just destroy that node if we can't create a format
on it.

Here I'm suggesting .bdrv_co_create_opts, though, which has an explicit
bdrv_create_file() call in all format drivers (it creates both protocol
and format layer at once; this is what qemu-img create/convert use). At
the point where bdrv_create_file() returns, the original file is already
overwritten or at least truncated, so removing the file in an error path
even if it existed before could be reasonable enough.

Kevin