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é <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block.c                   | 45 +++++++++++++++++++++++++++++++++++++++
 block/crypto.c            |  2 ++
 include/block/block.h     |  3 +++
 include/block/block_int.h |  6 ++++++
 4 files changed, 56 insertions(+)

diff --git a/block.c b/block.c
index 0a93ee9ac8..2b632baba2 100644
--- a/block.c
+++ b/block.c
@@ -547,6 +547,51 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+/**
+ * Helper that checks if a given path 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);
+}
+
+/**
+ * Co-routine function that erases a regular file. Its original
+ * intent is as a implementation of bdrv_co_delete_file for
+ * the "luks" driver that can leave created files behind in the
+ * file system when the authentication fails.
+ *
+ * The function is exposed here, and with 'generic' in its name,
+ * because file removal isn't usually format specific and any other
+ * BlockDriver might want to re-use this function.
+ */
+int coroutine_fn bdrv_co_delete_file_generic(const char *filename,
+                                             Error **errp)
+{
+    int ret;
+
+    /* Skip file: protocol prefix */
+    strstart(filename, "file:", &filename);
+
+    if (!bdrv_path_is_regular_file(filename)) {
+        ret = -ENOENT;
+        error_setg_errno(errp, -ret, "%s is not a regular file", filename);
+        goto done;
+    }
+
+    ret = unlink(filename);
+    if (ret < 0) {
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Error when deleting file %s", filename);
+    }
+
+done:
+    return ret;
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/block/crypto.c b/block/crypto.c
index 3af46b805f..c604c96c93 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -639,6 +639,8 @@ static BlockDriver bdrv_crypto_luks = {
     .bdrv_co_truncate   = block_crypto_co_truncate,
     .create_opts        = &block_crypto_create_opts_luks,
 
+    .bdrv_co_delete_file = bdrv_co_delete_file_generic,
+
     .bdrv_reopen_prepare = block_crypto_reopen_prepare,
     .bdrv_refresh_limits = block_crypto_refresh_limits,
     .bdrv_co_preadv     = block_crypto_co_preadv,
diff --git a/include/block/block.h b/include/block/block.h
index e452988b66..efb77daf9f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -363,6 +363,9 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
 
+bool bdrv_path_is_regular_file(const char *path);
+int coroutine_fn bdrv_co_delete_file_generic(const char *filename,
+                                             Error **errp);
 
 typedef struct BdrvCheckResult {
     int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01e855a066..74abb78ce7 100644
--- 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);
+
     /*
      * 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

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é <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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.

Kevin

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é <berrange@redhat.com>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 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 ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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é <berrange@redhat.com>
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > 
> > 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

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é <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 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