[Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Kevin Wolf]

When skipping implicit nodes in bdrv_block_device_info(), we know that
bs0 is always non-NULL; initially, because it's taken from a BdrvChild
and a BdrvChild never has a NULL bs, and after the first iteration
because implicit nodes always have a backing file.

Remove the NULL check and add an assertion that the implicit node does
indeed have a backing file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index d2b18ee9df..5f1a71f5d2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
         /* Skip automatically inserted nodes that the user isn't aware of for
          * query-block (blk != NULL), but not for query-named-block-nodes */
-        while (blk && bs0 && bs0->drv && bs0->implicit) {
+        while (blk && bs0->drv && bs0->implicit) {
             bs0 = backing_bs(bs0);
+            assert(bs0);
         }
     }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

On 07/31/2017 07:51 AM, Kevin Wolf wrote:
> When skipping implicit nodes in bdrv_block_device_info(), we know that
> bs0 is always non-NULL; initially, because it's taken from a BdrvChild
> and a BdrvChild never has a NULL bs, and after the first iteration
> because implicit nodes always have a backing file.
> 
> Remove the NULL check and add an assertion that the implicit node does
> indeed have a backing file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index d2b18ee9df..5f1a71f5d2 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>  
>          /* Skip automatically inserted nodes that the user isn't aware of for
>           * query-block (blk != NULL), but not for query-named-block-nodes */
> -        while (blk && bs0 && bs0->drv && bs0->implicit) {
> +        while (blk && bs0->drv && bs0->implicit) {
>              bs0 = backing_bs(bs0);
> +            assert(bs0);
>          }
>      }
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

On 07/31/2017 08:06 AM, Eric Blake wrote:
> On 07/31/2017 07:51 AM, Kevin Wolf wrote:

In the subject line, s/redundat/redundant/

>> When skipping implicit nodes in bdrv_block_device_info(), we know that
>> bs0 is always non-NULL; initially, because it's taken from a BdrvChild
>> and a BdrvChild never has a NULL bs, and after the first iteration
>> because implicit nodes always have a backing file.
>>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Am 31.07.2017 um 16:00 hat Eric Blake geschrieben:
> On 07/31/2017 08:06 AM, Eric Blake wrote:
> > On 07/31/2017 07:51 AM, Kevin Wolf wrote:
> 
> In the subject line, s/redundat/redundant/

Thanks, I'll fix this.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Jeff Cody]

On Mon, Jul 31, 2017 at 02:51:11PM +0200, Kevin Wolf wrote:
> When skipping implicit nodes in bdrv_block_device_info(), we know that
> bs0 is always non-NULL; initially, because it's taken from a BdrvChild

Not to mention, we deference bs0 in the chunk of code right above this, so
we'd segfault anyway if the initial value was NULL.

> and a BdrvChild never has a NULL bs, and after the first iteration
> because implicit nodes always have a backing file.
> 
> Remove the NULL check and add an assertion that the implicit node does
> indeed have a backing file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>


Reviewed-by: Jeff Cody <jcody@redhat.com>



> ---
>  block/qapi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index d2b18ee9df..5f1a71f5d2 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>  
>          /* Skip automatically inserted nodes that the user isn't aware of for
>           * query-block (blk != NULL), but not for query-named-block-nodes */
> -        while (blk && bs0 && bs0->drv && bs0->implicit) {
> +        while (blk && bs0->drv && bs0->implicit) {
>              bs0 = backing_bs(bs0);
> +            assert(bs0);
>          }
>      }
>  
> -- 
> 2.13.3
> 
> 

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Philippe Mathieu-Daudé]

On 07/31/2017 11:38 AM, Jeff Cody wrote:
> On Mon, Jul 31, 2017 at 02:51:11PM +0200, Kevin Wolf wrote:
>> When skipping implicit nodes in bdrv_block_device_info(), we know that
>> bs0 is always non-NULL; initially, because it's taken from a BdrvChild
> 
> Not to mention, we deference bs0 in the chunk of code right above this, so
> we'd segfault anyway if the initial value was NULL.

Yes, please move your assert before:

137:    if (bs0->drv && bs0->backing) {

Once there:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
>> and a BdrvChild never has a NULL bs, and after the first iteration
>> because implicit nodes always have a backing file.
>>
>> Remove the NULL check and add an assertion that the implicit node does
>> indeed have a backing file.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> 
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> 
> 
> 
>> ---
>>   block/qapi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index d2b18ee9df..5f1a71f5d2 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>   
>>           /* Skip automatically inserted nodes that the user isn't aware of for
>>            * query-block (blk != NULL), but not for query-named-block-nodes */
>> -        while (blk && bs0 && bs0->drv && bs0->implicit) {
>> +        while (blk && bs0->drv && bs0->implicit) {
>>               bs0 = backing_bs(bs0);
>> +            assert(bs0);
>>           }
>>       }
>>   
>> -- 
>> 2.13.3
>>
>>
> 

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Kevin Wolf]

Am 31.07.2017 um 16:54 hat Philippe Mathieu-Daudé geschrieben:
> On 07/31/2017 11:38 AM, Jeff Cody wrote:
> > On Mon, Jul 31, 2017 at 02:51:11PM +0200, Kevin Wolf wrote:
> > > When skipping implicit nodes in bdrv_block_device_info(), we know that
> > > bs0 is always non-NULL; initially, because it's taken from a BdrvChild
> > 
> > Not to mention, we deference bs0 in the chunk of code right above this, so
> > we'd segfault anyway if the initial value was NULL.

Not really. The last use of bs0 before the loop is:

    bs0 = bs0->backing->bs;bs0 = bs0->backing->bs;

So we're pointing to a different BDS now.

> Yes, please move your assert before:
> 
> 137:    if (bs0->drv && bs0->backing) {

That would assert something completely different and much more obvious.
(And apart from that, bdrv_query_image_info() in line 130 already
dereferences bs0, so it would be too late, too.)

What I want to assert here is that every implicit image has a backing
file.

Kevin

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Jeff Cody]

On Mon, Jul 31, 2017 at 11:54:57AM -0300, Philippe Mathieu-Daudé wrote:
> On 07/31/2017 11:38 AM, Jeff Cody wrote:
> >On Mon, Jul 31, 2017 at 02:51:11PM +0200, Kevin Wolf wrote:
> >>When skipping implicit nodes in bdrv_block_device_info(), we know that
> >>bs0 is always non-NULL; initially, because it's taken from a BdrvChild
> >
> >Not to mention, we deference bs0 in the chunk of code right above this, so
> >we'd segfault anyway if the initial value was NULL.
> 
> Yes, please move your assert before:
> 
> 137:    if (bs0->drv && bs0->backing) {
> 
> Once there:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>

I don't think moving the assert() is the correct thing to do, as it is
useful when iterating in the while() via backing_bs().

But maybe adding some asserts would be useful, if we are really concerned.
Looking at the code:

135         }
136

Maybe add an assert(bs0) here, as you suggest...

137         if (bs0->drv && bs0->backing) {
138             info->backing_file_depth++;
139             bs0 = bs0->backing->bs;

But then maybe we want one here, too?  Or maybe that is overkill :)

140             (*p_image_info)->has_backing_image = true;
141             p_image_info = &((*p_image_info)->backing_image);
142         } else {
143             break;
144         }
145
146         /* Skip automatically inserted nodes that the user isn't aware of for
147          * query-block (blk != NULL), but not for query-named-block-nodes */
148         while (blk && bs0 && bs0->drv && bs0->implicit) {
149             bs0 = backing_bs(bs0);
150         }



> >
> >>and a BdrvChild never has a NULL bs, and after the first iteration
> >>because implicit nodes always have a backing file.
> >>
> >>Remove the NULL check and add an assertion that the implicit node does
> >>indeed have a backing file.
> >>
> >>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >
> >
> >Reviewed-by: Jeff Cody <jcody@redhat.com>
> >
> >
> >
> >>---
> >>  block/qapi.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/block/qapi.c b/block/qapi.c
> >>index d2b18ee9df..5f1a71f5d2 100644
> >>--- a/block/qapi.c
> >>+++ b/block/qapi.c
> >>@@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >>          /* Skip automatically inserted nodes that the user isn't aware of for
> >>           * query-block (blk != NULL), but not for query-named-block-nodes */
> >>-        while (blk && bs0 && bs0->drv && bs0->implicit) {
> >>+        while (blk && bs0->drv && bs0->implicit) {
> >>              bs0 = backing_bs(bs0);
> >>+            assert(bs0);
> >>          }
> >>      }
> >>-- 
> >>2.13.3
> >>
> >>
> >

Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Philippe Mathieu-Daudé]

On 07/31/2017 12:17 PM, Jeff Cody wrote:
> On Mon, Jul 31, 2017 at 11:54:57AM -0300, Philippe Mathieu-Daudé wrote:
>> On 07/31/2017 11:38 AM, Jeff Cody wrote:
>>> On Mon, Jul 31, 2017 at 02:51:11PM +0200, Kevin Wolf wrote:
>>>> When skipping implicit nodes in bdrv_block_device_info(), we know that
>>>> bs0 is always non-NULL; initially, because it's taken from a BdrvChild
>>>
>>> Not to mention, we deference bs0 in the chunk of code right above this, so
>>> we'd segfault anyway if the initial value was NULL.
>>
>> Yes, please move your assert before:
>>
>> 137:    if (bs0->drv && bs0->backing) {
>>
>> Once there:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
> 
> I don't think moving the assert() is the correct thing to do, as it is
> useful when iterating in the while() via backing_bs().
> 
> But maybe adding some asserts would be useful, if we are really concerned.
> Looking at the code:
> 
> 135         }
> 136
> 
> Maybe add an assert(bs0) here, as you suggest...
> 
> 137         if (bs0->drv && bs0->backing) {
> 138             info->backing_file_depth++;
> 139             bs0 = bs0->backing->bs;
> 
> But then maybe we want one here, too?  Or maybe that is overkill :)
> 
> 140             (*p_image_info)->has_backing_image = true;
> 141             p_image_info = &((*p_image_info)->backing_image);
> 142         } else {
> 143             break;
> 144         }
> 145
> 146         /* Skip automatically inserted nodes that the user isn't aware of for
> 147          * query-block (blk != NULL), but not for query-named-block-nodes */
> 148         while (blk && bs0 && bs0->drv && bs0->implicit) {
> 149             bs0 = backing_bs(bs0);
> 150         }

I first thought of inverting the if():

         if (!(bs0->drv && bs0->backing)) {
             break;
         }
         info->backing_file_depth++;
         bs0 = bs0->backing->bs;
         assert(bs0);
         (*p_image_info)->has_backing_image = true;
         p_image_info = &((*p_image_info)->backing_image);

then read your mail and Kevin's one and realized if 3 ppl disagree 
commenting the same piece of code that fast, it means this code is not 
simple enough and surely need refactoring. Now it is not just about 
silencing Coverity :)

>>>
>>>> and a BdrvChild never has a NULL bs, and after the first iteration
>>>> because implicit nodes always have a backing file.
>>>>
>>>> Remove the NULL check and add an assertion that the implicit node does
>>>> indeed have a backing file.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>
>>>
>>> Reviewed-by: Jeff Cody <jcody@redhat.com>
>>>
>>>
>>>
>>>> ---
>>>>   block/qapi.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>> index d2b18ee9df..5f1a71f5d2 100644
>>>> --- a/block/qapi.c
>>>> +++ b/block/qapi.c
>>>> @@ -145,8 +145,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>           /* Skip automatically inserted nodes that the user isn't aware of for
>>>>            * query-block (blk != NULL), but not for query-named-block-nodes */
>>>> -        while (blk && bs0 && bs0->drv && bs0->implicit) {
>>>> +        while (blk && bs0->drv && bs0->implicit) {
>>>>               bs0 = backing_bs(bs0);
>>>> +            assert(bs0);
>>>>           }
>>>>       }
>>>> -- 
>>>> 2.13.3
>>>>
>>>>
>>>