diff mbox series

[net-next] rxrpc: Fix missing IPV6 #ifdef

Message ID 166807341463.2904467.10141806642379634063.stgit@warthog.procyon.org.uk
State New
Headers
Series [net-next] rxrpc: Fix missing IPV6 #ifdef |

Commit Message

David Howells Nov. 10, 2022, 9:43 a.m. UTC 
  Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
on IPV6 support being enabled.

Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
cc: netdev@vger.kernel.org
---

 net/rxrpc/local_object.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Hideaki Yoshifuji Nov. 10, 2022, 10:42 a.m. UTC | #1 
Hi,

2022年11月10日(木) 18:44 David Howells <dhowells@redhat.com>:
>
> Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
> on IPV6 support being enabled.
>
> Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: netdev@vger.kernel.org
> ---
>
>  net/rxrpc/local_object.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
> index a178f71e5082..25cdfcf7b415 100644
> --- a/net/rxrpc/local_object.c
> +++ b/net/rxrpc/local_object.c
> @@ -33,7 +33,9 @@ static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
>  {
>         if (ip_hdr(skb)->version == IPVERSION)
>                 return ip_icmp_error(sk, skb, err, port, info, payload);
> +#ifdef CONFIG_AF_RXRPC_IPV6
>         return ipv6_icmp_error(sk, skb, err, port, info, payload);
> +#endif
>  }
>

Because this introduces a missing return literally without CONFIG_AF_RXRPC_IPV6,
I would prefer #ifdef / #else for the whole function.

--yoshfuji

>  /*
>
>
David Howells Nov. 10, 2022, 1:26 p.m. UTC | #2 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com> wrote:

> Because this introduces a missing return literally without
> CONFIG_AF_RXRPC_IPV6, I would prefer #ifdef / #else for the whole function.

They're both void functions, so actually, there's nothing to return.

David
Andrew Lunn Nov. 10, 2022, 1:47 p.m. UTC | #3 
On Thu, Nov 10, 2022 at 09:43:34AM +0000, David Howells wrote:
> Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
> on IPV6 support being enabled.
> 
> Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> cc: netdev@vger.kernel.org
> ---
> 
>  net/rxrpc/local_object.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
> index a178f71e5082..25cdfcf7b415 100644
> --- a/net/rxrpc/local_object.c
> +++ b/net/rxrpc/local_object.c
> @@ -33,7 +33,9 @@ static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
>  {
>  	if (ip_hdr(skb)->version == IPVERSION)
>  		return ip_icmp_error(sk, skb, err, port, info, payload);
> +#ifdef CONFIG_AF_RXRPC_IPV6
>  	return ipv6_icmp_error(sk, skb, err, port, info, payload);
> +#endif

Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
#ifdef? It gives better build testing.

	Andrew
David Howells Nov. 10, 2022, 2:09 p.m. UTC | #4 
Andrew Lunn <andrew@lunn.ch> wrote:

> > +#ifdef CONFIG_AF_RXRPC_IPV6
> >  	return ipv6_icmp_error(sk, skb, err, port, info, payload);
> > +#endif
> 
> Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
> #ifdef? It gives better build testing.

Sure.  Does it actually make that much of a difference?  I guess the
declaration is there even if IPV6 is disabled.

David
Andrew Lunn Nov. 10, 2022, 3:37 p.m. UTC | #5 
On Thu, Nov 10, 2022 at 02:09:45PM +0000, David Howells wrote:
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > > +#ifdef CONFIG_AF_RXRPC_IPV6
> > >  	return ipv6_icmp_error(sk, skb, err, port, info, payload);
> > > +#endif
> > 
> > Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
> > #ifdef? It gives better build testing.
> 
> Sure.  Does it actually make that much of a difference?  I guess the
> declaration is there even if IPV6 is disabled.

And that is the point. Even if this feature is disabled, the type
checking will be performed, the number of parameters etc. Somebody who
modifies ipv6_icmp_error() is going to find problems with a single
compilation, rather than having to use a big collection of random
configs.

So this is a review comment i often make. Avoid #ifdef if you can, use
IS_ENABLED() inside an if statement.

	Andrew
diff mbox series

Patch

diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index a178f71e5082..25cdfcf7b415 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -33,7 +33,9 @@  static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
 {
 	if (ip_hdr(skb)->version == IPVERSION)
 		return ip_icmp_error(sk, skb, err, port, info, payload);
+#ifdef CONFIG_AF_RXRPC_IPV6
 	return ipv6_icmp_error(sk, skb, err, port, info, payload);
+#endif
 }
 
 /*