* [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
@ 2017-07-31 12:51 Kevin Wolf
2017-07-31 13:06 ` Eric Blake
2017-07-31 14:38 ` [Qemu-devel] " Jeff Cody
0 siblings, 2 replies; 9+ messages in thread
From: Kevin Wolf @ 2017-07-31 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 12:51 [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity Kevin Wolf
@ 2017-07-31 13:06 ` Eric Blake
2017-07-31 14:00 ` [Qemu-devel] [Qemu-block] " Eric Blake
2017-07-31 14:38 ` [Qemu-devel] " Jeff Cody
1 sibling, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-07-31 13:06 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: peter.maydell, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 13:06 ` Eric Blake
@ 2017-07-31 14:00 ` Eric Blake
2017-07-31 14:07 ` Kevin Wolf
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-07-31 14:00 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: peter.maydell, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 14:00 ` [Qemu-devel] [Qemu-block] " Eric Blake
@ 2017-07-31 14:07 ` Kevin Wolf
0 siblings, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2017-07-31 14:07 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, peter.maydell, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 12:51 [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity Kevin Wolf
2017-07-31 13:06 ` Eric Blake
@ 2017-07-31 14:38 ` Jeff Cody
2017-07-31 14:54 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-07-31 14:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, peter.maydell, qemu-devel
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 14:38 ` [Qemu-devel] " Jeff Cody
@ 2017-07-31 14:54 ` Philippe Mathieu-Daudé
2017-07-31 15:13 ` Kevin Wolf
2017-07-31 15:17 ` Jeff Cody
0 siblings, 2 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-31 14:54 UTC (permalink / raw)
To: Jeff Cody, Kevin Wolf; +Cc: peter.maydell, qemu-devel, qemu-block
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
>>
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 14:54 ` Philippe Mathieu-Daudé
@ 2017-07-31 15:13 ` Kevin Wolf
2017-07-31 15:17 ` Jeff Cody
1 sibling, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2017-07-31 15:13 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Jeff Cody, peter.maydell, qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 14:54 ` Philippe Mathieu-Daudé
2017-07-31 15:13 ` Kevin Wolf
@ 2017-07-31 15:17 ` Jeff Cody
2017-07-31 15:30 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-07-31 15:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, peter.maydell, qemu-devel, qemu-block
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
> >>
> >>
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity
2017-07-31 15:17 ` Jeff Cody
@ 2017-07-31 15:30 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-07-31 15:30 UTC (permalink / raw)
To: Jeff Cody; +Cc: Kevin Wolf, peter.maydell, qemu-devel, qemu-block
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
>>>>
>>>>
>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-31 15:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-31 12:51 [Qemu-devel] [PATCH for-2.10] block/qapi: Remove redundat NULL check to silence Coverity Kevin Wolf
2017-07-31 13:06 ` Eric Blake
2017-07-31 14:00 ` [Qemu-devel] [Qemu-block] " Eric Blake
2017-07-31 14:07 ` Kevin Wolf
2017-07-31 14:38 ` [Qemu-devel] " Jeff Cody
2017-07-31 14:54 ` Philippe Mathieu-Daudé
2017-07-31 15:13 ` Kevin Wolf
2017-07-31 15:17 ` Jeff Cody
2017-07-31 15:30 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.