Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, July 10, 2020 3:16 PM
> 
> On Fri, Jul 10, 2020 at 03:10:34PM +0200, Morten Brørup wrote:
> > > From: Olivier Matz [mailto:olivier.matz@6wind.com]
> > > Sent: Friday, July 10, 2020 2:41 PM
> > >
> > > On Fri, Jul 10, 2020 at 02:55:51PM +0800, Hongzhi Guo wrote:
> > > > Per RFC768:
> > > > If the computed checksum is zero, it is transmitted as all ones.
> > > > An all zero transmitted checksum value means that the transmitter
> > > > generated no checksum.
> > > >
> > > > RFC793 for TCP has no such special treatment for the checksum of
> > > zero.
> > > >
> > > > Fixes: 6006818cfb26 ("net: new checksum functions")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Hongzhi Guo <guohongzhi1@huawei.com>
> > > > ---
> > > > v2:
> > > > * Fixed commit tile
> > > > * Fixed the API comment
> > > > ---
> > > > ---
> > > >  lib/librte_net/rte_ip.h | 18 +++++++++++++++---
> > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
> > > > index 292f63fd7..d03c77120 100644
> > > > --- a/lib/librte_net/rte_ip.h
> > > > +++ b/lib/librte_net/rte_ip.h
> > > > @@ -325,7 +325,7 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr
> > > *ipv4_hdr, uint64_t ol_flags)
> > > >   *   The pointer to the beginning of the L4 header.
> > > >   * @return
> > > >   *   The complemented checksum to set in the IP packet
> > > > - *   or 0 on error
> > > > + *   or 0 if the IP length is invalid in the header.
> > > >   */
> > > >  static inline uint16_t
> > > >  rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const
> > > void *l4_hdr)
> >
> > 0 is a valid return value, so I suggest omitting it from the return
> value description:
> >
> >   * @return
> > - *   The complemented checksum to set in the IP packet
> > - *   or 0 on error
> > + *   The complemented checksum to set in the IP packet.
> >
> > The comparison "if (l3_len < sizeof(struct rte_ipv4_hdr))" is only
> there to protect against invalid input; it prevents l4_len from
> becoming negative.
> 
> I don't get why "0 if the IP length is invalid in the header" should
> be removed from the comment: 0 is both a valid return value and
> the value returned on invalid packet.

To avoid confusion. We do not want people to add error handling for a return value of 0.

0 is not a special value or an error, so it does not deserve explicit mentioning.

If we want to mention the return value for garbage input, we should not use the wording "or 0", because this suggests that 0 is not a normal return value.

> 
> > For the same reason, unlikely() should be added to this comparison.
> 
> Maybe yes, but that's another story I think.

Agree. I was just mentioning it so it can be done when modifying the function anyway.

> 
> > Otherwise,
> >
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >