[PATCH v2 4/5] block: simplify bdrv_child_user_desc()

Vladimir Sementsov-Ogievskiy posted 5 patches 4 years, 9 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
[PATCH v2 4/5] block: simplify bdrv_child_user_desc()
Posted by Vladimir Sementsov-Ogievskiy 4 years, 9 months ago
All existing parent types (block nodes, block devices, jobs) has the
realization. So, drop unreachable code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 54a3da9311..2f73523285 100644
--- a/block.c
+++ b/block.c
@@ -2029,11 +2029,7 @@ bool bdrv_is_writable(BlockDriverState *bs)
 
 static char *bdrv_child_user_desc(BdrvChild *c)
 {
-    if (c->klass->get_parent_desc) {
-        return c->klass->get_parent_desc(c);
-    }
-
-    return g_strdup("another user");
+    return c->klass->get_parent_desc(c);
 }
 
 static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)
-- 
2.29.2


Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()
Posted by Kevin Wolf 4 years, 8 months ago
Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Your fixes the other days showed that vvfat has a BdrvChildClass, too.
This instance doesn't define .get_parent_desc(), so the code that this
patch removes is potentially still reachable.

Kevin


Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago
31.05.2021 18:50, Kevin Wolf wrote:
> Am 04.05.2021 um 11:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> All existing parent types (block nodes, block devices, jobs) has the
>> realization. So, drop unreachable code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Your fixes the other days showed that vvfat has a BdrvChildClass, too.
> This instance doesn't define .get_parent_desc(), so the code that this
> patch removes is potentially still reachable.
> 

Right. Will fix it and add an assertion to bdrv_attach_child_common()


-- 
Best regards,
Vladimir

Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()
Posted by Alberto Garcia 4 years, 9 months ago
On Tue 04 May 2021 11:45:09 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> All existing parent types (block nodes, block devices, jobs) has the
> realization. So, drop unreachable code.

s/has/have/ , and I'm not sure what "have the realization" means

>  static char *bdrv_child_user_desc(BdrvChild *c)
>  {
> -    if (c->klass->get_parent_desc) {
> -        return c->klass->get_parent_desc(c);
> -    }
> -
> -    return g_strdup("another user");
> +    return c->klass->get_parent_desc(c);
>  }

Should we also assert(c->klass->get_parent_desc) ?

Berto

Re: [PATCH v2 4/5] block: simplify bdrv_child_user_desc()
Posted by Vladimir Sementsov-Ogievskiy 4 years, 9 months ago
10.05.2021 18:33, Alberto Garcia wrote:
> On Tue 04 May 2021 11:45:09 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> All existing parent types (block nodes, block devices, jobs) has the
>> realization. So, drop unreachable code.
> 
> s/has/have/ , and I'm not sure what "have the realization" means
> 
>>   static char *bdrv_child_user_desc(BdrvChild *c)
>>   {
>> -    if (c->klass->get_parent_desc) {
>> -        return c->klass->get_parent_desc(c);
>> -    }
>> -
>> -    return g_strdup("another user");
>> +    return c->klass->get_parent_desc(c);
>>   }
> 
> Should we also assert(c->klass->get_parent_desc) ?
> 

No, as crash on calling zero pointer is practically not worse than crash on failed assertion.


-- 
Best regards,
Vladimir