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

[Vito Caputo <vcaputo@pengaru.com>]

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