[libvirt] [PATCH] macvtap.c: add parenthesis for similar expressions
It wouldn't hurt to add some parenthesis in the following two similar expression for better readability. --- src/util/macvtap.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 09d7b78..92147ab 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_kernel, memcpy(ifla_vf_mac.mac, macaddr, 6); - if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if ((!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac)) < 0) goto buffer_too_small; } @@ -1060,8 +1060,8 @@ doPortProfileOpSetLink(bool nltarget_kernel, .qos = 0, }; - if (!nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) + if ((!nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), + &ifla_vf_vlan)) < 0) goto buffer_too_small; } -- 1.7.1
On 02/03/2011 11:12 AM, Davidlohr Bueso wrote:
It wouldn't hurt to add some parenthesis in the following two similar expression for better readability.
I take it you were using some gcc option about precedence of operations being confusing between ! and <; including that gcc warning message in your commit message can aid the discussion.
--- src/util/macvtap.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 09d7b78..92147ab 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_kernel,
memcpy(ifla_vf_mac.mac, macaddr, 6);
- if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if ((!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac)) < 0)
The indentation is wrong, too; the &ifla... of the second line should line up just after nla_put( of the first line. That was botched in December by commit 013c000 when the lines were reindented. Actually, you've uncovered a bigger bug, but didn't fix it right. Here, 'expr' is 'nla_put(...)'. But '!expr < 0' will never be true. ('!expr' is either 0 or 1, which is always non-negative). We either meant 'expr < 0' or '!expr', but not both (or maybe we meant '!(expr < 0)', which is easier if written 'expr >= 0'). The bug has existed since Stefan's commit 77e7c13 in November; and looking at the rest of that commit, it _looks_ like we meant to use 'expr < 0' (that is, the correct fix is to delete !), but I'd like confirmation from Stefan. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 02/03/2011 01:28:36 PM:
On 02/03/2011 11:12 AM, Davidlohr Bueso wrote:
It wouldn't hurt to add some parenthesis in the following two similar expression for better readability.
I take it you were using some gcc option about precedence of operations being confusing between ! and <; including that gcc warning message in your commit message can aid the discussion.
--- src/util/macvtap.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 09d7b78..92147ab 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_kernel,
memcpy(ifla_vf_mac.mac, macaddr, 6);
- if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if ((!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac)) < 0)
The indentation is wrong, too; the &ifla... of the second line should line up just after nla_put( of the first line. That was botched in December by commit 013c000 when the lines were reindented.
Actually, you've uncovered a bigger bug, but didn't fix it right.
ohoh, yes, there's a bug. This is how it should look like --- src/util/macvtap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_ker memcpy(ifla_vf_mac.mac, macaddr, 6); - if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac) < 0) goto buffer_too_small; } @@ -1060,8 +1060,8 @@ doPortProfileOpSetLink(bool nltarget_ker .qos = 0, }; - if (!nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), - &ifla_vf_vlan) < 0) + if (nla_put(nl_msg, IFLA_VF_VLAN, sizeof(ifla_vf_vlan), + &ifla_vf_vlan) < 0) goto buffer_too_small; } Regards, Stefan
On 02/03/2011 11:41 AM, Stefan Berger wrote:
Actually, you've uncovered a bigger bug, but didn't fix it right.
ohoh, yes, there's a bug. This is how it should look like
--- src/util/macvtap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_ker
memcpy(ifla_vf_mac.mac, macaddr, 6);
- if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac) < 0) goto buffer_too_small;
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> wrote on 02/03/2011 02:31:13 PM:
On 02/03/2011 11:41 AM, Stefan Berger wrote:
Actually, you've uncovered a bigger bug, but didn't fix it right.
ohoh, yes, there's a bug. This is how it should look like
--- src/util/macvtap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Index: libvirt-acl/src/util/macvtap.c =================================================================== --- libvirt-acl.orig/src/util/macvtap.c +++ libvirt-acl/src/util/macvtap.c @@ -1048,8 +1048,8 @@ doPortProfileOpSetLink(bool nltarget_ker
memcpy(ifla_vf_mac.mac, macaddr, 6);
- if (!nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), - &ifla_vf_mac) < 0) + if (nla_put(nl_msg, IFLA_VF_MAC, sizeof(ifla_vf_mac), + &ifla_vf_mac) < 0) goto buffer_too_small;
ACK.
Pushed.
participants (3)
-
Davidlohr Bueso -
Eric Blake -
Stefan Berger