[PATCH v6 0/3] qcow2: don't leave partially initialized file on image creation

[PATCH v6 0/3] qcow2: don't leave partially initialized file on image creation

[Maxim Levitsky]

Use the bdrv_co_delete_file interface to delete the underlying
file if qcow2 initialization fails (e.g due to bad encryption secret)

This makes the qcow2 driver behave the same way as the luks driver behaves.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1845353

V3: addressed review feedback and reworked commit messages

V4: got rid of code duplication by adding bdrv_co_delete_file_noerr
and made the qcow2 driver use this function to delete
both the main and the data file.

V5: addresssed review feedback on reworked version.

V6: addressed most of the review feedback.

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  crypto: luks: Fix tiny memory leak
  block: add bdrv_co_delete_file_noerr
  block: qcow2: remove the created file on initialization error

 block.c               | 22 ++++++++++++++++++++++
 block/crypto.c        | 13 ++-----------
 block/qcow2.c         |  8 +++++---
 include/block/block.h |  1 +
 4 files changed, 30 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH v6 1/3] crypto: luks: Fix tiny memory leak

[Maxim Levitsky]

When the underlying block device doesn't support the
bdrv_co_delete_file interface, an 'Error' object was leaked.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index aef5a5721a..b3a5275132 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -735,6 +735,8 @@ fail:
          */
         if ((r_del < 0) && (r_del != -ENOTSUP)) {
             error_report_err(local_delete_err);
+        } else {
+            error_free(local_delete_err);
         }
     }
 
-- 
2.26.2

[PATCH v6 2/3] block: add bdrv_co_delete_file_noerr

[Maxim Levitsky]

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.

Use it in luks driver

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 22 ++++++++++++++++++++++
 block/crypto.c        | 15 ++-------------
 include/block/block.h |  1 +
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index f1cedac362..b9edea3b97 100644
--- a/block.c
+++ b/block.c
@@ -704,6 +704,28 @@ 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.
+     */
+    if (ret == -ENOTSUP) {
+        error_free(local_err);
+    } else if (ret < 0) {
+        error_report_err(local_err);
+    }
+}
+
 /**
  * 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 b3a5275132..1d30fde38e 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -725,19 +725,8 @@ fail:
      * If an error occurred, delete 'filename'. Even if the file existed
      * beforehand, it has been truncated and corrupted in the process.
      */
-    if (ret && bs) {
-        Error *local_delete_err = NULL;
-        int r_del = bdrv_co_delete_file(bs, &local_delete_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.
-         */
-        if ((r_del < 0) && (r_del != -ENOTSUP)) {
-            error_report_err(local_delete_err);
-        } else {
-            error_free(local_delete_err);
-        }
+    if (ret) {
+        bdrv_co_delete_file_noerr(bs);
     }
 
     bdrv_unref(bs);
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 {
-- 
2.26.2

[PATCH v6 3/3] block: qcow2: remove the created file on initialization error

[Maxim Levitsky]

If the qcow initialization fails, we should remove the file if it was
already created, to avoid leaving stale files around.

We already do this for luks raw images.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3a90ef2786..a8638d250a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3847,12 +3847,14 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
 
     /* Create the qcow2 image (format layer) */
     ret = qcow2_co_create(create_options, errp);
+finish:
     if (ret < 0) {
-        goto finish;
+        bdrv_co_delete_file_noerr(bs);
+        bdrv_co_delete_file_noerr(data_bs);
+    } else {
+        ret = 0;
     }
 
-    ret = 0;
-finish:
     qobject_unref(qdict);
     bdrv_unref(bs);
     bdrv_unref(data_bs);
-- 
2.26.2

Re: [PATCH v6 2/3] block: add bdrv_co_delete_file_noerr

[Vladimir Sementsov-Ogievskiy]

17.12.2020 20:09, 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.
> 
> Use it in luks driver
> 
> Signed-off-by: Maxim Levitsky<mlevitsk@redhat.com>
> Reviewed-by: Alberto Garcia<berto@igalia.com>

I still doubt that not reporting ENOTSUPP has a real benefit.. Still, it's preexisting.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v6 3/3] block: qcow2: remove the created file on initialization error

[Vladimir Sementsov-Ogievskiy]

17.12.2020 20:09, Maxim Levitsky wrote:
> If the qcow initialization fails, we should remove the file if it was
> already created, to avoid leaving stale files around.
> 
> We already do this for luks raw images.
> 
> Signed-off-by: Maxim Levitsky<mlevitsk@redhat.com>
> Reviewed-by: Alberto Garcia<berto@igalia.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v6 0/3] qcow2: don't leave partially initialized file on image creation

[Maxim Levitsky]

On Thu, 2020-12-17 at 19:09 +0200, Maxim Levitsky wrote:
> Use the bdrv_co_delete_file interface to delete the underlying
> file if qcow2 initialization fails (e.g due to bad encryption secret)
> 
> This makes the qcow2 driver behave the same way as the luks driver behaves.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1845353
> 
> V3: addressed review feedback and reworked commit messages
> 
> V4: got rid of code duplication by adding bdrv_co_delete_file_noerr
> and made the qcow2 driver use this function to delete
> both the main and the data file.
> 
> V5: addresssed review feedback on reworked version.
> 
> V6: addressed most of the review feedback.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (3):
>   crypto: luks: Fix tiny memory leak
>   block: add bdrv_co_delete_file_noerr
>   block: qcow2: remove the created file on initialization error
> 
>  block.c               | 22 ++++++++++++++++++++++
>  block/crypto.c        | 13 ++-----------
>  block/qcow2.c         |  8 +++++---
>  include/block/block.h |  1 +
>  4 files changed, 30 insertions(+), 14 deletions(-)
> 
> -- 
> 2.26.2
> 
> 
> 
Any update on this?

Best regards,
	Maxim Levitsky

Re: [PATCH v6 0/3] qcow2: don't leave partially initialized file on image creation

[Kevin Wolf]

Am 17.12.2020 um 18:09 hat Maxim Levitsky geschrieben:
> Use the bdrv_co_delete_file interface to delete the underlying
> file if qcow2 initialization fails (e.g due to bad encryption secret)
> 
> This makes the qcow2 driver behave the same way as the luks driver behaves.

Thanks, applied to the block branch.

Kevin