8:53 a.m.
After these regular errors, I still cannot build cleanly:
../../src/conf/domain_conf.c: In function 'virDomainChrPreAlloc':
../../src/conf/domain_conf.c:14109:57: error: potential null pointer dereference
[-Werror=null-dereference]
return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
^
../../src/conf/domain_conf.c: In function 'virDomainChrRemove':
../../src/conf/domain_conf.c:14133:21: error: potential null pointer dereference
[-Werror=null-dereference]
for (i = 0; i < *cntPtr; i++) {
^~~~~~~
cc1: all warnings being treated as errors
These are obviously false positives. I know some developers here
are not very fond of fixing those, so my question is - is it
worth the effort or will my patches be NACKed straight away?
Michal Privoznik (2):
apibuild: Substitute only pure number tokens
virSocketAddrIsPrivate: Work on 32bits platforms
docs/apibuild.py | 2 +-
src/util/virsocketaddr.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
--
2.8.3
8:53 a.m.
New subject: [libvirt] [PATCH 1/2] apibuild: Substitute only pure number tokens
In 38df47c9af1 I've tried to prepare our apibuild.py script for
change made in 0628f3498ce (1U << 31). What I've done in the
former commit was to replace \d+U in parsed tokens with \d.
Problem was, my regular expression there was not quite right as
it also translated VIR_123U_VAL into VIR_123_VAL.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/apibuild.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/apibuild.py b/docs/apibuild.py
index 712b8b9..f5216ea 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -1399,7 +1399,7 @@ class CParser:
while token[0] != "sep" or (token[1] != ',' and
token[1] != '}'):
# We might be dealing with '1U << 12' here
- value = value + re.sub("(\d+)U","\\1",
token[1])
+ value = value + re.sub("^(\d+)U$","\\1",
token[1])
token = self.token()
else:
try:
--
2.8.3
8:53 a.m.
New subject: [libvirt] [PATCH 2/2] virSocketAddrIsPrivate: Work on 32bits platforms
Yet another one of those where signed int (or long int) is not
enough. And useless to as we're aiming at unsigned anyway.
../../src/util/virsocketaddr.c: In function 'virSocketAddrIsPrivate':
../../src/util/virsocketaddr.c:289:45: error: result of '192l << 24'
requires 33 bits to represent, but 'long int' only has 32 bits
[-Werror=shift-overflow=]
return ((val & 0xFFFF0000) == ((192L << 24) + (168 << 16)) ||
^~
../../src/util/virsocketaddr.c:290:45: error: result of '172l << 24'
requires 33 bits to represent, but 'long int' only has 32 bits
[-Werror=shift-overflow=]
(val & 0xFFF00000) == ((172L << 24) + (16 << 16)) ||
^~
cc1: all warnings being treated as errors
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virsocketaddr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index a0c92ea..98cb4ca 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -286,9 +286,9 @@ virSocketAddrIsPrivate(const virSocketAddr *addr)
case AF_INET:
val = ntohl(addr->data.inet4.sin_addr.s_addr);
- return ((val & 0xFFFF0000) == ((192L << 24) + (168 << 16)) ||
- (val & 0xFFF00000) == ((172L << 24) + (16 << 16)) ||
- (val & 0xFF000000) == ((10L << 24)));
+ return ((val & 0xFFFF0000) == ((192UL << 24) + (168 << 16)) ||
+ (val & 0xFFF00000) == ((172UL << 24) + (16 << 16)) ||
+ (val & 0xFF000000) == ((10UL << 24)));
case AF_INET6:
return ((addr->data.inet6.sin6_addr.s6_addr[0] & 0xFE) == 0xFC ||
--
2.8.3
9:56 a.m.
On Mon, May 30, 2016 at 15:53:08 +0200, Michal Privoznik wrote:
After these regular errors, I still cannot build cleanly:
../../src/conf/domain_conf.c: In function 'virDomainChrPreAlloc':
../../src/conf/domain_conf.c:14109:57: error: potential null pointer dereference
[-Werror=null-dereference]
return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
^
../../src/conf/domain_conf.c: In function 'virDomainChrRemove':
../../src/conf/domain_conf.c:14133:21: error: potential null pointer dereference
[-Werror=null-dereference]
for (i = 0; i < *cntPtr; i++) {
^~~~~~~
cc1: all warnings being treated as errors
These are obviously false positives. I know some developers here
are not very fond of fixing those, so my question is - is it
worth the effort or will my patches be NACKed straight away?
Looking at the code I'm not surprised that the compiler thinks that it's
a null-deref. The only reason why it isn't is that we don't ever pass
the VIR_DOMAIN_CHR_DEVICE_TYPE_LAST as device type and that's very hard
to infer.
An option would be to remove the monstrosity called
virDomainChrGetDomainPtrsInternal and friends and mabye produce a saner
code that will be easier to introspect both for humans and compilers.
Michal Privoznik (2):
apibuild: Substitute only pure number tokens
virSocketAddrIsPrivate: Work on 32bits platforms
ACK to those two. This time I verified that I've applied the patch prior
to trying to break it.
Peter
10:45 a.m.
On 30.05.2016 16:56, Peter Krempa wrote:
On Mon, May 30, 2016 at 15:53:08 +0200, Michal Privoznik wrote:
> After these regular errors, I still cannot build cleanly:
>
> ../../src/conf/domain_conf.c: In function 'virDomainChrPreAlloc':
> ../../src/conf/domain_conf.c:14109:57: error: potential null pointer dereference
[-Werror=null-dereference]
> return VIR_REALLOC_N(*arrPtr, *cntPtr + 1);
> ^
> ../../src/conf/domain_conf.c: In function 'virDomainChrRemove':
> ../../src/conf/domain_conf.c:14133:21: error: potential null pointer dereference
[-Werror=null-dereference]
> for (i = 0; i < *cntPtr; i++) {
> ^~~~~~~
> cc1: all warnings being treated as errors
>
> These are obviously false positives. I know some developers here
> are not very fond of fixing those, so my question is - is it
> worth the effort or will my patches be NACKed straight away?
Looking at the code I'm not surprised that the compiler thinks that it's
a null-deref. The only reason why it isn't is that we don't ever pass
the VIR_DOMAIN_CHR_DEVICE_TYPE_LAST as device type and that's very hard
to infer.
An option would be to remove the monstrosity called
virDomainChrGetDomainPtrsInternal and friends and mabye produce a saner
code that will be easier to introspect both for humans and compilers.
I guess that's matter of taste. I find the current code compact and easy
to read. What would be easier is if we dropped def->consoles,
def->serials, def->parallels ... in favour of one array, say def->chr
where all of those are merged together. But that's a bigger portion of
work, isn't it.
>
> Michal Privoznik (2):
> apibuild: Substitute only pure number tokens
> virSocketAddrIsPrivate: Work on 32bits platforms
ACK to those two. This time I verified that I've applied the patch prior
to trying to break it.
Thanks, pushed.
Michal
3098
days inactive
3098
days old
4 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa