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

[PATCH v3 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 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

Best regards,
	Maxim Levitsky

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

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

-- 
2.26.2

[PATCH v3 1/2] 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>
---
 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 v3 2/2] 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3a90ef2786..3bc2096b72 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3848,6 +3848,19 @@ 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);
+        } else {
+            error_free(local_delete_err);
+        }
         goto finish;
     }
 
-- 
2.26.2

Re: [PATCH v3 1/2] crypto: luks: Fix tiny memory leak

[Alberto Garcia]

On Tue 08 Dec 2020 03:21:58 PM CET, 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>

Berto

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

[Alberto Garcia]

On Tue 08 Dec 2020 03:21:59 PM CET, 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>

>      ret = qcow2_co_create(create_options, errp);
>      if (ret < 0) {
> +
> +        Error *local_delete_err = NULL;

Why that empty line though?

Berto

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

[Maxim Levitsky]

On Tue, 2020-12-08 at 16:26 +0100, Alberto Garcia wrote:
> On Tue 08 Dec 2020 03:21:59 PM CET, 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>
> 
> >      ret = qcow2_co_create(create_options, errp);
> >      if (ret < 0) {
> > +
> > +        Error *local_delete_err = NULL;
> 
> Why that empty line though?

I didn't notice. I can send a new version if this is needed.

Thanks for the review!

Best regards,
	Maxim Levitsky
> 
> Berto
> 

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

[Vladimir Sementsov-Ogievskiy]

08.12.2020 17:21, 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>
> ---
>   block/qcow2.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3a90ef2786..3bc2096b72 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3848,6 +3848,19 @@ 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);
> +        } else {
> +            error_free(local_delete_err);
> +        }
>           goto finish;
>       }
>   
> 

Hi!

As I understand, qcow2_co_create is a new interface and qcow2_co_create_opts() is old, and now works as a wrapper on qcow2_co_create.

I think it's better to do the cleanup in qcow2_co_create, to bring the feature both to new and old interface in the same way.


-- 
Best regards,
Vladimir

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

[Maxim Levitsky]

On Tue, 2020-12-08 at 18:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.12.2020 17:21, 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>
> > ---
> >   block/qcow2.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 3a90ef2786..3bc2096b72 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -3848,6 +3848,19 @@ 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);
> > +        } else {
> > +            error_free(local_delete_err);
> > +        }
> >           goto finish;
> >       }
> >   
> > 
> 
> Hi!
> 
> As I understand, qcow2_co_create is a new interface and qcow2_co_create_opts() is old, and now works as a wrapper on qcow2_co_create.
> 
> I think it's better to do the cleanup in qcow2_co_create, to bring the feature both to new and old interface in the same way.

I think that the new interface doesn't need this fix, since 
using the new interface is only possible from qmp which 
forces the user to explicitly create and open the file 
prior to formatting it with qcow2 format.

Thus it is logical to make the user remove it as well if creation fails.

Best regards,
	Maxim Levitsky

> 
> 

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

[Vladimir Sementsov-Ogievskiy]

08.12.2020 19:27, Maxim Levitsky wrote:
> On Tue, 2020-12-08 at 18:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 08.12.2020 17:21, 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>
>>> ---
>>>    block/qcow2.c | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 3a90ef2786..3bc2096b72 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -3848,6 +3848,19 @@ 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);
>>> +        } else {
>>> +            error_free(local_delete_err);
>>> +        }
>>>            goto finish;
>>>        }
>>>    
>>>
>>
>> Hi!
>>
>> As I understand, qcow2_co_create is a new interface and qcow2_co_create_opts() is old, and now works as a wrapper on qcow2_co_create.
>>
>> I think it's better to do the cleanup in qcow2_co_create, to bring the feature both to new and old interface in the same way.
> 
> I think that the new interface doesn't need this fix, since
> using the new interface is only possible from qmp which
> forces the user to explicitly create and open the file
> prior to formatting it with qcow2 format.
> 

Oh yes, you are right. File is created by bdrv_create_file() in qcow2_co_create_opts() not in qcow2_co_create(). Still, I think, you should remove the file on any failure after bdrv_create_file() call, but you remove it only on the last failure point..


-- 
Best regards,
Vladimir

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

[Maxim Levitsky]

On Tue, 2020-12-08 at 19:54 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 08.12.2020 19:27, Maxim Levitsky wrote:
> > On Tue, 2020-12-08 at 18:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > 08.12.2020 17:21, 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>
> > > > ---
> > > >    block/qcow2.c | 13 +++++++++++++
> > > >    1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > > index 3a90ef2786..3bc2096b72 100644
> > > > --- a/block/qcow2.c
> > > > +++ b/block/qcow2.c
> > > > @@ -3848,6 +3848,19 @@ 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);
> > > > +        } else {
> > > > +            error_free(local_delete_err);
> > > > +        }
> > > >            goto finish;
> > > >        }
> > > >    
> > > > 
> > > 
> > > Hi!
> > > 
> > > As I understand, qcow2_co_create is a new interface and qcow2_co_create_opts() is old, and now works as a wrapper on qcow2_co_create.
> > > 
> > > I think it's better to do the cleanup in qcow2_co_create, to bring the feature both to new and old interface in the same way.
> > 
> > I think that the new interface doesn't need this fix, since
> > using the new interface is only possible from qmp which
> > forces the user to explicitly create and open the file
> > prior to formatting it with qcow2 format.
> > 
> 
> Oh yes, you are right. File is created by bdrv_create_file() in qcow2_co_create_opts() not in qcow2_co_create(). Still, I think, you should remove the file on any failure after bdrv_create_file() call, but you remove it only on the last failure point..

You are right! The bulk of the code that can fail is in qcow2_co_create_opts but there 
are indeed few error conditions prior to that.

Thanks for pointing that out.
I'll fix that.

Best regards,
	Maxim Levitsky


> 
>