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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Huichao,
 
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 

...

> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..82c070b 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -12,6 +12,12 @@
> 
>  #include "ip_frag_common.h"
> 
> +/* IP options */
> +#define RTE_IPOPT_EOL				0
> +#define RTE_IPOPT_NOP				1
> +#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
> +#define RTE_IPOPT_MAX_LEN			40
> +
>  /* Fragment Offset */
>  #define	RTE_IPV4_HDR_DF_SHIFT			14
>  #define	RTE_IPV4_HDR_MF_SHIFT			13
> @@ -22,6 +28,8 @@
> 
>  #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
> 
> +#define	RTE_IPV4_HDR_MAX_LEN		60
> +
>  static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
>  		const struct rte_ipv4_hdr *src, uint16_t header_len,
>  		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> @@ -41,6 +49,58 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +static inline void __create_ipopt_frag_hdr(uint8_t *iph,
> +	uint16_t *ipopt_len, uint8_t *ipopt_frag_hdr)
> +{
> +	uint16_t len = *ipopt_len;
> +	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +
> +	*ipopt_len = 0;
> +	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> +	iph_opt->ihl = sizeof(struct rte_ipv4_hdr) / RTE_IPV4_IHL_MULTIPLIER;
> +	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
> +
> +	if (unlikely(len > RTE_IPOPT_MAX_LEN))
> +		return;
> +
> +	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> +
> +	while (len > 0) {
> +		if (unlikely(*p_opt == RTE_IPOPT_NOP)) {
> +			len--;
> +			p_opt++;
> +#ifdef RTE_IPOPT_KEEP_IP_HLEN

Who will define this macro and when?
In general we trying to avoid conditional compilations within DPDK.
Can we always use one way or another?
As you are doing a copy anyway, probably no harm just
completely remove RTE_IPOPT_KEEP_IP_HLEN and related behaviour
and copy only options that need to be copied.
WDYT?


> +			ipopt_frag_hdr[(*ipopt_len)++] = RTE_IPOPT_NOP;
> +#endif
> +			continue;
> +		} else if (unlikely(*p_opt == RTE_IPOPT_EOL))
> +			break;
> +
> +		if (p_opt[1] < 2 || p_opt[1] > len)
> +			break;
> +		if (RTE_IPOPT_COPIED(*p_opt)) {
> +			rte_memcpy(ipopt_frag_hdr + *ipopt_len,
> +				p_opt, p_opt[1]);
> +			*ipopt_len += p_opt[1];
> +#ifdef RTE_IPOPT_KEEP_IP_HLEN
> +		} else {
> +			memset(ipopt_frag_hdr + *ipopt_len,
> +				RTE_IPOPT_NOP, p_opt[1]);
> +			*ipopt_len += p_opt[1];
> +#endif
> +		}
> +
> +		len -= p_opt[1];
> +		p_opt += p_opt[1];
> +	}
> +
> +	len = RTE_ALIGN_CEIL(*ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
> +	memset(ipopt_frag_hdr + *ipopt_len,
> +		RTE_IPOPT_EOL, len - *ipopt_len);
> +	*ipopt_len = len;
> +	iph_opt->ihl += len / RTE_IPV4_IHL_MULTIPLIER;
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -76,6 +136,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	uint32_t more_in_segs;
>  	uint16_t fragment_offset, flag_offset, frag_size, header_len;
>  	uint16_t frag_bytes_remaining;
> +	uint8_t ipopt_frag_hdr[RTE_IPV4_HDR_MAX_LEN];
> +	uint16_t ipopt_len;
> 
>  	/*
>  	 * Formal parameter checking.
> @@ -117,6 +179,7 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	in_seg_data_pos = header_len;
>  	out_pkt_pos = 0;
>  	fragment_offset = 0;
> +	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> 
>  	more_in_segs = 1;
>  	while (likely(more_in_segs)) {
> @@ -188,10 +251,26 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		    (uint16_t)out_pkt->pkt_len,
>  		    flag_offset, fragment_offset, more_in_segs);
> 
> -		fragment_offset = (uint16_t)(fragment_offset +
> -		    out_pkt->pkt_len - header_len);
> +		/* Create a separate IP header to handle frag options. */
> +		if (unlikely((fragment_offset == 0) &&
> +			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) &&
> +			    (ipopt_len))) {
> +			__create_ipopt_frag_hdr((uint8_t *)in_hdr,
> +				&ipopt_len, ipopt_frag_hdr);
> +
> +			fragment_offset = (uint16_t)(fragment_offset +
> +				out_pkt->pkt_len - header_len);
> 
> -		out_pkt->l3_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;
> +		}
> 
>  		/* Write the fragment to the output list */
>  		pkts_out[out_pkt_pos] = out_pkt;
> --
> 1.8.3.1