6:44 a.m.
MinGW complained that we might be dereferencing a NULL pointer. While
that's most probably not going to be true (now), the logic certainly
allows for that and we might actually do this a lot in the future with
sparse vcpu mapping.
../../src/conf/domain_conf.c: In function 'virDomainDefGetVcpuPinInfoHelper':
../../src/conf/domain_conf.c:1545:17: error: potential null pointer
dereference [-Werror=null-dereference]
if (vcpu->cpumask)
~~~~^~~~~~~~~
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
I could've pushed this as a build breaker, but I'm not really sure
everyone will like this to be handled this way. I also did another
fix for this where we don't do int->size_t->int casting all the time,
but it's probably not worth the hassle. Also I don't know whether
Peter has more stuff for this in his pockets now, so I figured I
rather submit this for review.
src/conf/domain_conf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 16e0736e09db..308073897880 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1542,6 +1542,9 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i);
virBitmapPtr bitmap = NULL;
+ if (!vcpu)
+ continue;
+
if (vcpu->cpumask)
bitmap = vcpu->cpumask;
else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
--
2.9.0
8:01 a.m.
On Tue, Jul 12, 2016 at 13:44:10 +0200, Martin Kletzander wrote:
MinGW complained that we might be dereferencing a NULL pointer.
While
^^ Does anybody use that?
that's most probably not going to be true (now), the logic
certainly
allows for that and we might actually do this a lot in the future with
sparse vcpu mapping.
../../src/conf/domain_conf.c: In function 'virDomainDefGetVcpuPinInfoHelper':
../../src/conf/domain_conf.c:1545:17: error: potential null pointer
dereference [-Werror=null-dereference]
if (vcpu->cpumask)
~~~~^~~~~~~~~
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
I could've pushed this as a build breaker, but I'm not really sure
everyone will like this to be handled this way. I also did another
fix for this where we don't do int->size_t->int casting all the time,
but it's probably not worth the hassle. Also I don't know whether
Peter has more stuff for this in his pockets now, so I figured I
rather submit this for review.
It's impossible since virDomainDefGetVcpu returns a valid pointer if i
is less than def->maxvcpus.
Since we are rather adding dead code than not supporting broken
toolchain ... ACK.
src/conf/domain_conf.c | 3 +++
1 file changed, 3 insertions(+)
Peter
10:54 a.m.
On Tue, Jul 12, 2016 at 03:01:57PM +0200, Peter Krempa wrote:
On Tue, Jul 12, 2016 at 13:44:10 +0200, Martin Kletzander wrote:
> MinGW complained that we might be dereferencing a NULL pointer. While
^^ Does anybody use that?
Well, if nobody uses that, feel free to send a patch removing all mingw
functionality then.
> that's most probably not going to be true (now), the logic
certainly
> allows for that and we might actually do this a lot in the future with
> sparse vcpu mapping.
>
> ../../src/conf/domain_conf.c: In function
'virDomainDefGetVcpuPinInfoHelper':
> ../../src/conf/domain_conf.c:1545:17: error: potential null pointer
> dereference [-Werror=null-dereference]
> if (vcpu->cpumask)
> ~~~~^~~~~~~~~
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> I could've pushed this as a build breaker, but I'm not really sure
> everyone will like this to be handled this way. I also did another
> fix for this where we don't do int->size_t->int casting all the time,
> but it's probably not worth the hassle. Also I don't know whether
> Peter has more stuff for this in his pockets now, so I figured I
> rather submit this for review.
It's impossible since virDomainDefGetVcpu returns a valid pointer if i
is less than def->maxvcpus.
It is, but only thanks to how we work with the structure. Since
def->vcpus is not virDomainVcpuDefPtr but rather _array_ of
virDomainVcpuDefPtr, then it *can* be NULL very easily. I know we don't
deal with checking member[x] when x < nmembers, but OTOH it's not that
often array of pointers. Also we do this very similar thing somewhere
else, and the compiler doesn't complain, so we can change to that or
just leave it as it is.
Just so you know, my reasoning for this was that sparse vcpus could
cause the array to be not mapped fully. I see now that every one of
those pointers are mapped even when it's not needed, so I guess that's
invalid. No idea why we have that as an array of pointers then.
Since we are rather adding dead code than not supporting broken
toolchain ... ACK.
The toolchain is actually not broken. The assumption from the syntactic
POV is valid. If we thought it is broken we'd probably use
-Wno-null-dereference or similar.
If two lines of dead code (that are clearly readable) are that annoying
to you (more than size_t->int->size_t hindering readability in that
function), I suggest raising that concern in a separate discussion on
the ML so that we have a consensus about dealing with MinGW, clang,
coverity, gcc > 4.9 and other scenarios that cause this. Way less
people will read it when it's just part of a review for three-line
build-breaker (from my experience).
I, for one, don't really care about this that much, so I won't push
this. Let's see how we resolve this once and for all (unless someone
pushes another patch under the build-breaker rule).
>
> src/conf/domain_conf.c | 3 +++
> 1 file changed, 3 insertions(+)
Peter
11:24 a.m.
On Tue, Jul 12, 2016 at 17:54:03 +0200, Martin Kletzander wrote:
On Tue, Jul 12, 2016 at 03:01:57PM +0200, Peter Krempa wrote:
>On Tue, Jul 12, 2016 at 13:44:10 +0200, Martin Kletzander wrote:
[...]
>> I could've pushed this as a build breaker, but I'm
not really sure
>> everyone will like this to be handled this way. I also did another
>> fix for this where we don't do int->size_t->int casting all the time,
>> but it's probably not worth the hassle. Also I don't know whether
>> Peter has more stuff for this in his pockets now, so I figured I
>> rather submit this for review.
>
>It's impossible since virDomainDefGetVcpu returns a valid pointer if i
>is less than def->maxvcpus.
>
It is, but only thanks to how we work with the structure. Since
def->vcpus is not virDomainVcpuDefPtr but rather _array_ of
virDomainVcpuDefPtr, then it *can* be NULL very easily. I know we don't
It can't. Just the compiler thinks it can. We allocate it densely.
deal with checking member[x] when x < nmembers, but OTOH it's
not that
often array of pointers. Also we do this very similar thing somewhere
else, and the compiler doesn't complain, so we can change to that or
just leave it as it is.
Just so you know, my reasoning for this was that sparse vcpus could
cause the array to be not mapped fully. I see now that every one of
those pointers are mapped even when it's not needed, so I guess that's
invalid. No idea why we have that as an array of pointers then.
Since I've added private data every single member of the array needs to
be initialized separately. I've opted for allocating them separately to
go with the majority of our functions. I still plan for that to remain
densely allocated.
>Since we are rather adding dead code than not supporting broken
>toolchain ... ACK.
>
The toolchain is actually not broken. The assumption from the syntactic
POV is valid. If we thought it is broken we'd probably use
-Wno-null-dereference or similar.
We've already added some dead code because of that check.
If two lines of dead code (that are clearly readable) are that
annoying
It might hint to readers that the array is indeed sparse. Reading the
code again I now think that rather than skipping the loop the check for
vcpu should be moved into the if right below it:
if (vcpu && vcpu->cpumask)
bitmap = vcpu->cpumask;
By skipping the loop you don't fill anything which is semanticaly wrong
in context of that function.
to you (more than size_t->int->size_t hindering readability in
that
Erm, yes, that was bugging me and I'm not sure why I didn't get rid of
that when I was refactoring it.
function), I suggest raising that concern in a separate discussion
on
the ML so that we have a consensus about dealing with MinGW, clang,
coverity, gcc > 4.9 and other scenarios that cause this. Way less
people will read it when it's just part of a review for three-line
build-breaker (from my experience).
I, for one, don't really care about this that much, so I won't push
this. Let's see how we resolve this once and for all (unless someone
pushes another patch under the build-breaker rule).
Or you could push this righ away since I clearly ACKed it.
12:21 p.m.
On Tue, Jul 12, 2016 at 06:24:01PM +0200, Peter Krempa wrote:
On Tue, Jul 12, 2016 at 17:54:03 +0200, Martin Kletzander wrote:
> On Tue, Jul 12, 2016 at 03:01:57PM +0200, Peter Krempa wrote:
> >On Tue, Jul 12, 2016 at 13:44:10 +0200, Martin Kletzander wrote:
[...]
> >> I could've pushed this as a build breaker, but I'm not really sure
> >> everyone will like this to be handled this way. I also did another
> >> fix for this where we don't do int->size_t->int casting all the
time,
> >> but it's probably not worth the hassle. Also I don't know whether
> >> Peter has more stuff for this in his pockets now, so I figured I
> >> rather submit this for review.
> >
> >It's impossible since virDomainDefGetVcpu returns a valid pointer if i
> >is less than def->maxvcpus.
> >
>
> It is, but only thanks to how we work with the structure. Since
> def->vcpus is not virDomainVcpuDefPtr but rather _array_ of
> virDomainVcpuDefPtr, then it *can* be NULL very easily. I know we don't
It can't. Just the compiler thinks it can. We allocate it densely.
> deal with checking member[x] when x < nmembers, but OTOH it's not that
> often array of pointers. Also we do this very similar thing somewhere
> else, and the compiler doesn't complain, so we can change to that or
> just leave it as it is.
>
> Just so you know, my reasoning for this was that sparse vcpus could
> cause the array to be not mapped fully. I see now that every one of
> those pointers are mapped even when it's not needed, so I guess that's
> invalid. No idea why we have that as an array of pointers then.
Since I've added private data every single member of the array needs to
be initialized separately. I've opted for allocating them separately to
go with the majority of our functions. I still plan for that to remain
densely allocated.
> >Since we are rather adding dead code than not supporting broken
> >toolchain ... ACK.
> >
>
> The toolchain is actually not broken. The assumption from the syntactic
> POV is valid. If we thought it is broken we'd probably use
> -Wno-null-dereference or similar.
We've already added some dead code because of that check.
> If two lines of dead code (that are clearly readable) are that annoying
It might hint to readers that the array is indeed sparse. Reading the
code again I now think that rather than skipping the loop the check for
vcpu should be moved into the if right below it:
if (vcpu && vcpu->cpumask)
bitmap = vcpu->cpumask;
By skipping the loop you don't fill anything which is semanticaly wrong
in context of that function.
> to you (more than size_t->int->size_t hindering readability in that
Erm, yes, that was bugging me and I'm not sure why I didn't get rid of
that when I was refactoring it.
> function), I suggest raising that concern in a separate discussion on
> the ML so that we have a consensus about dealing with MinGW, clang,
> coverity, gcc > 4.9 and other scenarios that cause this. Way less
> people will read it when it's just part of a review for three-line
> build-breaker (from my experience).
>
> I, for one, don't really care about this that much, so I won't push
> this. Let's see how we resolve this once and for all (unless someone
> pushes another patch under the build-breaker rule).
Or you could push this righ away since I clearly ACKed it.
I already had yet another fix in mind that doesn't add any dead code,
doesn't hint users anything and takes care of the error. Plus it uses
the same construct that already exists somewhere else in the code.
So would you like this one better?
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 16e0736e09db..c1f98d8852e6 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -1539,7 +1539,7 @@ virDomainDefGetVcpuPinInfoHelper(virDomainDefPtr def,
virBitmapSetAll(allcpumap);
for (i = 0; i < maxvcpus && i < ncpumaps; i++) {
- virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i);
+ virDomainVcpuDefPtr vcpu = def->vcpus[i];
virBitmapPtr bitmap = NULL;
if (vcpu->cpumask)
--
Martin
7:26 a.m.
On Tue, Jul 12, 2016 at 06:24:01PM +0200, Peter Krempa wrote:
[...]
It might hint to readers that the array is indeed sparse. Reading the
code again I now think that rather than skipping the loop the check for
vcpu should be moved into the if right below it:
if (vcpu && vcpu->cpumask)
bitmap = vcpu->cpumask;
By skipping the loop you don't fill anything which is semanticaly wrong
in context of that function.
I pushed this version after our discussion about this. I can start
watching the CI mails again ;)
Martin
3058
days inactive
3059
days old
5 comments
2 participants
participants (2)
-
Martin Kletzander -
Peter Krempa