Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1700312 > unrolled thread
| Started by | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| First post | 2017-07-31 20:50 +0200 |
| Last post | 2017-08-24 19:00 +0200 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers <ndesaulniers@google.com> - 2017-07-31 20:50 +0200
Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers <ndesaulniers@google.com> - 2017-08-09 01:30 +0200
Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning Pablo Neira Ayuso <pablo@netfilter.org> - 2017-08-11 19:50 +0200
[PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers <ndesaulniers@google.com> - 2017-08-11 20:20 +0200
Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers <ndesaulniers@google.com> - 2017-08-14 19:40 +0200
Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning Nick Desaulniers <ndesaulniers@google.com> - 2017-08-24 18:30 +0200
Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning Pablo Neira Ayuso <pablo@netfilter.org> - 2017-08-24 19:00 +0200
| From | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| Date | 2017-07-31 20:50 +0200 |
| Subject | [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <u9nVn-1jR-1@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| Date | 2017-08-09 01:30 +0200 |
| Message-ID | <ucm6K-73w-17@gated-at.bofh.it> |
| In reply to | #1700312 |
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
[toc] | [prev] | [next] | [standalone]
| From | Pablo Neira Ayuso <pablo@netfilter.org> |
|---|---|
| Date | 2017-08-11 19:50 +0200 |
| Subject | Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <udmel-6sy-7@gated-at.bofh.it> |
| In reply to | #1700312 |
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!
[toc] | [prev] | [next] | [standalone]
| From | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| Date | 2017-08-11 20:20 +0200 |
| Subject | [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <udmHn-6RA-9@gated-at.bofh.it> |
| In reply to | #1709890 |
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
[toc] | [prev] | [next] | [standalone]
| From | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| Date | 2017-08-14 19:40 +0200 |
| Subject | Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <uervk-72Y-23@gated-at.bofh.it> |
| In reply to | #1709902 |
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
[toc] | [prev] | [next] | [standalone]
| From | Nick Desaulniers <ndesaulniers@google.com> |
|---|---|
| Date | 2017-08-24 18:30 +0200 |
| Subject | Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <ui3b3-tc-1@gated-at.bofh.it> |
| In reply to | #1711278 |
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
[toc] | [prev] | [next] | [standalone]
| From | Pablo Neira Ayuso <pablo@netfilter.org> |
|---|---|
| Date | 2017-08-24 19:00 +0200 |
| Subject | Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning |
| Message-ID | <ui3E6-Db-27@gated-at.bofh.it> |
| In reply to | #1711278 |
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!
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web