Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Pablo Neira Ayuso]

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
>  wrote:
> 
> > if (x)
> >   return
> > ...
> >
> > rather than:
> >
> > if (!x == 0)
> 
> should remove the `!`, ex:
> 
> if (x == 0)

Amended this here, and applied. Thanks!

Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Pablo Neira Ayuso]

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
>  wrote:
> 
> > if (x)
> >   return
> > ...
> >
> > rather than:
> >
> > if (!x == 0)
> 
> should remove the `!`, ex:
> 
> if (x == 0)

Amended this here, and applied. Thanks!

Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

bumping for review (resending, had gmail set to html mode)

On Mon, Aug 14, 2017 at 10:36 AM, 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
>  wrote:
>
>> if (x)
>>   return
>> ...
>>
>> rather than:
>>
>> if (!x == 0)
>
> should remove the `!`, ex:
>
> if (x == 0)
>
>>   ...
>> else
>>   return
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

bumping for review (resending, had gmail set to html mode)

On Mon, Aug 14, 2017 at 10:36 AM, 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
>  wrote:
>
>> if (x)
>>   return
>> ...
>>
>> rather than:
>>
>> if (!x == 0)
>
> should remove the `!`, ex:
>
> if (x == 0)
>
>>   ...
>> else
>>   return
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

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
 wrote:

> if (x)
>   return
> ...
>
> rather than:
>
> if (!x == 0)

should remove the `!`, ex:

if (x == 0)

>   ...
> else
>   return

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

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
 wrote:

> if (x)
>   return
> ...
>
> rather than:
>
> if (!x == 0)

should remove the `!`, ex:

if (x == 0)

>   ...
> else
>   return

-- 
Thanks,
~Nick Desaulniers

[PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

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 
---
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,
  >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",
 _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,
  >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",
 >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, [idx],
  >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, [0], , ) &&
-   (ntohl(addr.ip) & 0xff00) == 0x7f00) {
-   set_h225_addr(skb, protoff, data, 0, [0],
- >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, [0], , ) &&
+   (ntohl(addr.ip) & 0xff00) == 0x7f00) {
+   if (set_h225_addr(skb, protoff, data, 0, [0],
+ >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",
 >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,
-  >tuplehash[!dir].tuple.dst.u3,
-  htons(nated_port)) == 0) {
+   if (set_h225_addr(skb, 

[PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning

[Nick Desaulniers]

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 
---
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,
  >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",
 _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,
  >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",
 >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, [idx],
  >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, [0], , ) &&
-   (ntohl(addr.ip) & 0xff00) == 0x7f00) {
-   set_h225_addr(skb, protoff, data, 0, [0],
- >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, [0], , ) &&
+   (ntohl(addr.ip) & 0xff00) == 0x7f00) {
+   if (set_h225_addr(skb, protoff, data, 0, [0],
+ >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",
 >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,
-  >tuplehash[!dir].tuple.dst.u3,
-  htons(nated_port)) == 0) {
+   if (set_h225_addr(skb, protoff, data, dataoff,