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

[PATCH v2 0/2] 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 initilization fails (e.g due to bad encryption secret)

This gives the qcow2 the same treatment as to luks.

V2: added a patch to fix a memory leak.

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

Maxim Levitsky (2):
  crypto: luks: fix tiny memory leak
  block: qcow2: remove the created file on initialization error

 block/crypto.c |  1 +
 block/qcow2.c  | 12 ++++++++++++
 2 files changed, 13 insertions(+)

-- 
2.26.2

[PATCH v2 1/2] crypto: luks: fix tiny memory leak

[Maxim Levitsky]

In the case when underlying block device doesn't support the
bdrv_co_delete_file interface, an 'Error' wasn't freed.

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

diff --git a/block/crypto.c b/block/crypto.c
index 0807557763..9b61fd4aa8 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -736,6 +736,7 @@ fail:
         if ((r_del < 0) && (r_del != -ENOTSUP)) {
             error_report_err(local_delete_err);
         }
+        error_free(local_delete_err);
     }
 
     bdrv_unref(bs);
-- 
2.26.2

[PATCH v2 2/2] block: qcow2: remove the created file on initialization error

[Maxim Levitsky]

If the qcow initialization fails after we created the storage file,
we should remove it 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index b05512718c..4dc6102df8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3834,6 +3834,18 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
     /* Create the qcow2 image (format layer) */
     ret = qcow2_co_create(create_options, errp);
     if (ret < 0) {
+
+        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);
+        }
+        error_free(local_delete_err);
         goto finish;
     }
 
-- 
2.26.2

Re: [PATCH v2 1/2] crypto: luks: fix tiny memory leak

[Alberto Garcia]

On Sun 11 Oct 2020 12:21:35 PM CEST, Maxim Levitsky wrote:
> In the case when underlying block device doesn't support the
> bdrv_co_delete_file interface, an 'Error' wasn't freed.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/block/crypto.c b/block/crypto.c
> index 0807557763..9b61fd4aa8 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -736,6 +736,7 @@ fail:
>          if ((r_del < 0) && (r_del != -ENOTSUP)) {
>              error_report_err(local_delete_err);
>          }
> +        error_free(local_delete_err);
>      }

error_report_err() already calls error_free().

Berto

Re: [PATCH v2 1/2] crypto: luks: fix tiny memory leak

[Maxim Levitsky]

On Tue, 2020-10-13 at 14:26 +0200, Alberto Garcia wrote:
> On Sun 11 Oct 2020 12:21:35 PM CEST, Maxim Levitsky wrote:
> > In the case when underlying block device doesn't support the
> > bdrv_co_delete_file interface, an 'Error' wasn't freed.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 0807557763..9b61fd4aa8 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -736,6 +736,7 @@ fail:
> >          if ((r_del < 0) && (r_del != -ENOTSUP)) {
> >              error_report_err(local_delete_err);
> >          }
> > +        error_free(local_delete_err);
> >      }
> 
> error_report_err() already calls error_free().

Thanks. I didn't knew this.
Will send V3 soon.

Best regards,
	Maxim Levitsky
> 
> Berto
>