Re:RE: [PATCH v3] ip_frag: add IPv4 options fragment and test data

["Huichao Cai" <chcchc88@163.com>]

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

Hi Konstantin,
> These macros are dups for what we have in rte_ipv4_fragmentation.c
> Would probably make sense to name them RTE_IPV4_IPOPT_... and put them 
> in some public .h to avoid duplication.
I named them RTE_IPV4_IPOPT_xxx and put them in "rte_ip_frag.h".


> Could you clarify what this macro does?
> BTW, I assume it is a local one?
> If so, no need for RTE_ prefix.
Yes,it is a local macro.I will cancel the RTE_ prefix.It is a toggle switch used as a different way to assemble frag test data.It is convenient for users to use different ways to assemble test data.
> Instead of returning void and having out parameter (ipopt_len),
> why just not make it a return value?
> static inline uint16_t
> __create_ipopt_frag_hdr(const uint8_t *iph, uint8_t * ipopt_frag_hdr, uint16_t len)
> {
> 	....
> 	return ipopt_len;
> }
> We probably can update ihl once at the very end of this function.
Ok,I will modify it this way,Thank you for your advice.
> Can we probably do that before the loop (as we have to do it only once anyway?
> Something like:

> ....
> ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> if (ipopt_len > RTE_IPOPT_MAX_LEN)
> 	return -EINVAL;
> if (ipopt_len != 0)
> 	ipopt_len = __create_ipopt_frag_hdr((in_hdr, ipopt_frag_hdr, ipopt_len);
> ....

> And then:
> while (likely(more_in_segs)) {
> 	...
> 	if (ipopt_len ! = 0)
> 		in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;	
> }
The modified code is as follows：
ipopt_len = header_len - sizeof(struct rte_ipv4_hdr); if (unlikely(ipopt_len > RTE_IPV4_IPOPT_MAX_LEN)) return -EINVAL; else if (ipopt_len == 0) /* Used to mark without processing frag. */ ipopt_len = RTE_IPV4_IPOPT_MAX_LEN + 1; /* The first frag. */ else if ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) /* Create a separate IP header to handle frag options. */ ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr, ipopt_len, ipopt_frag_hdr);

while (likely(more_in_segs)) {
                ...
		if (unlikely((fragment_offset == 0) &&
			    (ipopt_len <= RTE_IPV4_IPOPT_MAX_LEN) &&
			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
			fragment_offset = (uint16_t)(fragment_offset +
				out_pkt->pkt_len - header_len);
			out_pkt->l3_len = header_len;

			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
		} else {
			fragment_offset = (uint16_t)(fragment_offset +
				out_pkt->pkt_len - header_len);
			out_pkt->l3_len = header_len;
		}



}
These two pieces of code were previously merged together.It doesn't look as brief as before.I would like to hear from you.
Some minor issues：
1. There are some RTE_ prefixes in the rte_ipv4_fragmentation.c.Do I need to move to a public header file?
    /* Fragment Offset */
#define RTE_IPV4_HDR_DF_SHIFT 14 #define RTE_IPV4_HDR_MF_SHIFT 13 #define RTE_IPV4_HDR_FO_SHIFT 3
2. Some comments are in the following format：/**< xxx */,What does this symbol(**<) mean?


Huichao,Cai

[-- Attachment #2: Type: text/html, Size: 9808 bytes --]