* [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
@ 2017-07-31 18:39 Nick Desaulniers
2017-08-08 23:28 ` Nick Desaulniers
2017-08-11 17:42 ` Pablo Neira Ayuso
0 siblings, 2 replies; 7+ messages in thread
From: Nick Desaulniers @ 2017-07-31 18:39 UTC (permalink / raw)
Cc: mka, lorenzo, Nick Desaulniers, Pablo Neira Ayuso,
Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam,
netdev, linux-kernel
Clang produces the following warning:
net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
logical not is only applied to the left hand side of this comparison
[-Werror,-Wlogical-not-parentheses]
if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
^
add parentheses after the '!' to evaluate the comparison first
add parentheses around left hand side expression to silence this warning
There's not necessarily a bug here, but it's cleaner to use the form:
if (x != 0)
rather than:
if (!x == 0)
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Also, it's even cleaner to use the form:
if (x)
but then if the return codes change from treating 0 as success (unlikely),
then all call sites must be updated.
I'm happy to send v2 that changes to that form, and updates the other call
sites to be:
if (set_h225_addr())
handle_failures()
else
handle_success()
net/ipv4/netfilter/nf_nat_h323.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7ebba0b6..d8fb251fa6e3 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -550,9 +550,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
}
/* Modify signal */
- if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
- &ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
+ if (set_h225_addr(skb, protoff, data, dataoff, taddr,
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ htons(nated_port)) != 0) {
nf_ct_unexpect_related(exp);
return -1;
}
--
2.14.0.rc0.400.g1c36432dff-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-07-31 18:39 [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers
@ 2017-08-08 23:28 ` Nick Desaulniers
2017-08-11 17:42 ` Pablo Neira Ayuso
1 sibling, 0 replies; 7+ messages in thread
From: Nick Desaulniers @ 2017-08-08 23:28 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Matthias Kaehlcke, Lorenzo Colitti, Pablo Neira Ayuso,
Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam,
netdev, linux-kernel
bumping for review
On Mon, Jul 31, 2017 at 11:39 AM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> Clang produces the following warning:
>
> net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
> logical not is only applied to the left hand side of this comparison
> [-Werror,-Wlogical-not-parentheses]
> if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
> ^
> add parentheses after the '!' to evaluate the comparison first
> add parentheses around left hand side expression to silence this warning
>
> There's not necessarily a bug here, but it's cleaner to use the form:
>
> if (x != 0)
>
> rather than:
>
> if (!x == 0)
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Also, it's even cleaner to use the form:
>
> if (x)
>
> but then if the return codes change from treating 0 as success (unlikely),
> then all call sites must be updated.
>
> I'm happy to send v2 that changes to that form, and updates the other call
> sites to be:
>
> if (set_h225_addr())
> handle_failures()
> else
> handle_success()
>
> net/ipv4/netfilter/nf_nat_h323.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
> index 574f7ebba0b6..d8fb251fa6e3 100644
> --- a/net/ipv4/netfilter/nf_nat_h323.c
> +++ b/net/ipv4/netfilter/nf_nat_h323.c
> @@ -550,9 +550,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
> }
>
> /* Modify signal */
> - if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
> - &ct->tuplehash[!dir].tuple.dst.u3,
> - htons(nated_port)) == 0) {
> + if (set_h225_addr(skb, protoff, data, dataoff, taddr,
> + &ct->tuplehash[!dir].tuple.dst.u3,
> + htons(nated_port)) != 0) {
> nf_ct_unexpect_related(exp);
> return -1;
> }
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-07-31 18:39 [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers
2017-08-08 23:28 ` Nick Desaulniers
@ 2017-08-11 17:42 ` Pablo Neira Ayuso
2017-08-11 18:16 ` [PATCH v2] " Nick Desaulniers
1 sibling, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2017-08-11 17:42 UTC (permalink / raw)
To: Nick Desaulniers
Cc: mka, lorenzo, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
netfilter-devel, coreteam, netdev, linux-kernel
Hi Nick,
On Mon, Jul 31, 2017 at 11:39:49AM -0700, Nick Desaulniers wrote:
> Clang produces the following warning:
[...]
> Also, it's even cleaner to use the form:
>
> if (x)
>
> but then if the return codes change from treating 0 as success (unlikely),
> then all call sites must be updated.
>
> I'm happy to send v2 that changes to that form, and updates the other call
> sites to be:
>
> if (set_h225_addr())
> handle_failures()
> else
> handle_success()
That sounds very reasonable, send a v2 if this triggers a larger
patch.
Or I can just take this patch, as you prefer.
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-08-11 17:42 ` Pablo Neira Ayuso
@ 2017-08-11 18:16 ` Nick Desaulniers
2017-08-14 17:36 ` Nick Desaulniers
0 siblings, 1 reply; 7+ messages in thread
From: Nick Desaulniers @ 2017-08-11 18:16 UTC (permalink / raw)
To: pablo
Cc: mka, lorenzo, Nick Desaulniers, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev,
linux-kernel
Clang produces the following warning:
net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
logical not is only applied to the left hand side of this comparison
[-Werror,-Wlogical-not-parentheses]
if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
^
add parentheses after the '!' to evaluate the comparison first
add parentheses around left hand side expression to silence this warning
There's not necessarily a bug here, but it's cleaner to return early,
ex:
if (x)
return
...
rather than:
if (!x == 0)
...
else
return
Also added a return code check that seemed to be missing in one
instance.
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes in v2:
* Reorder if/else blocks to return early.
* Also handle this for set_h245_addr(), not just set_h225_addr().
* Add return code check for the Gnomemeeting case.
net/ipv4/netfilter/nf_nat_h323.c | 57 +++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7ebba0b6..ac8342dcb55e 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -252,16 +252,16 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct nf_conn *ct,
if (set_h245_addr(skb, protoff, data, dataoff, taddr,
&ct->tuplehash[!dir].tuple.dst.u3,
htons((port & htons(1)) ? nated_port + 1 :
- nated_port)) == 0) {
- /* Save ports */
- info->rtp_port[i][dir] = rtp_port;
- info->rtp_port[i][!dir] = htons(nated_port);
- } else {
+ nated_port))) {
nf_ct_unexpect_related(rtp_exp);
nf_ct_unexpect_related(rtcp_exp);
return -1;
}
+ /* Save ports */
+ info->rtp_port[i][dir] = rtp_port;
+ info->rtp_port[i][!dir] = htons(nated_port);
+
/* Success */
pr_debug("nf_nat_h323: expect RTP %pI4:%hu->%pI4:%hu\n",
&rtp_exp->tuple.src.u3.ip,
@@ -370,15 +370,15 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn *ct,
/* Modify signal */
if (set_h225_addr(skb, protoff, data, dataoff, taddr,
&ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
- /* Save ports */
- info->sig_port[dir] = port;
- info->sig_port[!dir] = htons(nated_port);
- } else {
+ htons(nated_port))) {
nf_ct_unexpect_related(exp);
return -1;
}
+ /* Save ports */
+ info->sig_port[dir] = port;
+ info->sig_port[!dir] = htons(nated_port);
+
pr_debug("nf_nat_q931: expect H.245 %pI4:%hu->%pI4:%hu\n",
&exp->tuple.src.u3.ip,
ntohs(exp->tuple.src.u.tcp.port),
@@ -462,24 +462,27 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn *ct,
/* Modify signal */
if (set_h225_addr(skb, protoff, data, 0, &taddr[idx],
&ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
- /* Save ports */
- info->sig_port[dir] = port;
- info->sig_port[!dir] = htons(nated_port);
-
- /* Fix for Gnomemeeting */
- if (idx > 0 &&
- get_h225_addr(ct, *data, &taddr[0], &addr, &port) &&
- (ntohl(addr.ip) & 0xff000000) == 0x7f000000) {
- set_h225_addr(skb, protoff, data, 0, &taddr[0],
- &ct->tuplehash[!dir].tuple.dst.u3,
- info->sig_port[!dir]);
- }
- } else {
+ htons(nated_port))) {
nf_ct_unexpect_related(exp);
return -1;
}
+ /* Save ports */
+ info->sig_port[dir] = port;
+ info->sig_port[!dir] = htons(nated_port);
+
+ /* Fix for Gnomemeeting */
+ if (idx > 0 &&
+ get_h225_addr(ct, *data, &taddr[0], &addr, &port) &&
+ (ntohl(addr.ip) & 0xff000000) == 0x7f000000) {
+ if (set_h225_addr(skb, protoff, data, 0, &taddr[0],
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ info->sig_port[!dir])) {
+ nf_ct_unexpect_related(exp);
+ return -1;
+ }
+ }
+
/* Success */
pr_debug("nf_nat_ras: expect Q.931 %pI4:%hu->%pI4:%hu\n",
&exp->tuple.src.u3.ip,
@@ -550,9 +553,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct nf_conn *ct,
}
/* Modify signal */
- if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
- &ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
+ if (set_h225_addr(skb, protoff, data, dataoff, taddr,
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ htons(nated_port))) {
nf_ct_unexpect_related(exp);
return -1;
}
--
2.14.0.434.g98096fd7a8-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-08-11 18:16 ` [PATCH v2] " Nick Desaulniers
@ 2017-08-14 17:36 ` Nick Desaulniers
2017-08-24 16:25 ` Nick Desaulniers
2017-08-24 16:49 ` Pablo Neira Ayuso
0 siblings, 2 replies; 7+ messages in thread
From: Nick Desaulniers @ 2017-08-14 17:36 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Matthias Kaehlcke, Lorenzo Colitti, Nick Desaulniers,
Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam,
netdev, linux-kernel
Minor nit for the commit message that can get fixed up when being merged:
On Fri, Aug 11, 2017 at 11:16 AM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> if (x)
> return
> ...
>
> rather than:
>
> if (!x == 0)
should remove the `!`, ex:
if (x == 0)
> ...
> else
> return
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-08-14 17:36 ` Nick Desaulniers
@ 2017-08-24 16:25 ` Nick Desaulniers
2017-08-24 16:49 ` Pablo Neira Ayuso
1 sibling, 0 replies; 7+ messages in thread
From: Nick Desaulniers @ 2017-08-24 16:25 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Matthias Kaehlcke, Lorenzo Colitti, Nick Desaulniers,
Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, netfilter-devel, coreteam,
netdev, linux-kernel
bumping for review (resending, had gmail set to html mode)
On Mon, Aug 14, 2017 at 10:36 AM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> Minor nit for the commit message that can get fixed up when being merged:
>
> On Fri, Aug 11, 2017 at 11:16 AM, Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>
>> if (x)
>> return
>> ...
>>
>> rather than:
>>
>> if (!x == 0)
>
> should remove the `!`, ex:
>
> if (x == 0)
>
>> ...
>> else
>> return
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
2017-08-14 17:36 ` Nick Desaulniers
2017-08-24 16:25 ` Nick Desaulniers
@ 2017-08-24 16:49 ` Pablo Neira Ayuso
1 sibling, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2017-08-24 16:49 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Matthias Kaehlcke, Lorenzo Colitti, Jozsef Kadlecsik,
Florian Westphal, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, netfilter-devel, coreteam, netdev,
linux-kernel
On Mon, Aug 14, 2017 at 10:36:03AM -0700, Nick Desaulniers wrote:
> Minor nit for the commit message that can get fixed up when being merged:
>
> On Fri, Aug 11, 2017 at 11:16 AM, Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>
> > if (x)
> > return
> > ...
> >
> > rather than:
> >
> > if (!x == 0)
>
> should remove the `!`, ex:
>
> if (x == 0)
Amended this here, and applied. Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-08-24 16:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-31 18:39 [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers
2017-08-08 23:28 ` Nick Desaulniers
2017-08-11 17:42 ` Pablo Neira Ayuso
2017-08-11 18:16 ` [PATCH v2] " Nick Desaulniers
2017-08-14 17:36 ` Nick Desaulniers
2017-08-24 16:25 ` Nick Desaulniers
2017-08-24 16:49 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).