Re: [PATCH 1/7] net: Don't set transport offset to invalid value

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > This is a lot of code change. This would do.
> >
> > @@ -2434,8 +2434,6 @@ static inline void
> > skb_probe_transport_header(struct sk_buff *skb,
> >
> >         if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
> >                 skb_set_transport_header(skb, keys.control.thoff);
> > -       else
> > -               skb_set_transport_header(skb, offset_hint);
> >  }
> >
> > Though leaving an unused argument is a bit ugly. For net-next, indeed
> > better to clean up (please mark your patchset with net or net-next,
> > btw)
>
> It's for net-next (I'll resend with the correct mark), so I'll stick
> with the current implementation.

Absolutely, sounds good.

> > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> > netback/netback.c
> > > index 80aae3a32c2a..b49b6e56ca47 100644
> > > --- a/drivers/net/xen-netback/netback.c
> > > +++ b/drivers/net/xen-netback/netback.c
> > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue
> > *queue)
> > >                 struct xen_netif_tx_request *txp;
> > >                 u16 pending_idx;
> > >                 unsigned data_len;
> > > +               bool th_set;
> > >
> > >                 pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> > >                 txp = &queue->pending_tx_info[pending_idx].req;
> > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct xenvif_queue
> > *queue)
> > >                         continue;
> > >                 }
> > >
> > > -               skb_probe_transport_header(skb, 0);
> > > +               th_set = skb_try_probe_transport_header(skb);
> >
> > Can use skb_transport_header_was_set(). Then at least there is no need
> > to change the function's return value.
>
> I suppose this comment relates to the previous one, and if we do it for
> net-next, it's fine to make change I made, isn't it?

If this is the only reason for the boolean return value, using
skb_transport_header_was_set() is more standard (I immediately know
what's happening when I read it), slightly less code change and avoids
introducing a situation where the majority of callers ignore a return
value. I think it's preferable. But these merits are certainly
debatable, so either is fine.