[libvirt] [PATCH 0/2] build: fix bugs detected by newer gcc
Rawhide's gcc 4.5.0 got smarter in relation to Fedora 13's gcc 4.4.4, and exposed a real bug (introduced by commit ed9c14a7 this January) as well as extra noise that is easy enough to silence. The new warnings looked like: esx/esx_vi_types.c:1231:1: warning: logical 'or' of collectively exhaustive tests is always true [-Wlogical-op] And the esx_vi_types.c case is proof why it is a bad idea to ever stick -Werror in a spec file - upgrade the compiler, and something that previously built silently can now fail to build; in contrast, using -Werror during development is great as it forces you to think about the issue, and in the case of xen, fix a real bug. Eric Blake (2): xen: fix logic bug esx: silence spurious compiler warning src/esx/esx_vi_types.c | 6 ++++-- src/xen/xend_internal.c | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) -- 1.7.2
The recent switch to enable -Wlogical-op paid off again. gcc 4.5.0 (rawhide) is smarter than 4.4.4 (Fedora 13). * src/xen/xend_internal.c (xenDaemonAttachDeviceFlags) (xenDaemonUpdateDeviceFlags, xenDaemonDetachDeviceFlags): Use correct operator. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xen/xend_internal.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index fad5ce8..311775a 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3888,7 +3888,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying " @@ -4027,7 +4027,7 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying " @@ -4138,7 +4138,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying " -- 1.7.2
On Wed, Jul 28, 2010 at 05:34:14PM -0600, Eric Blake wrote:
The recent switch to enable -Wlogical-op paid off again. gcc 4.5.0 (rawhide) is smarter than 4.4.4 (Fedora 13).
* src/xen/xend_internal.c (xenDaemonAttachDeviceFlags) (xenDaemonUpdateDeviceFlags, xenDaemonDetachDeviceFlags): Use correct operator.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xen/xend_internal.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index fad5ce8..311775a 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3888,7 +3888,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying " @@ -4027,7 +4027,7 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying " @@ -4138,7 +4138,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, } else { /* Only live config can be changed if xendConfigVersion < 3 */ if (priv->xendConfigVersion < 3 && - (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT || + (flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT && flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) { virXendError(VIR_ERR_OPERATION_INVALID, "%s", _("Xend version does not support modifying "
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 07/29/2010 11:17 AM, Daniel Veillard wrote:
On Wed, Jul 28, 2010 at 05:34:14PM -0600, Eric Blake wrote:
The recent switch to enable -Wlogical-op paid off again. gcc 4.5.0 (rawhide) is smarter than 4.4.4 (Fedora 13).
* src/xen/xend_internal.c (xenDaemonAttachDeviceFlags) (xenDaemonUpdateDeviceFlags, xenDaemonDetachDeviceFlags): Use correct operator.
ACK,
Thanks; pushed. I'll wait for Matthias' opinion on the ESX patch, since this was a bug fix, but that one is just cosmetic. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/esx/esx_vi_types.c (_DESERIALIZE_NUMBER) (ESX_VI__TEMPLATE__DESERIALIZE_NUMBER): Add range check to shut up gcc 4.5.0 regarding long long. --- src/esx/esx_vi_types.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index 6e75995..5cf30b1 100644 --- a/src/esx/esx_vi_types.c +++ b/src/esx/esx_vi_types.c @@ -333,7 +333,8 @@ goto cleanup; \ } \ \ - if (value < (_min) || value > (_max)) { \ + if (((_min) != INT64_MIN && value < (_min)) \ + || ((_max) != INT64_MAX && value > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ "Value '%s' is not representable as "_xsdType, \ (const char *)string); \ @@ -922,7 +923,8 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) goto failure; \ } \ \ - if (number < (_min) || number > (_max)) { \ + if (((_min) != INT64_MIN && number < (_min)) \ + || ((_max) != INT64_MAX && number > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ _("Value '%s' is out of %s range"), \ (*anyType)->value, _xsdType); \ -- 1.7.2
On Wed, Jul 28, 2010 at 05:34:15PM -0600, Eric Blake wrote:
* src/esx/esx_vi_types.c (_DESERIALIZE_NUMBER) (ESX_VI__TEMPLATE__DESERIALIZE_NUMBER): Add range check to shut up gcc 4.5.0 regarding long long. --- src/esx/esx_vi_types.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index 6e75995..5cf30b1 100644 --- a/src/esx/esx_vi_types.c +++ b/src/esx/esx_vi_types.c @@ -333,7 +333,8 @@ goto cleanup; \ } \ \ - if (value < (_min) || value > (_max)) { \ + if (((_min) != INT64_MIN && value < (_min)) \ + || ((_max) != INT64_MAX && value > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ "Value '%s' is not representable as "_xsdType, \ (const char *)string); \ @@ -922,7 +923,8 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) goto failure; \ } \ \ - if (number < (_min) || number > (_max)) { \ + if (((_min) != INT64_MIN && number < (_min)) \ + || ((_max) != INT64_MAX && number > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ _("Value '%s' is out of %s range"), \ (*anyType)->value, _xsdType); \ -- 1.7.2
Way harder to understand than the original, IMHO I defer to Matthias for this :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/7/29 Eric Blake <eblake@redhat.com>:
* src/esx/esx_vi_types.c (_DESERIALIZE_NUMBER) (ESX_VI__TEMPLATE__DESERIALIZE_NUMBER): Add range check to shut up gcc 4.5.0 regarding long long. --- src/esx/esx_vi_types.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_vi_types.c b/src/esx/esx_vi_types.c index 6e75995..5cf30b1 100644 --- a/src/esx/esx_vi_types.c +++ b/src/esx/esx_vi_types.c @@ -333,7 +333,8 @@ goto cleanup; \ } \ \ - if (value < (_min) || value > (_max)) { \ + if (((_min) != INT64_MIN && value < (_min)) \ + || ((_max) != INT64_MAX && value > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ "Value '%s' is not representable as "_xsdType, \ (const char *)string); \ @@ -922,7 +923,8 @@ esxVI_AnyType_Deserialize(xmlNodePtr node, esxVI_AnyType **anyType) goto failure; \ } \ \ - if (number < (_min) || number > (_max)) { \ + if (((_min) != INT64_MIN && number < (_min)) \ + || ((_max) != INT64_MAX && number > (_max))) { \ ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, \ _("Value '%s' is out of %s range"), \ (*anyType)->value, _xsdType); \ -- 1.7.2
It seems that newer gcc warns about this, because in case of _min/_max == INT64_MIN/INT64_MAX the original condition is always false and adding the additional checks avoids this situation. ACK. Matthias
On 07/29/2010 06:25 PM, Matthias Bolte wrote:
2010/7/29 Eric Blake <eblake@redhat.com>:
* src/esx/esx_vi_types.c (_DESERIALIZE_NUMBER) (ESX_VI__TEMPLATE__DESERIALIZE_NUMBER): Add range check to shut up gcc 4.5.0 regarding long long. --- src/esx/esx_vi_types.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
- if (value < (_min) || value > (_max)) { \ + if (((_min) != INT64_MIN && value < (_min)) \ + || ((_max) != INT64_MAX && value > (_max))) { \
It seems that newer gcc warns about this, because in case of _min/_max == INT64_MIN/INT64_MAX the original condition is always false and adding the additional checks avoids this situation.
The original condition is still false for INT64_MIN/INT64_MAX even with the additional checks, but the additional checks are enough to shut up gcc. Hopefully, since the additional checks are compile time constants (based on all uses of the macros) it doesn't bloat the assembly any. I just hope that we don't have to keep tweaking this down the road for future gcc (the whole point of the macro is that we know we are using the same code for multiple types, where the range checks are important for all but one of the types).
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte