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é <f4...@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 <kw...@redhat.com>
Reviewed-by: Jeff Cody <jc...@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
