[Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

1. add some missing #if IS_ENABLED(CONFIG_IPV6)
2. make ping_supported() bool
3. fix "%pI6c" format

Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/ipv4/ping.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 71f6ad0..8c92781 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -318,7 +318,7 @@ int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 			return -EINVAL;
 
 		pr_debug("ping_check_bind_addr(sk=%p,addr=%pI6c,port=%d)\n",
-			 sk, addr->sin6_addr.s6_addr, ntohs(addr->sin6_port));
+			 sk, &addr->sin6_addr, ntohs(addr->sin6_port));
 
 		addr_type = ipv6_addr_type(&addr->sin6_addr);
 		scoped = __ipv6_addr_needs_scope_id(addr_type);
@@ -444,10 +444,18 @@ EXPORT_SYMBOL_GPL(ping_bind);
  * Is this a supported type of ICMP message?
  */
 
-static inline int ping_supported(int family, int type, int code)
+static inline bool ping_supported(int family, int type, int code)
 {
-	return (family == AF_INET && type == ICMP_ECHO && code == 0) ||
-	       (family == AF_INET6 && type == ICMPV6_ECHO_REQUEST && code == 0);
+	if (family == AF_INET) {
+		if (type == ICMP_ECHO && code == 0)
+			return true;
+#if IS_ENABLED(CONFIG_IPV6)
+	} else if (family == AF_INET6) {
+		if (type == ICMPV6_ECHO_REQUEST && code == 0)
+			return true;
+#endif
+	}
+	return false;
 }
 
 /*
@@ -472,11 +480,13 @@ void ping_err(struct sk_buff *skb, int offset, u32 info)
 		type = icmp_hdr(skb)->type;
 		code = icmp_hdr(skb)->code;
 		icmph = (struct icmphdr *)(skb->data + offset);
+#if IS_ENABLED(CONFIG_IPV6)
 	} else if (skb->protocol == htons(ETH_P_IPV6)) {
 		family = AF_INET6;
 		type = icmp6_hdr(skb)->icmp6_type;
 		code = icmp6_hdr(skb)->icmp6_code;
 		icmph = (struct icmphdr *) (skb->data + offset);
+#endif
 	} else {
 		BUG();
 	}
-- 
1.7.7.6

[Patch net-next 2/2] ping: use ipv6_iface_scope_id() to get scope id

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

If we don't need scope id, we should initialize it to zero.

Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/ipv4/ping.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 8c92781..b1993b3 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -905,9 +905,8 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 		if (np->sndflow)
 			sin6->sin6_flowinfo = ip6_flowinfo(ip6);
 
-		if (__ipv6_addr_needs_scope_id(
-		    ipv6_addr_type(&sin6->sin6_addr)))
-			sin6->sin6_scope_id = IP6CB(skb)->iif;
+		sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
+							  IP6CB(skb)->iif);
 
 		if (inet6_sk(sk)->rxopt.all)
 			pingv6_ops.ip6_datagram_recv_ctl(sk, msg, skb);
-- 
1.7.7.6

Re: [Patch net-next 2/2] ping: use ipv6_iface_scope_id() to get scope id

[Lorenzo Colitti]

On Mon, Jun 3, 2013 at 2:55 PM, Cong Wang <amwang@redhat.com> wrote:
> From: Cong Wang <amwang@redhat.com>
>
> If we don't need scope id, we should initialize it to zero.
>
> Cc: Lorenzo Colitti <lorenzo@google.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
>  net/ipv4/ping.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 8c92781..b1993b3 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -905,9 +905,8 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
>                 if (np->sndflow)
>                         sin6->sin6_flowinfo = ip6_flowinfo(ip6);
>
> -               if (__ipv6_addr_needs_scope_id(
> -                   ipv6_addr_type(&sin6->sin6_addr)))
> -                       sin6->sin6_scope_id = IP6CB(skb)->iif;
> +               sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
> +                                                         IP6CB(skb)->iif);
>
>                 if (inet6_sk(sk)->rxopt.all)
>                         pingv6_ops.ip6_datagram_recv_ctl(sk, msg, skb);
> --
> 1.7.7.6
>

Good catch, thanks. While you're at it - if np->sndflow is false,
sin6_flowinfo is not being initialized either. Care to fix that too?
The other functions do this like this:

sin->sin6_flowinfo = 0;
if (np->sndflow)
        sin6->sin6_flowinfo = ip6_flowinfo(ip6);

Re: [Patch net-next 2/2] ping: use ipv6_iface_scope_id() to get scope id

[Cong Wang]

On Mon, 2013-06-03 at 16:59 +0900, Lorenzo Colitti wrote:
> 
> Good catch, thanks. While you're at it - if np->sndflow is false,
> sin6_flowinfo is not being initialized either. Care to fix that too?
> The other functions do this like this:
> 
> sin->sin6_flowinfo = 0;
> if (np->sndflow)
>         sin6->sin6_flowinfo = ip6_flowinfo(ip6);

OK, I will add this piece and resend patch 2/2.

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Joe Perches]

On Mon, 2013-06-03 at 13:55 +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
[]
> 3. fix "%pI6c" format
[]
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
[]
> @@ -318,7 +318,7 @@ int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
>  			return -EINVAL;
>  
>  		pr_debug("ping_check_bind_addr(sk=%p,addr=%pI6c,port=%d)\n",
> -			 sk, addr->sin6_addr.s6_addr, ntohs(addr->sin6_port));
> +			 sk, &addr->sin6_addr, ntohs(addr->sin6_port));

why does this matter?

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Cong Wang]

On Sun, 2013-06-02 at 22:59 -0700, Joe Perches wrote:
> 
> why does this matter?
> 
> 

It is not a problem at all, I just want to make it clear that %pI6c
accepts struct in6_addr *.

BTW, $subject already says this patch is a pure cleanup.

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Lorenzo Colitti]

On Mon, Jun 3, 2013 at 2:55 PM, Cong Wang <amwang@redhat.com> wrote:
> -static inline int ping_supported(int family, int type, int code)
> +static inline bool ping_supported(int family, int type, int code)
>  {
> -       return (family == AF_INET && type == ICMP_ECHO && code == 0) ||
> -              (family == AF_INET6 && type == ICMPV6_ECHO_REQUEST && code == 0);
> +       if (family == AF_INET) {
> +               if (type == ICMP_ECHO && code == 0)
> +                       return true;
> +#if IS_ENABLED(CONFIG_IPV6)
> +       } else if (family == AF_INET6) {
> +               if (type == ICMPV6_ECHO_REQUEST && code == 0)
> +                       return true;
> +#endif
> +       }
> +       return false;
>  }

I don't think it's necessary to #ifdef out code here. It's not needed
to get the code to compile, because AF_INET6 and ICMPV6_ECHO_REQUEST
are defined even if CONFIG_IPV6=n. It's also not necessary at runtime,
because if we get here with family = AF_INET6 even though IPv6 is
disabled, then then there's already a problem. Adding the ifdef also
makes the function much hard to read.

>
>  /*
> @@ -472,11 +480,13 @@ void ping_err(struct sk_buff *skb, int offset, u32 info)
>                 type = icmp_hdr(skb)->type;
>                 code = icmp_hdr(skb)->code;
>                 icmph = (struct icmphdr *)(skb->data + offset);
> +#if IS_ENABLED(CONFIG_IPV6)
>         } else if (skb->protocol == htons(ETH_P_IPV6)) {
>                 family = AF_INET6;
>                 type = icmp6_hdr(skb)->icmp6_type;
>                 code = icmp6_hdr(skb)->icmp6_code;
>                 icmph = (struct icmphdr *) (skb->data + offset);
> +#endif
>         } else {
>                 BUG();
>         }

I think this is unnecessary too. ping_err is only called by icmp_err
and by icmpv6_err, and if icmp_err is calling us on an IPv4 packet, or
if icmpv6_err is calling us when IPv6 is unloaded, then there are
bigger problems.

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Cong Wang]

On Mon, 2013-06-03 at 16:02 +0900, Lorenzo Colitti wrote:
> On Mon, Jun 3, 2013 at 2:55 PM, Cong Wang <amwang@redhat.com> wrote:
> > -static inline int ping_supported(int family, int type, int code)
> > +static inline bool ping_supported(int family, int type, int code)
> >  {
> > -       return (family == AF_INET && type == ICMP_ECHO && code == 0) ||
> > -              (family == AF_INET6 && type == ICMPV6_ECHO_REQUEST && code == 0);
> > +       if (family == AF_INET) {
> > +               if (type == ICMP_ECHO && code == 0)
> > +                       return true;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +       } else if (family == AF_INET6) {
> > +               if (type == ICMPV6_ECHO_REQUEST && code == 0)
> > +                       return true;
> > +#endif
> > +       }
> > +       return false;
> >  }
> 
> I don't think it's necessary to #ifdef out code here. It's not needed
> to get the code to compile, because AF_INET6 and ICMPV6_ECHO_REQUEST
> are defined even if CONFIG_IPV6=n. It's also not necessary at runtime,
> because if we get here with family = AF_INET6 even though IPv6 is
> disabled, then then there's already a problem. Adding the ifdef also
> makes the function much hard to read.

Remember it is a cleanup, not a bug fix, right? :) I don't think there
is any way to make this 10- line of code hard to read.

> 
> >
> >  /*
> > @@ -472,11 +480,13 @@ void ping_err(struct sk_buff *skb, int offset, u32 info)
> >                 type = icmp_hdr(skb)->type;
> >                 code = icmp_hdr(skb)->code;
> >                 icmph = (struct icmphdr *)(skb->data + offset);
> > +#if IS_ENABLED(CONFIG_IPV6)
> >         } else if (skb->protocol == htons(ETH_P_IPV6)) {
> >                 family = AF_INET6;
> >                 type = icmp6_hdr(skb)->icmp6_type;
> >                 code = icmp6_hdr(skb)->icmp6_code;
> >                 icmph = (struct icmphdr *) (skb->data + offset);
> > +#endif
> >         } else {
> >                 BUG();
> >         }
> 
> I think this is unnecessary too. ping_err is only called by icmp_err
> and by icmpv6_err, and if icmp_err is calling us on an IPv4 packet, or
> if icmpv6_err is calling us when IPv6 is unloaded, then there are
> bigger problems.

Again, it is just a clean up.

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[Lorenzo Colitti]

On Mon, Jun 3, 2013 at 4:13 PM, Cong Wang <amwang@redhat.com> wrote:
> Remember it is a cleanup, not a bug fix, right? :) I don't think there
> is any way to make this 10- line of code hard to read.

I suppose, if growing the function from 2 to 9 lines without changing
its behaviour can be called a "cleanup". :-)

Re: [Patch net-next 1/2] ping: some cleanup for net/ipv4/ping.c

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Mon, 3 Jun 2013 16:17:25 +0900

> On Mon, Jun 3, 2013 at 4:13 PM, Cong Wang <amwang@redhat.com> wrote:
>> Remember it is a cleanup, not a bug fix, right? :) I don't think there
>> is any way to make this 10- line of code hard to read.
> 
> I suppose, if growing the function from 2 to 9 lines without changing
> its behaviour can be called a "cleanup". :-)

I have to say this patch makes the code look worse, ifdefs are 99
times out of 100 the wrong thing to do.

Sorry I won't be applying these patches.