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

[PATCH v5 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.

V5: addresssed review feedback on reworked version.

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               | 24 ++++++++++++++++++++++++
 block/crypto.c        | 13 ++-----------
 block/qcow2.c         |  6 ++++--
 include/block/block.h |  1 +
 4 files changed, 31 insertions(+), 13 deletions(-)

-- 
2.26.2

[PATCH v5 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 v5 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 24 ++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/block.c b/block.c
index f1cedac362..5d35ba2fb8 100644
--- a/block.c
+++ b/block.c
@@ -704,6 +704,30 @@ 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/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 v5 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 v5 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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

Re: [PATCH v5 1/4] crypto: luks: Fix tiny memory leak

[Vladimir Sementsov-Ogievskiy]

09.12.2020 23:33, Maxim Levitsky wrote:
> 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);
>           }
>       }
>   
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

09.12.2020 23:33, 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.

I've looked at original commit added this logic, and didn't find exactly, why should we hide it..

> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   block.c               | 24 ++++++++++++++++++++++++
>   include/block/block.h |  1 +
>   2 files changed, 25 insertions(+)
> 
> diff --git a/block.c b/block.c
> index f1cedac362..5d35ba2fb8 100644
> --- a/block.c
> +++ b/block.c
> @@ -704,6 +704,30 @@ 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.
> +     */

It's not predictable for user: user doesn't know internal processes and interfaces.

The problem: we've left extra file on failure scenario and can't remove it. We want to warn the user that there is a wrong file left. Why not to warn, when the removement is unsupported internally by the driver?

I think, it's better to report it in any case.

Another reason: less code and logic on failure case. Optimizing failure scenarios in different manner is seldom a good thing to do.

And finally: why to report the error at all? If we have errp parameter, it's better to add the info to it using error_append.. something like error_append(errp, " (also failed to remove unfinished file %s: %s)", file_name, error_get_pretty(local_err))


Probably I'm making mountains out of molehills. It should work, so take my r-b anyway:

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


Side note: I'd not split patches 02 and 03: this way it would be simpler to see the code movement.

> +    if (ret == -ENOTSUP) {
> +        error_free(local_err);
> +    } else if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +}
> +
> +
> +

three empty lines..

>   /**
>    * 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 {
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

09.12.2020 23:33, 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>

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

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

09.12.2020 23:33, 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>
> ---
>   block/qcow2.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3a90ef2786..68c9182f92 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);
>       }
>   
>       ret = 0;

Ooops :) After this patch the function always returns zero :)

> -finish:
> +
>       qobject_unref(qdict);
>       bdrv_unref(bs);
>       bdrv_unref(data_bs);
> 


-- 
Best regards,
Vladimir

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

[Maxim Levitsky]

On Thu, 2020-12-10 at 14:00 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2020 23:33, 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>
> > ---
> >   block/qcow2.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 3a90ef2786..68c9182f92 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);
> >       }
> >   
> >       ret = 0;
> 
> Ooops :) After this patch the function always returns zero :)

Indeed :-(

These small changes done in the last minute are the most dangerous.

Best regards,
	Maxim Levitsky
> 
> > -finish:
> > +
> >       qobject_unref(qdict);
> >       bdrv_unref(bs);
> >       bdrv_unref(data_bs);
> > 
> 
> 

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

[Maxim Levitsky]

On Thu, 2020-12-10 at 13:54 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 09.12.2020 23:33, 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.
> 
> I've looked at original commit added this logic, and didn't find exactly, why should we hide it..
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > ---
> >   block.c               | 24 ++++++++++++++++++++++++
> >   include/block/block.h |  1 +
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index f1cedac362..5d35ba2fb8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -704,6 +704,30 @@ 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.
> > +     */
> 
> It's not predictable for user: user doesn't know internal processes and interfaces.
> 
> The problem: we've left extra file on failure scenario and can't remove it. We want to warn the user that there is a wrong file left. Why not to warn, when the removement is unsupported internally by the driver?
> 
> I think, it's better to report it in any case.

This is a good point, but it is not something I changed.
I prefer this to be changed in a separate patch.


> 
> Another reason: less code and logic on failure case. Optimizing failure scenarios in different manner is seldom a good thing to do.
> 
> And finally: why to report the error at all? If we have errp parameter, it's better to add the info to it using error_append.. something like error_append(errp, " (also failed to remove unfinished file %s: %s)", file_name, error_get_pretty(local_err))

The idea behind this was to reduce code duplication in the callers of this function,
and this is why this function doesn't even have the 'errp' parameter.
I'll think about it.

> 
> 
> Probably I'm making mountains out of molehills. It should work, so take my r-b anyway:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> Side note: I'd not split patches 02 and 03: this way it would be simpler to see the code movement.
> 
> > +    if (ret == -ENOTSUP) {
> > +        error_free(local_err);
> > +    } else if (ret < 0) {
> > +        error_report_err(local_err);
> > +    }
> > +}
> > +
> > +
> > +
> 
> three empty lines..

Will fix.
> 
> >   /**
> >    * 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 {
> > 

Thanks a lot for the review!

Best regards,
	Maxim Levitsky

> 
>