Re: [PATCH iproute2 v2] iproute: lwtunnel set TUNNEL_KEY on LWTUNNEL_IP_FLAGS

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 20 Dec 2018 08:17:27 +0800
wenxu@ucloud.cn wrote:

> From: wenxu <wenxu@ucloud.cn>
> 
> ip l add dev tun type gretap external
> ip r a 10.0.0.1 encap ip dst 192.168.152.171 id 1000 dev gretap
> 
> For gretap example when the command set the id but don't set the
> TUNNEL_KEY flags. There is no key field in the send packet
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  include/uapi/linux/if_tunnel.h |   20 ++++++++++++++++++++
>  ip/iproute_lwtunnel.c          |   11 ++++++++++-
>  2 files changed, 30 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/linux/if_tunnel.h b/include/uapi/linux/if_tunnel.h
> index ecdc766..c7f0a5e 100644
> --- a/include/uapi/linux/if_tunnel.h
> +++ b/include/uapi/linux/if_tunnel.h

Already picked up header from kernel update

> diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
> index aee18ac..c659035 100644
> --- a/ip/iproute_lwtunnel.c
> +++ b/ip/iproute_lwtunnel.c
> @@ -18,6 +18,7 @@
>  #include <string.h>
>  #include <linux/ila.h>
>  #include <linux/lwtunnel.h>
> +#include <linux/if_tunnel.h>
>  #include <linux/mpls_iptunnel.h>
>  #include <errno.h>
>  
> @@ -31,7 +32,6 @@
>  #include <linux/seg6_iptunnel.h>
>  #include <linux/seg6_hmac.h>
>  #include <linux/seg6_local.h>
> -#include <net/if.h>
>  
>  static const char *format_encap_type(int type)
>  {
> @@ -780,12 +780,14 @@ static int parse_encap_ip(struct rtattr *rta, size_t len,
>  	char **argv = *argvp;
>  	int argc = *argcp;
>  	int ret = 0;
> +	__u16 flags = 0;
>  
>  	while (argc > 0) {
>  		if (strcmp(*argv, "id") == 0) {
>  			__u64 id;
>  
>  			NEXT_ARG();
> +			flags |= TUNNEL_KEY;

How is this backwards compatible with older kernels? You are always setting
flag. The design of the kernel attribute may be broken and have to be
reverted.

Old iproute2 has to work on 4.21 and new iproute2 has to work on old kernels
(even 2.6.32).


A bigger problem is you added a way to set the option without a corresponding way
to display the current settings. You need to look for and decode the netlink
attribute. And once again the API has to work backwards and forwards.