[PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence

[Leo Xu (Networking SW)]

Hi Thomas

PSB


> 31/01/2023 07:53, Leo Xu (Networking SW):
> > From: Thomas Monjalon <thomas at monjalon.net>
> > > 20/12/2022 08:44, Leo Xu:
> > > > +/**
> > > > + * ICMP6 header
> > > > + */
> > > > +struct rte_icmp6_hdr {
> > > > +     uint8_t type;
> > > > +     uint8_t code;
> > > > +     rte_be16_t checksum;
> > > > +} __rte_packed;
> > > > +
> > > > +/**
> > > > + * ICMP6 echo
> > > > + */
> > > > +struct rte_icmp6_echo {
> > > > +     struct rte_icmp6_hdr hdr;
> > > > +     rte_be16_t identifier;
> > > > +     rte_be16_t sequence;
> > > > +} __rte_packed;
> > >
> > > It is exactly the same as struct rte_icmp_hdr.
> > > Why not reuse it?
> > > Maybe introduce struct rte_icmp_base_hdr and define
> > > rte_icmp_echo_hdr as rte_icmp_hdr?
> >
> > Hi Thomas,
> > Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is
> not that good.
> > since, rte_icmp_hdr default their headers always having id and sequence
> fields, which is not applicable for most other icmp6/icmp types packets.
> >
> > I may suggest to keep icmp and icmp6 structures independent against each
> other, because, looks like these two protocols definitions do not share common
> base.
> 
> What about introducing rte_icmp_base_hdr?
> We should try to avoid duplicating things.
> 

You mean introduce rte_icmp_base_hdr like following?
struct rte_icmp_base_hdr {
	uint8_t  icmp_type;
	uint8_t  icmp_code;
	rte_be16_t icmp_cksum;
} __rte_packed;

And change the existing rte_icmp_hdr to be:
struct rte_icmp_hdr {
	rte_icmp_base_hdr bash_hdr;
	rte_be16_t icmp_ident; 
	rte_be16_t icmp_seq_nb;
} __rte_packed;
#define rte_icmp6_echo struct rte_icmp_hdr;

If it is, there will be some compatibilities issues, since we changed existing structure.

Or, maybe I'm missing something.
Would you help to give more details about above comment?