[PATCH] net: core: skbuff: skb_checksum_setup() drop err

[PATCH] net: core: skbuff: skb_checksum_setup() drop err

[Vito Caputo]

Return directly from all switch cases, no point in storing in err.

Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
---
 net/core/skbuff.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f5f904f46893..c59b68a413b5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4888,23 +4888,14 @@ static int skb_checksum_setup_ipv6(struct sk_buff *skb, bool recalculate)
  */
 int skb_checksum_setup(struct sk_buff *skb, bool recalculate)
 {
-	int err;
-
 	switch (skb->protocol) {
 	case htons(ETH_P_IP):
-		err = skb_checksum_setup_ipv4(skb, recalculate);
-		break;
-
+		return skb_checksum_setup_ipv4(skb, recalculate);
 	case htons(ETH_P_IPV6):
-		err = skb_checksum_setup_ipv6(skb, recalculate);
-		break;
-
+		return skb_checksum_setup_ipv6(skb, recalculate);
 	default:
-		err = -EPROTO;
-		break;
+		return -EPROTO;
 	}
-
-	return err;
 }
 EXPORT_SYMBOL(skb_checksum_setup);
 
-- 
2.11.0

Re: [PATCH] net: core: skbuff: skb_checksum_setup() drop err

[Eric Dumazet]

On 10/12/19 5:30 PM, Vito Caputo wrote:
> Return directly from all switch cases, no point in storing in err.
> 
> Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
> ---
>  net/core/skbuff.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f5f904f46893..c59b68a413b5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4888,23 +4888,14 @@ static int skb_checksum_setup_ipv6(struct sk_buff *skb, bool recalculate)
>   */
>  int skb_checksum_setup(struct sk_buff *skb, bool recalculate)
>  {
> -	int err;
> -
>  	switch (skb->protocol) {
>  	case htons(ETH_P_IP):
> -		err = skb_checksum_setup_ipv4(skb, recalculate);
> -		break;
> -
> +		return skb_checksum_setup_ipv4(skb, recalculate);
>  	case htons(ETH_P_IPV6):
> -		err = skb_checksum_setup_ipv6(skb, recalculate);
> -		break;
> -
> +		return skb_checksum_setup_ipv6(skb, recalculate);
>  	default:
> -		err = -EPROTO;
> -		break;
> +		return -EPROTO;
>  	}
> -
> -	return err;
>  }
>  EXPORT_SYMBOL(skb_checksum_setup);


We prefer having a single return point in a function, if possible.

The err variable would make easier for debugging support,
if say a developer needs to trace this function.

Re: [PATCH] net: core: skbuff: skb_checksum_setup() drop err

[Vito Caputo]

On Sun, Oct 13, 2019 at 12:58:04PM -0700, Eric Dumazet wrote:
> 
> 
> On 10/12/19 5:30 PM, Vito Caputo wrote:
> > Return directly from all switch cases, no point in storing in err.
> > 
> > Signed-off-by: Vito Caputo <vcaputo@pengaru.com>
> > ---
> >  net/core/skbuff.c | 15 +++------------
> >  1 file changed, 3 insertions(+), 12 deletions(-)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index f5f904f46893..c59b68a413b5 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -4888,23 +4888,14 @@ static int skb_checksum_setup_ipv6(struct sk_buff *skb, bool recalculate)
> >   */
> >  int skb_checksum_setup(struct sk_buff *skb, bool recalculate)
> >  {
> > -	int err;
> > -
> >  	switch (skb->protocol) {
> >  	case htons(ETH_P_IP):
> > -		err = skb_checksum_setup_ipv4(skb, recalculate);
> > -		break;
> > -
> > +		return skb_checksum_setup_ipv4(skb, recalculate);
> >  	case htons(ETH_P_IPV6):
> > -		err = skb_checksum_setup_ipv6(skb, recalculate);
> > -		break;
> > -
> > +		return skb_checksum_setup_ipv6(skb, recalculate);
> >  	default:
> > -		err = -EPROTO;
> > -		break;
> > +		return -EPROTO;
> >  	}
> > -
> > -	return err;
> >  }
> >  EXPORT_SYMBOL(skb_checksum_setup);
> 
> 
> We prefer having a single return point in a function, if possible.
> 
> The err variable would make easier for debugging support,
> if say a developer needs to trace this function.
> 

Except there are examples under net/core of precisely this pattern, e.g.:

---

__be32 flow_get_u32_src(const struct flow_keys *flow)
{
        switch (flow->control.addr_type) {
        case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
                return flow->addrs.v4addrs.src;
        case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
                return (__force __be32)ipv6_addr_hash(
                        &flow->addrs.v6addrs.src);
        case FLOW_DISSECTOR_KEY_TIPC:
                return flow->addrs.tipckey.key;
        default:
                return 0;
        }
}
EXPORT_SYMBOL(flow_get_u32_src);

__be32 flow_get_u32_dst(const struct flow_keys *flow)
{
        switch (flow->control.addr_type) {
        case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
                return flow->addrs.v4addrs.dst;
        case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
                return (__force __be32)ipv6_addr_hash(
                        &flow->addrs.v6addrs.dst);
        default:
                return 0;
        }
}
EXPORT_SYMBOL(flow_get_u32_dst);

---

This compact form of mapping is found throughout the kernel, is
skb_checksum_setup() special?

Regards,
Vito Caputo

Re: [PATCH] net: core: skbuff: skb_checksum_setup() drop err

[Eric Dumazet]

On 10/13/19 1:17 PM, Vito Caputo wrote:

> 
> Except there are examples under net/core of precisely this pattern, e.g.:
> 
>

We do not care about having consistent code styles.

linux kernel has been contributed by thousands.

Each contributor has its own preferences.

We do not want to enforce very strong rules.
This is all cosmetic.

The real things we care are the backports we have to do every day.

Having these 'cleanups' in the way make our life miserable.