patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Morten Brørup <mb@smartsharesystems.com>
To: "Olivier Matz" <olivier.matz@6wind.com>
Cc: "Hongzhi Guo" <guohongzhi1@huawei.com>, <dev@dpdk.org>,
	<stable@dpdk.org>, <stephen@networkplumber.org>,
	<thomas@monjalon.net>, <konstantin.ananyev@intel.com>,
	<ferruh.yigit@intel.com>, <nicolas.chautru@intel.com>,
	<zhoujingbin@huawei.com>, <chenchanghu@huawei.com>,
	<jerry.lilijun@huawei.com>, <haifeng.lin@huawei.com>
Subject: Re: [dpdk-stable] [PATCH] net: fix unneeded replacement of 0 by ffff for TCP checksum
Date: Fri, 10 Jul 2020 15:29:36 +0200
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C61119@smartserver.smartshare.dk> (raw)
In-Reply-To: <20200710131604.GB5869@platinum>

> 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>
> >


  reply	other threads:[~2020-07-10 13:29 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-10  6:55 Hongzhi Guo
2020-07-10 12:41 ` Olivier Matz
2020-07-10 13:10   ` Morten Brørup
2020-07-10 13:16     ` Olivier Matz
2020-07-10 13:29       ` Morten Brørup [this message]
2020-07-10 13:41         ` Olivier Matz
2020-07-10 13:56           ` [dpdk-stable] [dpdk-dev] " Morten Brørup
2020-07-10 14:40             ` Olivier Matz
2020-07-10 14:52               ` Olivier Matz
2020-07-10 21:03               ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35C61119@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=chenchanghu@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=guohongzhi1@huawei.com \
    --cc=haifeng.lin@huawei.com \
    --cc=jerry.lilijun@huawei.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=nicolas.chautru@intel.com \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    --cc=zhoujingbin@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git