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

[PATCH v4 0/4] 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.

Best regards,
	Maxim Levitsky

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

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

-- 
2.26.2

[PATCH v4 1/4] 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>
---
 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 v4 2/4] 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.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block.c               | 23 +++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/block.c b/block.c
index f1cedac362..57e6d9750a 100644
--- a/block.c
+++ b/block.c
@@ -704,6 +704,29 @@ 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;
+
+    if (!bs) {
+        return;
+    }
+
+    int 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/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 v4 3/4] crypto: luks: use bdrv_co_delete_file_noerr

[Maxim Levitsky]

This refactoring is now possible thanks to this function.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

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);
-- 
2.26.2

[PATCH v4 4/4] 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>
---
 block/qcow2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3a90ef2786..b5169b7cad 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3847,12 +3847,13 @@ 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);
     }
 
-    ret = 0;
-finish:
     qobject_unref(qdict);
     bdrv_unref(bs);
     bdrv_unref(data_bs);
-- 
2.26.2

Re: [PATCH v4 2/4] block: add bdrv_co_delete_file_noerr

[Alberto Garcia]

On Wed 09 Dec 2020 05:44:39 PM CET, Maxim Levitsky wrote:
> +void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs)
> +{
> +    Error *local_err = NULL;
> +
> +    if (!bs) {
> +        return;
> +    }
> +
> +    int ret = bdrv_co_delete_file(bs, &local_err);
       ^^^

According to the QEMU coding style we should not have declarations in
the middle of a block.

The patch looks otherwise fine.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v4 3/4] crypto: luks: use bdrv_co_delete_file_noerr

[Alberto Garcia]

On Wed 09 Dec 2020 05:44:40 PM CET, Maxim Levitsky wrote:
> This refactoring is now possible thanks to this function.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v4 4/4] block: qcow2: remove the created file on initialization error

[Alberto Garcia]

On Wed 09 Dec 2020 05:44:41 PM CET, Maxim Levitsky wrote:
> @@ -3847,12 +3847,13 @@ 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);
>      }
>  
> -    ret = 0;

Many/most functions in qcow2.c force ret to be 0 on success, we could
also keep that here (although in practice I don't think that ret can be
greater than 0 in this case, or that the caller would care).

Either way,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v4 2/4] block: add bdrv_co_delete_file_noerr

[Maxim Levitsky]

On Wed, 2020-12-09 at 18:34 +0100, Alberto Garcia wrote:
> On Wed 09 Dec 2020 05:44:39 PM CET, Maxim Levitsky wrote:
> > +void coroutine_fn bdrv_co_delete_file_noerr(BlockDriverState *bs)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    if (!bs) {
> > +        return;
> > +    }
> > +
> > +    int ret = bdrv_co_delete_file(bs, &local_err);
>        ^^^
> 
> According to the QEMU coding style we should not have declarations in
> the middle of a block.

Oops!

I will send next version now.

Thanks a lot for the review!

Best regards,
	Maxim Levitsky

> 
> The patch looks otherwise fine.
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 

Re: [PATCH v4 4/4] block: qcow2: remove the created file on initialization error

[Maxim Levitsky]

On Wed, 2020-12-09 at 18:41 +0100, Alberto Garcia wrote:
> On Wed 09 Dec 2020 05:44:41 PM CET, Maxim Levitsky wrote:
> > @@ -3847,12 +3847,13 @@ 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);
> >      }
> >  
> > -    ret = 0;
> 
> Many/most functions in qcow2.c force ret to be 0 on success, we could
> also keep that here (although in practice I don't think that ret can be
> greater than 0 in this case, or that the caller would care).

I also noticed this when I was sending the patches, and I wasn't sure
if I want to keep that 'ret = 0' or not.
I will add it back.

Best regards,
	Maxim Levitsky

> 
> Either way,
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
>