diff mbox series

[v7,12/47] block: Use bdrv_filter_(bs|child) where obvious

Message ID 20200625152215.941773-13-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: Deal with filters | expand

Commit Message

Max Reitz June 25, 2020, 3:21 p.m. UTC 
Places that use patterns like

    if (bs->drv->is_filter && bs->file) {
        ... something about bs->file->bs ...
    }

should be

    BlockDriverState *filtered = bdrv_filter_bs(bs);
    if (filtered) {
        ... something about @filtered ...
    }

instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                        | 31 ++++++++++++++++++++-----------
 block/io.c                     |  7 +++++--
 migration/block-dirty-bitmap.c |  8 +-------
 3 files changed, 26 insertions(+), 20 deletions(-)

Comments

Andrey Shinkevich July 8, 2020, 6:24 p.m. UTC | #1 
On 25.06.2020 18:21, Max Reitz wrote:
> Places that use patterns like
>
>      if (bs->drv->is_filter && bs->file) {
>          ... something about bs->file->bs ...
>      }
>
> should be
>
>      BlockDriverState *filtered = bdrv_filter_bs(bs);
>      if (filtered) {
>          ... something about @filtered ...
>      }
>
> instead.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block.c                        | 31 ++++++++++++++++++++-----------
>   block/io.c                     |  7 +++++--
>   migration/block-dirty-bitmap.c |  8 +-------
>   3 files changed, 26 insertions(+), 20 deletions(-)
>
...
> diff --git a/block/io.c b/block/io.c
> index df8f2a98d4..385176b331 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>                                     Error **errp)
>   {
>       BlockDriverState *bs = child->bs;
> +    BdrvChild *filtered;
>       BlockDriver *drv = bs->drv;
>       BdrvTrackedRequest req;
>       int64_t old_size, new_bytes;
> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>           goto out;
>       }
>   
> +    filtered = bdrv_filter_child(bs);
> +

Isn't better to have this initialization right before the relevant 
if/else block?

Andrey

>       /*
>        * If the image has a backing file that is large enough that it would
>        * provide data for the new area, we cannot leave it unallocated because
> @@ -3390,8 +3393,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>               goto out;
>           }
>           ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp);
> -    } else if (bs->file && drv->is_filter) {
> -        ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
> +    } else if (filtered) {
> +        ret = bdrv_co_truncate(filtered, offset, exact, prealloc, flags, errp);
>       } else {
>           error_setg(errp, "Image format driver does not support resize");
>           ret = -ENOTSUP;

...

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Max Reitz July 9, 2020, 8:59 a.m. UTC | #2 
On 08.07.20 20:24, Andrey Shinkevich wrote:
> On 25.06.2020 18:21, Max Reitz wrote:
>> Places that use patterns like
>>
>>      if (bs->drv->is_filter && bs->file) {
>>          ... something about bs->file->bs ...
>>      }
>>
>> should be
>>
>>      BlockDriverState *filtered = bdrv_filter_bs(bs);
>>      if (filtered) {
>>          ... something about @filtered ...
>>      }
>>
>> instead.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c                        | 31 ++++++++++++++++++++-----------
>>   block/io.c                     |  7 +++++--
>>   migration/block-dirty-bitmap.c |  8 +-------
>>   3 files changed, 26 insertions(+), 20 deletions(-)
>>
> ...
>> diff --git a/block/io.c b/block/io.c
>> index df8f2a98d4..385176b331 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>                                     Error **errp)
>>   {
>>       BlockDriverState *bs = child->bs;
>> +    BdrvChild *filtered;
>>       BlockDriver *drv = bs->drv;
>>       BdrvTrackedRequest req;
>>       int64_t old_size, new_bytes;
>> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>> *child, int64_t offset, bool exact,
>>           goto out;
>>       }
>>   +    filtered = bdrv_filter_child(bs);
>> +
> 
> Isn't better to have this initialization right before the relevant
> if/else block?

Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
another BdrvChild to be initialized here (@backing), and we need to
initialize that one here.  So I felt it made sense to group them together.

They got split up when I decided to put @filtered into this patch and
@backing into its own.  So now it may look a bit weird, but I feel like
after patch 16 it makes sense.

(I’m indifferent, basically.)

Max
Andrey Shinkevich July 9, 2020, 9:11 a.m. UTC | #3 
On 09.07.2020 11:59, Max Reitz wrote:
> On 08.07.20 20:24, Andrey Shinkevich wrote:
>> On 25.06.2020 18:21, Max Reitz wrote:
>>> Places that use patterns like
>>>
>>>       if (bs->drv->is_filter && bs->file) {
>>>           ... something about bs->file->bs ...
>>>       }
>>>
>>> should be
>>>
>>>       BlockDriverState *filtered = bdrv_filter_bs(bs);
>>>       if (filtered) {
>>>           ... something about @filtered ...
>>>       }
>>>
>>> instead.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>    block.c                        | 31 ++++++++++++++++++++-----------
>>>    block/io.c                     |  7 +++++--
>>>    migration/block-dirty-bitmap.c |  8 +-------
>>>    3 files changed, 26 insertions(+), 20 deletions(-)
>>>
>> ...
>>> diff --git a/block/io.c b/block/io.c
>>> index df8f2a98d4..385176b331 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -3307,6 +3307,7 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>>> *child, int64_t offset, bool exact,
>>>                                      Error **errp)
>>>    {
>>>        BlockDriverState *bs = child->bs;
>>> +    BdrvChild *filtered;
>>>        BlockDriver *drv = bs->drv;
>>>        BdrvTrackedRequest req;
>>>        int64_t old_size, new_bytes;
>>> @@ -3358,6 +3359,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild
>>> *child, int64_t offset, bool exact,
>>>            goto out;
>>>        }
>>>    +    filtered = bdrv_filter_child(bs);
>>> +
>> Isn't better to have this initialization right before the relevant
>> if/else block?
> Hm, well, yes.  In this case, though, maybe not.  Patch 16 will add
> another BdrvChild to be initialized here (@backing), and we need to
> initialize that one here.  So I felt it made sense to group them together.
>
> They got split up when I decided to put @filtered into this patch and
> @backing into its own.  So now it may look a bit weird, but I feel like
> after patch 16 it makes sense.
>
> (I’m indifferent, basically.)
>
> Max

Yes, it makes a sence. I am on the way to reviewing further and have not 
reached the 16th yet.

It is a minor thing anyway )) Thank you for your response.

Andrey
diff mbox series

Patch

diff --git a/block.c b/block.c
index 6449f3a11d..a44af9c3c1 100644
--- a/block.c
+++ b/block.c
@@ -710,11 +710,12 @@  int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
 int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 {
     BlockDriver *drv = bs->drv;
+    BlockDriverState *filtered = bdrv_filter_bs(bs);
 
     if (drv && drv->bdrv_probe_blocksizes) {
         return drv->bdrv_probe_blocksizes(bs, bsz);
-    } else if (drv && drv->is_filter && bs->file) {
-        return bdrv_probe_blocksizes(bs->file->bs, bsz);
+    } else if (filtered) {
+        return bdrv_probe_blocksizes(filtered, bsz);
     }
 
     return -ENOTSUP;
@@ -729,11 +730,12 @@  int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 {
     BlockDriver *drv = bs->drv;
+    BlockDriverState *filtered = bdrv_filter_bs(bs);
 
     if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
-    } else if (drv && drv->is_filter && bs->file) {
-        return bdrv_probe_geometry(bs->file->bs, geo);
+    } else if (filtered) {
+        return bdrv_probe_geometry(filtered, geo);
     }
 
     return -ENOTSUP;
@@ -5421,6 +5423,8 @@  int bdrv_has_zero_init_1(BlockDriverState *bs)
 
 int bdrv_has_zero_init(BlockDriverState *bs)
 {
+    BlockDriverState *filtered;
+
     if (!bs->drv) {
         return 0;
     }
@@ -5433,8 +5437,10 @@  int bdrv_has_zero_init(BlockDriverState *bs)
     if (bs->drv->bdrv_has_zero_init) {
         return bs->drv->bdrv_has_zero_init(bs);
     }
-    if (bs->file && bs->drv->is_filter) {
-        return bdrv_has_zero_init(bs->file->bs);
+
+    filtered = bdrv_filter_bs(bs);
+    if (filtered) {
+        return bdrv_has_zero_init(filtered);
     }
 
     /* safe default */
@@ -5479,8 +5485,9 @@  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
         return -ENOMEDIUM;
     }
     if (!drv->bdrv_get_info) {
-        if (bs->file && drv->is_filter) {
-            return bdrv_get_info(bs->file->bs, bdi);
+        BlockDriverState *filtered = bdrv_filter_bs(bs);
+        if (filtered) {
+            return bdrv_get_info(filtered, bdi);
         }
         return -ENOTSUP;
     }
@@ -6546,6 +6553,8 @@  int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
 bool bdrv_recurse_can_replace(BlockDriverState *bs,
                               BlockDriverState *to_replace)
 {
+    BlockDriverState *filtered;
+
     if (!bs || !bs->drv) {
         return false;
     }
@@ -6560,9 +6569,9 @@  bool bdrv_recurse_can_replace(BlockDriverState *bs,
     }
 
     /* For filters without an own implementation, we can recurse on our own */
-    if (bs->drv->is_filter) {
-        BdrvChild *child = bs->file ?: bs->backing;
-        return bdrv_recurse_can_replace(child->bs, to_replace);
+    filtered = bdrv_filter_bs(bs);
+    if (filtered) {
+        return bdrv_recurse_can_replace(filtered, to_replace);
     }
 
     /* Safe default */
diff --git a/block/io.c b/block/io.c
index df8f2a98d4..385176b331 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3307,6 +3307,7 @@  int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
                                   Error **errp)
 {
     BlockDriverState *bs = child->bs;
+    BdrvChild *filtered;
     BlockDriver *drv = bs->drv;
     BdrvTrackedRequest req;
     int64_t old_size, new_bytes;
@@ -3358,6 +3359,8 @@  int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
         goto out;
     }
 
+    filtered = bdrv_filter_child(bs);
+
     /*
      * If the image has a backing file that is large enough that it would
      * provide data for the new area, we cannot leave it unallocated because
@@ -3390,8 +3393,8 @@  int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
             goto out;
         }
         ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, flags, errp);
-    } else if (bs->file && drv->is_filter) {
-        ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
+    } else if (filtered) {
+        ret = bdrv_co_truncate(filtered, offset, exact, prealloc, flags, errp);
     } else {
         error_setg(errp, "Image format driver does not support resize");
         ret = -ENOTSUP;
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 47bc0f650c..dec656c074 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -356,13 +356,7 @@  static int init_dirty_bitmap_migration(void)
         while (bs && bs->drv && bs->drv->is_filter &&
                !bdrv_has_named_bitmaps(bs))
         {
-            if (bs->backing) {
-                bs = bs->backing->bs;
-            } else if (bs->file) {
-                bs = bs->file->bs;
-            } else {
-                bs = NULL;
-            }
+            bs = bdrv_filter_bs(bs);
         }
 
         if (bs && bs->drv && !bs->drv->is_filter) {