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

[Thomas Monjalon <thomas@monjalon.net>]

02/02/2023 19:33, Leo Xu (Networking SW):
> > 31/01/2023 07:53, Leo Xu (Networking SW):
> > > From: Thomas Monjalon <thomas@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?

Currently we have this:
struct rte_icmp_hdr {
    uint8_t  icmp_type;     /* ICMP packet type. */
    uint8_t  icmp_code;     /* ICMP packet code. */
    rte_be16_t icmp_cksum;  /* ICMP packet checksum. */
    rte_be16_t icmp_ident;  /* ICMP packet identifier. */
    rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */
} __rte_packed;

I agree we can move some fields in a base struct,
it would change the API.
We could manage with a union, but we would lose the benefit.
It looks like we need to keep rte_icmp_hdr as is.
So we need to duplicate and define new structs.

What about removing the "6" from the new structs,
so it would apply both to IPv4 and IPv6?

struct rte_icmp_base_hdr {
	uint8_t type;
	uint8_t code;
	rte_be16_t checksum;
} __rte_packed;

struct rte_icmp_echo_hdr {
	struct rte_icmp_base_hdr base;
	rte_be16_t identifier;
	rte_be16_t sequence;
} __rte_packed;