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

[Jeff Cody <jcody@redhat.com>]

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