[v5] Transactional block-graph modifying API

[PATCH v5 14/45] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr

Posted by Vladimir Sementsov-Ogievskiy 3 years, 8 months ago

Now the indirection is not actually used, we can safely reduce it to
simple pointer.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 block/snapshot.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 02a880911f..4eb9258de6 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
 }
 
 /**
- * Return a pointer to the child BDS pointer to which we can fall
+ * Return a pointer to child of given BDS to which we can fall
  * back if the given BDS does not support snapshots.
  * Return NULL if there is no BDS to (safely) fall back to.
- *
- * We need to return an indirect pointer because bdrv_snapshot_goto()
- * has to modify the BdrvChild pointer.
  */
-static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
+static BdrvChild *bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
 {
-    BdrvChild **fallback;
-    BdrvChild *child = bdrv_primary_child(bs);
+    BdrvChild *fallback = bdrv_primary_child(bs);
+    BdrvChild *child;
 
     /* We allow fallback only to primary child */
-    if (!child) {
+    if (!fallback) {
         return NULL;
     }
-    fallback = (child == bs->file ? &bs->file : &bs->backing);
-    assert(*fallback == child);
 
     /*
      * Check that there are no other children that would need to be
      * snapshotted.  If there are, it is not safe to fall back to
-     * *fallback.
+     * fallback.
      */
     QLIST_FOREACH(child, &bs->children, next) {
         if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
                            BDRV_CHILD_FILTERED) &&
-            child != *fallback)
+            child != fallback)
         {
             return NULL;
         }
@@ -189,8 +184,8 @@ static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
 
 static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
 {
-    BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);
-    return child_ptr ? (*child_ptr)->bs : NULL;
+    BdrvChild *child_ptr = bdrv_snapshot_fallback_ptr(bs);
+    return child_ptr ? child_ptr->bs : NULL;
 }
 
 int bdrv_can_snapshot(BlockDriverState *bs)
@@ -237,7 +232,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
                        Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    BdrvChild **fallback_ptr;
+    BdrvChild *fallback;
     int ret, open_ret;
 
     GLOBAL_STATE_CODE();
@@ -260,13 +255,13 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         return ret;
     }
 
-    fallback_ptr = bdrv_snapshot_fallback_ptr(bs);
-    if (fallback_ptr) {
+    fallback = bdrv_snapshot_fallback_ptr(bs);
+    if (fallback) {
         QDict *options;
         QDict *file_options;
         Error *local_err = NULL;
-        BlockDriverState *fallback_bs = (*fallback_ptr)->bs;
-        char *subqdict_prefix = g_strdup_printf("%s.", (*fallback_ptr)->name);
+        BlockDriverState *fallback_bs = fallback->bs;
+        char *subqdict_prefix = g_strdup_printf("%s.", fallback->name);
 
         options = qdict_clone_shallow(bs->options);
 
@@ -277,8 +272,8 @@ 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,
+        /* Force .bdrv_open() below to re-attach fallback_bs on fallback */
+        qdict_put_str(options, fallback->name,
                       bdrv_get_node_name(fallback_bs));
 
         /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
@@ -287,7 +282,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
         }
 
         /* .bdrv_open() will re-attach it */
-        bdrv_unref_child(bs, *fallback_ptr);
+        bdrv_unref_child(bs, fallback);
 
         ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
         open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
-- 
2.35.1

Re: [PATCH v5 14/45] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr

Posted by Hanna Reitz 3 years, 6 months ago

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> Now the indirection is not actually used, we can safely reduce it to
> simple pointer.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/snapshot.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)

Looks good, just wondering whether we should drop some of the "_ptr" 
suffixes now.

> diff --git a/block/snapshot.c b/block/snapshot.c
> index 02a880911f..4eb9258de6 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>   }
>   
>   /**
> - * Return a pointer to the child BDS pointer to which we can fall
> + * Return a pointer to child of given BDS to which we can fall
>    * back if the given BDS does not support snapshots.
>    * Return NULL if there is no BDS to (safely) fall back to.
> - *
> - * We need to return an indirect pointer because bdrv_snapshot_goto()
> - * has to modify the BdrvChild pointer.
>    */
> -static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
> +static BdrvChild *bdrv_snapshot_fallback_ptr(BlockDriverState *bs)

The _ptr part was meant to point out that it returns an indirect 
pointer; maybe we should name it bdrv_snapshot_fallback_child() now?

>   {
> -    BdrvChild **fallback;
> -    BdrvChild *child = bdrv_primary_child(bs);
> +    BdrvChild *fallback = bdrv_primary_child(bs);
> +    BdrvChild *child;
>   
>       /* We allow fallback only to primary child */
> -    if (!child) {
> +    if (!fallback) {
>           return NULL;
>       }
> -    fallback = (child == bs->file ? &bs->file : &bs->backing);
> -    assert(*fallback == child);
>   
>       /*
>        * Check that there are no other children that would need to be
>        * snapshotted.  If there are, it is not safe to fall back to
> -     * *fallback.
> +     * fallback.
>        */
>       QLIST_FOREACH(child, &bs->children, next) {
>           if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
>                              BDRV_CHILD_FILTERED) &&
> -            child != *fallback)
> +            child != fallback)
>           {
>               return NULL;
>           }
> @@ -189,8 +184,8 @@ static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
>   
>   static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
>   {
> -    BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);

Just "child" is enough (and better) now, I think.

> -    return child_ptr ? (*child_ptr)->bs : NULL;
> +    BdrvChild *child_ptr = bdrv_snapshot_fallback_ptr(bs);
> +    return child_ptr ? child_ptr->bs : NULL;
>   }
>   
>   int bdrv_can_snapshot(BlockDriverState *bs)

Re: [PATCH v5 14/45] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr

Posted by Vladimir Sementsov-Ogievskiy 3 years, 6 months ago

On 6/7/22 18:58, Hanna Reitz wrote:
> On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
>> Now the indirection is not actually used, we can safely reduce it to
>> simple pointer.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>>   block/snapshot.c | 39 +++++++++++++++++----------------------
>>   1 file changed, 17 insertions(+), 22 deletions(-)
> 
> Looks good, just wondering whether we should drop some of the "_ptr" suffixes now.
> 
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 02a880911f..4eb9258de6 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>   }
>>   /**
>> - * Return a pointer to the child BDS pointer to which we can fall
>> + * Return a pointer to child of given BDS to which we can fall
>>    * back if the given BDS does not support snapshots.
>>    * Return NULL if there is no BDS to (safely) fall back to.
>> - *
>> - * We need to return an indirect pointer because bdrv_snapshot_goto()
>> - * has to modify the BdrvChild pointer.
>>    */
>> -static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
>> +static BdrvChild *bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
> 
> The _ptr part was meant to point out that it returns an indirect pointer; maybe we should name it bdrv_snapshot_fallback_child() now?
> 
>>   {
>> -    BdrvChild **fallback;
>> -    BdrvChild *child = bdrv_primary_child(bs);
>> +    BdrvChild *fallback = bdrv_primary_child(bs);
>> +    BdrvChild *child;
>>       /* We allow fallback only to primary child */
>> -    if (!child) {
>> +    if (!fallback) {
>>           return NULL;
>>       }
>> -    fallback = (child == bs->file ? &bs->file : &bs->backing);
>> -    assert(*fallback == child);
>>       /*
>>        * Check that there are no other children that would need to be
>>        * snapshotted.  If there are, it is not safe to fall back to
>> -     * *fallback.
>> +     * fallback.
>>        */
>>       QLIST_FOREACH(child, &bs->children, next) {
>>           if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
>>                              BDRV_CHILD_FILTERED) &&
>> -            child != *fallback)
>> +            child != fallback)
>>           {
>>               return NULL;
>>           }
>> @@ -189,8 +184,8 @@ static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
>>   static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
>>   {
>> -    BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);
> 
> Just "child" is enough (and better) now, I think.
> 
>> -    return child_ptr ? (*child_ptr)->bs : NULL;
>> +    BdrvChild *child_ptr = bdrv_snapshot_fallback_ptr(bs);
>> +    return child_ptr ? child_ptr->bs : NULL;
>>   }
>>   int bdrv_can_snapshot(BlockDriverState *bs)
> 
> 

Agree to all comments, will do

-- 
Best regards,
Vladimir