[PATCH] block/snapshot: Clarify goto fallback behavior

[PATCH] block/snapshot: Clarify goto fallback behavior

[Max Reitz]

In the bdrv_snapshot_goto() fallback code, we work with a pointer to
either bs->file or bs->backing.  We close that child, close the node
(with .bdrv_close()), apply the snapshot on the child node, and then
re-open the node (with .bdrv_open()).

In order for .bdrv_open() to attach the same child node that we had
before, we pass "file={child-node}" or "backing={child-node}" to it.
Therefore, when .bdrv_open() has returned success, we can assume that
bs->file or bs->backing (respectively) points to our original child
again.  This is verified by an assertion.

All of this is not immediately clear from a quick glance at the code,
so add a comment to the assertion what it is for, and why it is valid.
It certainly confused Coverity.

Reported-by: Coverity (CID 1452774)
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/snapshot.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index e8ae9a28c1..cce5e35b35 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         qobject_unref(file_options);
         g_free(subqdict_prefix);
 
+        /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr */
         qdict_put_str(options, (*fallback_ptr)->name,
                       bdrv_get_node_name(fallback_bs));
 
+        /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
         if (drv->bdrv_close) {
             drv->bdrv_close(bs);
         }
 
+        /* .bdrv_open() will re-attach it */
         bdrv_unref_child(bs, *fallback_ptr);
         *fallback_ptr = NULL;
 
@@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
             return ret < 0 ? ret : open_ret;
         }
 
-        assert(fallback_bs == (*fallback_ptr)->bs);
+        /*
+         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
+         * was closed above and set to NULL, but the .bdrv_open() call
+         * has opened it again, because we set the respective option
+         * (with the qdict_put_str() call above).
+         * Assert that .bdrv_open() has attached some child on
+         * *fallback_ptr, and that it has attached the one we wanted
+         * it to (i.e., fallback_bs).
+         */
+        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
         bdrv_unref(fallback_bs);
         return ret;
     }
-- 
2.31.1

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Vladimir Sementsov-Ogievskiy]

03.05.2021 12:54, Max Reitz wrote:
> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
> either bs->file or bs->backing.  

> We close that child,

Do we?

> close the node
> (with .bdrv_close()), apply the snapshot on the child node, and then
> re-open the node (with .bdrv_open()).
> 
> In order for .bdrv_open() to attach the same child node that we had
> before, we pass "file={child-node}" or "backing={child-node}" to it.
> Therefore, when .bdrv_open() has returned success, we can assume that
> bs->file or bs->backing (respectively) points to our original child
> again.  This is verified by an assertion.
> 
> All of this is not immediately clear from a quick glance at the code,
> so add a comment to the assertion what it is for, and why it is valid.
> It certainly confused Coverity.
> 
> Reported-by: Coverity (CID 1452774)
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/snapshot.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index e8ae9a28c1..cce5e35b35 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>           qobject_unref(file_options);
>           g_free(subqdict_prefix);
>   
> +        /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr */
>           qdict_put_str(options, (*fallback_ptr)->name,
>                         bdrv_get_node_name(fallback_bs));
>   
> +        /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
>           if (drv->bdrv_close) {
>               drv->bdrv_close(bs);
>           }
>   
> +        /* .bdrv_open() will re-attach it */
>           bdrv_unref_child(bs, *fallback_ptr);
>           *fallback_ptr = NULL;
>   
> @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>               return ret < 0 ? ret : open_ret;
>           }
>   
> -        assert(fallback_bs == (*fallback_ptr)->bs);
> +        /*
> +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
> +         * was closed above and set to NULL, but the .bdrv_open() call
> +         * has opened it again, because we set the respective option
> +         * (with the qdict_put_str() call above).
> +         * Assert that .bdrv_open() has attached some child on
> +         * *fallback_ptr, and that it has attached the one we wanted
> +         * it to (i.e., fallback_bs).
> +         */
> +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>           bdrv_unref(fallback_bs);
>           return ret;
>       }
> 

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

===

I still think that this fallback has more problems.. Nothing guarantee that all format drivers (even restricted to those who have only one child) support such logic.

For example, .bdrv_open() may rely on state structure were zeroed and not initialize some fields. And .bdrv_close() may just g_free() some things, not setting them to zero.. So we probably should call bdrv_open()/bdrv_close() generic functions. But we can't: at least bdrv_close() requires that node has no parents.

Not saying about that we lose everything on failure (when actually, it's better to restore original state on failure).

This feature should instead be done with help of bdrv_reopen_multiple(), and even with it it's not obvious how to do it properly.

The feature were done long ago in 2010 with commit 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.

Note also, that the only protocol driver that support snapshots is rbd, and snapshot support was added to it in 2012 with commit bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.

Other two drivers with support are sheepdog (which is deprecated) and qcow2 (I doubt that it should be used as protocol driver for some other format).


Do we really need this fallback? Is it used somewhere? May be, we can just deprecate it, and look will someone complain?


-- 
Best regards,
Vladimir

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Max Reitz]

On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
> 03.05.2021 12:54, Max Reitz wrote:
>> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
>> either bs->file or bs->backing. 
> 
>> We close that child,
> 
> Do we?

We *detach it.

>> close the node
>> (with .bdrv_close()), apply the snapshot on the child node, and then
>> re-open the node (with .bdrv_open()).
>>
>> In order for .bdrv_open() to attach the same child node that we had
>> before, we pass "file={child-node}" or "backing={child-node}" to it.
>> Therefore, when .bdrv_open() has returned success, we can assume that
>> bs->file or bs->backing (respectively) points to our original child
>> again.  This is verified by an assertion.
>>
>> All of this is not immediately clear from a quick glance at the code,
>> so add a comment to the assertion what it is for, and why it is valid.
>> It certainly confused Coverity.
>>
>> Reported-by: Coverity (CID 1452774)
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/snapshot.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index e8ae9a28c1..cce5e35b35 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>           qobject_unref(file_options);
>>           g_free(subqdict_prefix);
>> +        /* Force .bdrv_open() below to re-attach fallback_bs on 
>> *fallback_ptr */
>>           qdict_put_str(options, (*fallback_ptr)->name,
>>                         bdrv_get_node_name(fallback_bs));
>> +        /* Now close bs, apply the snapshot on fallback_bs, and 
>> re-open bs */
>>           if (drv->bdrv_close) {
>>               drv->bdrv_close(bs);
>>           }
>> +        /* .bdrv_open() will re-attach it */
>>           bdrv_unref_child(bs, *fallback_ptr);
>>           *fallback_ptr = NULL;
>> @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>               return ret < 0 ? ret : open_ret;
>>           }
>> -        assert(fallback_bs == (*fallback_ptr)->bs);
>> +        /*
>> +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
>> +         * was closed above and set to NULL, but the .bdrv_open() call
>> +         * has opened it again, because we set the respective option
>> +         * (with the qdict_put_str() call above).
>> +         * Assert that .bdrv_open() has attached some child on
>> +         * *fallback_ptr, and that it has attached the one we wanted
>> +         * it to (i.e., fallback_bs).
>> +         */
>> +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>>           bdrv_unref(fallback_bs);
>>           return ret;
>>       }
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> ===
> 
> I still think that this fallback has more problems.. Nothing guarantee 
> that all format drivers (even restricted to those who have only one 
> child) support such logic.
> 
> For example, .bdrv_open() may rely on state structure were zeroed and 
> not initialize some fields. And .bdrv_close() may just g_free() some 
> things, not setting them to zero.. So we probably should call 
> bdrv_open()/bdrv_close() generic functions. But we can't: at least 
> bdrv_close() requires that node has no parents.
> 
> Not saying about that we lose everything on failure (when actually, it's 
> better to restore original state on failure).
> 
> This feature should instead be done with help of bdrv_reopen_multiple(), 
> and even with it it's not obvious how to do it properly.
> 
> The feature were done long ago in 2010 with commit 
> 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
> 
> Note also, that the only protocol driver that support snapshots is rbd, 
> and snapshot support was added to it in 2012 with commit 
> bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
> 
> Other two drivers with support are sheepdog (which is deprecated) and 
> qcow2 (I doubt that it should be used as protocol driver for some other 
> format).
> 
> 
> Do we really need this fallback? Is it used somewhere? May be, we can 
> just deprecate it, and look will someone complain?

Maybe? :)

Max

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Vladimir Sementsov-Ogievskiy]

05.05.2021 19:25, Max Reitz wrote:
> On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
>> 03.05.2021 12:54, Max Reitz wrote:
>>> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
>>> either bs->file or bs->backing. 
>>
>>> We close that child,
>>
>> Do we?
> 
> We *detach it.
> 
>>> close the node
>>> (with .bdrv_close()), apply the snapshot on the child node, and then
>>> re-open the node (with .bdrv_open()).
>>>
>>> In order for .bdrv_open() to attach the same child node that we had
>>> before, we pass "file={child-node}" or "backing={child-node}" to it.
>>> Therefore, when .bdrv_open() has returned success, we can assume that
>>> bs->file or bs->backing (respectively) points to our original child
>>> again.  This is verified by an assertion.
>>>
>>> All of this is not immediately clear from a quick glance at the code,
>>> so add a comment to the assertion what it is for, and why it is valid.
>>> It certainly confused Coverity.
>>>
>>> Reported-by: Coverity (CID 1452774)
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block/snapshot.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>> index e8ae9a28c1..cce5e35b35 100644
>>> --- a/block/snapshot.c
>>> +++ b/block/snapshot.c
>>> @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>           qobject_unref(file_options);
>>>           g_free(subqdict_prefix);
>>> +        /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr */
>>>           qdict_put_str(options, (*fallback_ptr)->name,
>>>                         bdrv_get_node_name(fallback_bs));
>>> +        /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
>>>           if (drv->bdrv_close) {
>>>               drv->bdrv_close(bs);
>>>           }
>>> +        /* .bdrv_open() will re-attach it */
>>>           bdrv_unref_child(bs, *fallback_ptr);
>>>           *fallback_ptr = NULL;
>>> @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>               return ret < 0 ? ret : open_ret;
>>>           }
>>> -        assert(fallback_bs == (*fallback_ptr)->bs);
>>> +        /*
>>> +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
>>> +         * was closed above and set to NULL, but the .bdrv_open() call
>>> +         * has opened it again, because we set the respective option
>>> +         * (with the qdict_put_str() call above).
>>> +         * Assert that .bdrv_open() has attached some child on
>>> +         * *fallback_ptr, and that it has attached the one we wanted
>>> +         * it to (i.e., fallback_bs).
>>> +         */
>>> +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>>>           bdrv_unref(fallback_bs);
>>>           return ret;
>>>       }
>>>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> ===
>>
>> I still think that this fallback has more problems.. Nothing guarantee that all format drivers (even restricted to those who have only one child) support such logic.
>>
>> For example, .bdrv_open() may rely on state structure were zeroed and not initialize some fields. And .bdrv_close() may just g_free() some things, not setting them to zero.. So we probably should call bdrv_open()/bdrv_close() generic functions. But we can't: at least bdrv_close() requires that node has no parents.
>>
>> Not saying about that we lose everything on failure (when actually, it's better to restore original state on failure).
>>
>> This feature should instead be done with help of bdrv_reopen_multiple(), and even with it it's not obvious how to do it properly.
>>
>> The feature were done long ago in 2010 with commit 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
>>
>> Note also, that the only protocol driver that support snapshots is rbd, and snapshot support was added to it in 2012 with commit bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
>>
>> Other two drivers with support are sheepdog (which is deprecated) and qcow2 (I doubt that it should be used as protocol driver for some other format).
>>
>>
>> Do we really need this fallback? Is it used somewhere? May be, we can just deprecate it, and look will someone complain?
> 
> Maybe? :)
> 

:) I'll send a patch later.


-- 
Best regards,
Vladimir

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Vladimir Sementsov-Ogievskiy]

05.05.2021 23:37, Vladimir Sementsov-Ogievskiy wrote:
> 05.05.2021 19:25, Max Reitz wrote:
>> On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
>>> 03.05.2021 12:54, Max Reitz wrote:
>>>> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
>>>> either bs->file or bs->backing. 
>>>
>>>> We close that child,
>>>
>>> Do we?
>>
>> We *detach it.
>>
>>>> close the node
>>>> (with .bdrv_close()), apply the snapshot on the child node, and then
>>>> re-open the node (with .bdrv_open()).
>>>>
>>>> In order for .bdrv_open() to attach the same child node that we had
>>>> before, we pass "file={child-node}" or "backing={child-node}" to it.
>>>> Therefore, when .bdrv_open() has returned success, we can assume that
>>>> bs->file or bs->backing (respectively) points to our original child
>>>> again.  This is verified by an assertion.
>>>>
>>>> All of this is not immediately clear from a quick glance at the code,
>>>> so add a comment to the assertion what it is for, and why it is valid.
>>>> It certainly confused Coverity.
>>>>
>>>> Reported-by: Coverity (CID 1452774)
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>   block/snapshot.c | 14 +++++++++++++-
>>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>>> index e8ae9a28c1..cce5e35b35 100644
>>>> --- a/block/snapshot.c
>>>> +++ b/block/snapshot.c
>>>> @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>>           qobject_unref(file_options);
>>>>           g_free(subqdict_prefix);
>>>> +        /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr */
>>>>           qdict_put_str(options, (*fallback_ptr)->name,
>>>>                         bdrv_get_node_name(fallback_bs));
>>>> +        /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
>>>>           if (drv->bdrv_close) {
>>>>               drv->bdrv_close(bs);
>>>>           }
>>>> +        /* .bdrv_open() will re-attach it */
>>>>           bdrv_unref_child(bs, *fallback_ptr);
>>>>           *fallback_ptr = NULL;
>>>> @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>>>               return ret < 0 ? ret : open_ret;
>>>>           }
>>>> -        assert(fallback_bs == (*fallback_ptr)->bs);
>>>> +        /*
>>>> +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
>>>> +         * was closed above and set to NULL, but the .bdrv_open() call
>>>> +         * has opened it again, because we set the respective option
>>>> +         * (with the qdict_put_str() call above).
>>>> +         * Assert that .bdrv_open() has attached some child on
>>>> +         * *fallback_ptr, and that it has attached the one we wanted
>>>> +         * it to (i.e., fallback_bs).
>>>> +         */
>>>> +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
>>>>           bdrv_unref(fallback_bs);
>>>>           return ret;
>>>>       }
>>>>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> ===
>>>
>>> I still think that this fallback has more problems.. Nothing guarantee that all format drivers (even restricted to those who have only one child) support such logic.
>>>
>>> For example, .bdrv_open() may rely on state structure were zeroed and not initialize some fields. And .bdrv_close() may just g_free() some things, not setting them to zero.. So we probably should call bdrv_open()/bdrv_close() generic functions. But we can't: at least bdrv_close() requires that node has no parents.
>>>
>>> Not saying about that we lose everything on failure (when actually, it's better to restore original state on failure).
>>>
>>> This feature should instead be done with help of bdrv_reopen_multiple(), and even with it it's not obvious how to do it properly.
>>>
>>> The feature were done long ago in 2010 with commit 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
>>>
>>> Note also, that the only protocol driver that support snapshots is rbd, and snapshot support was added to it in 2012 with commit bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
>>>
>>> Other two drivers with support are sheepdog (which is deprecated) and qcow2 (I doubt that it should be used as protocol driver for some other format).
>>>
>>>
>>> Do we really need this fallback? Is it used somewhere? May be, we can just deprecate it, and look will someone complain?
>>
>> Maybe? :)
>>
> 
> :) I'll send a patch later.
> 
> 

Or not.. We need first some clean snapshot qmp api. And blockdev-reopen. And then just deprecate old hmp snapshot api.


-- 
Best regards,
Vladimir

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Peter Maydell]

On Mon, 3 May 2021 at 10:55, Max Reitz <mreitz@redhat.com> wrote:
>
> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
> either bs->file or bs->backing.  We close that child, close the node
> (with .bdrv_close()), apply the snapshot on the child node, and then
> re-open the node (with .bdrv_open()).
>
> In order for .bdrv_open() to attach the same child node that we had
> before, we pass "file={child-node}" or "backing={child-node}" to it.
> Therefore, when .bdrv_open() has returned success, we can assume that
> bs->file or bs->backing (respectively) points to our original child
> again.  This is verified by an assertion.
>
> All of this is not immediately clear from a quick glance at the code,
> so add a comment to the assertion what it is for, and why it is valid.
> It certainly confused Coverity.
>
> Reported-by: Coverity (CID 1452774)
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Did this patch get lost? I was just going through outstanding
coverity issues and noticed it was posted a month ago and not
in master...

thanks
-- PMM

Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Max Reitz]

On 03.06.21 18:02, Peter Maydell wrote:
> On Mon, 3 May 2021 at 10:55, Max Reitz <mreitz@redhat.com> wrote:
>> In the bdrv_snapshot_goto() fallback code, we work with a pointer to
>> either bs->file or bs->backing.  We close that child, close the node
>> (with .bdrv_close()), apply the snapshot on the child node, and then
>> re-open the node (with .bdrv_open()).
>>
>> In order for .bdrv_open() to attach the same child node that we had
>> before, we pass "file={child-node}" or "backing={child-node}" to it.
>> Therefore, when .bdrv_open() has returned success, we can assume that
>> bs->file or bs->backing (respectively) points to our original child
>> again.  This is verified by an assertion.
>>
>> All of this is not immediately clear from a quick glance at the code,
>> so add a comment to the assertion what it is for, and why it is valid.
>> It certainly confused Coverity.
>>
>> Reported-by: Coverity (CID 1452774)
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Did this patch get lost? I was just going through outstanding
> coverity issues and noticed it was posted a month ago and not
> in master...

Oh, right, sorry.  Thanks for the reminder.

Now applied to my block branch:

https://github.com/XanClic/qemu/commits/block