Re: [RFC PATCH net-next v6 2/2] selftests: net: test PPPoE packets in gro.sh

[Willem de Bruijn]

Qingfang Deng wrote:
> Add PPPoE test-cases to the GRO selftest.
> 
> Signed-off-by: Qingfang Deng <dqfext@xxxxxxxxx>
> ---
> v6: new patch
> 
> I'm not sure if I should include all the existing IPv4/v6 tests for
> PPPoE. There are tests that hardcode ETH_HLEN as the NH offset, which
> are meant to test the L3 protocol, not the underlying protocol, and
> changing all of them seems too invasive.

Overall this change looks good to me. But indeed all the churn of

    (pppoe ? PPPOE_SES_HLEN : 0)
 
is a bit annoying. Left a few representative snippets below.
I think that could be avoided similar to MAX_HDR_LEN by having a
global definition and using that everywhere. Still a lot of churn, but
at least no repeated open-coded logic.

Also consider whether running all these tests again in PPPoE adds
value, if not, then indeed trim the tests to those that truly add
code coverage. 

> @@ -745,7 +777,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
>  	memcpy(optpkt + tcp_offset + optlen, buf + tcp_offset,
>  	       sizeof(struct tcphdr) + PAYLOAD_LEN);
>  
> -	iph = (struct iphdr *)(optpkt + ETH_HLEN);
> +	iph = (struct iphdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
>  	iph->ihl = 5 + (optlen / 4);
>  	iph->tot_len = htons(ntohs(iph->tot_len) + optlen);
>  	iph->check = 0;
> @@ -755,7 +787,7 @@ static void add_ipv4_ts_option(void *buf, void *optpkt)
>  static void add_ipv6_exthdr(void *buf, void *optpkt, __u8 exthdr_type, char *ext_payload)
>  {
>  	struct ipv6_opt_hdr *exthdr = (struct ipv6_opt_hdr *)(optpkt + tcp_offset);
> -	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN);
> +	struct ipv6hdr *iph = (struct ipv6hdr *)(optpkt + ETH_HLEN + (pppoe ? PPPOE_SES_HLEN : 0));
>  	char *exthdr_payload_start = (char *)(exthdr + 1);
>  
>  	exthdr->hdrlen = 0;