qcow2: cleanup created file when qcow2_co_create

[PATCH] qcow2: cleanup created file when qcow2_co_create

Posted by Maxim Levitsky 3 years, 9 months ago

This is basically the same thing as commit
'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
does but for qcow2 files to ensure that we don't leave qcow2 files
when creation fails.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index fadf3422f8..8b848924b5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3794,6 +3794,17 @@ 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);
+        }
         goto finish;
     }
 
-- 
2.26.2

Re: [PATCH] qcow2: cleanup created file when qcow2_co_create

Posted by Maxim Levitsky 3 years, 9 months ago

On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index fadf3422f8..8b848924b5 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3794,6 +3794,17 @@ 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);
> +        }
>          goto finish;
>      }
>  
Any update on this? Do you think we can add this to 5.1 or is it too late?
Best regards,
	Maxim Levitsky

Re: [PATCH] qcow2: cleanup created file when qcow2_co_create

Posted by Maxim Levitsky 3 years, 8 months ago

On Thu, 2020-07-16 at 14:33 +0300, Maxim Levitsky wrote:
> This is basically the same thing as commit
> 'crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails'
> does but for qcow2 files to ensure that we don't leave qcow2 files
> when creation fails.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Very kind ping on this patch.

Best regards,
	Maxim levitsky

> ---
>  block/qcow2.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index fadf3422f8..8b848924b5 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3794,6 +3794,17 @@ 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);
> +        }
>          goto finish;
>      }
>

Re: [PATCH] qcow2: cleanup created file when qcow2_co_create

Posted by Alberto Garcia 3 years, 8 months ago

On Thu 16 Jul 2020 01:33:59 PM CEST, Maxim Levitsky wrote:
>      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);
> +        }

I think you're leaking the error on ENOTSUP.

Berto

Re: [PATCH] qcow2: cleanup created file when qcow2_co_create

Posted by Maxim Levitsky 3 years, 6 months ago

On Tue, 2020-09-01 at 15:30 +0200, Alberto Garcia wrote:
> On Thu 16 Jul 2020 01:33:59 PM CEST, Maxim Levitsky wrote:
> >      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);
> > +        }
> 
> I think you're leaking the error on ENOTSUP.

Indeed, just like in the original code from where I copy&pasta'ed this from.
I'll send a patch to fix this and update the patch as well.

Thanks,
Best regards,
	Maxim Levitsky

> 
> Berto
>