* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.