Re: [Qemu-block] [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é 



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 



Reviewed-by: Jeff Cody 




---
  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-block] [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é 
>

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 
> >
> >
> >Reviewed-by: Jeff Cody 
> >
> >
> >
> >>---
> >>  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-block] [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-block] [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é 




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 



Reviewed-by: Jeff Cody 




---
  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-block] [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 


Reviewed-by: Jeff Cody 



> ---
>  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-block] [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity

[Kevin Wolf]

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


signature.asc
Description: PGP signature

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

[Eric Blake]

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



signature.asc
Description: OpenPGP digital signature

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

[Eric Blake]

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 
> ---
>  block/qapi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

> 
> 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



signature.asc
Description: OpenPGP digital signature