[libvirt] [PATCH 0/2] Yet another round of build fixes
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
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@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
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@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
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
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
participants (2)
-
Michal Privoznik -
Peter Krempa