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
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
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
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
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 > >
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 >> >> >
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
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 > >> > >> > >
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 >>>> >>>> >>>
© 2016 - 2024 Red Hat, Inc.