Re: [PATCH net-next 1/2] ipv6: ioam: Add support for the ip6ip6 encapsulation

[David Ahern <dsahern@gmail.com>]

On 9/30/21 9:19 AM, Justin Iurman wrote:
>>>  static const struct nla_policy ioam6_iptunnel_policy[IOAM6_IPTUNNEL_MAX + 1] = {
>>> -	[IOAM6_IPTUNNEL_TRACE]	= NLA_POLICY_EXACT_LEN(sizeof(struct ioam6_trace_hdr)),
>>> +	[IOAM6_IPTUNNEL_TRACE]	= NLA_POLICY_EXACT_LEN(sizeof(struct
>>> ioam6_iptunnel_trace)),
>>
>> you can't do that. Once a kernel is released with a given UAPI, it can
>> not be changed. You could go the other way and handle
>>
>> struct ioam6_iptunnel_trace {
>> +	struct ioam6_trace_hdr trace;
>> +	__u8 mode;
>> +	struct in6_addr tundst;	/* unused for inline mode */
>> +};
> 
> Makes sense. But I'm not sure what you mean by "go the other way". Should I handle ioam6_iptunnel_trace as well, in addition to ioam6_trace_hdr, so that the uapi is backward compatible?

by "the other way" I meant let ioam6_trace_hdr be the top element in the
new ioam6_iptunnel_trace struct. If the IOAM6_IPTUNNEL_TRACE size ==
ioam6_trace_hdr then you know it is the legacy argument vs sizeof
ioam6_iptunnel_trace which is the new.

> 
>> Also, no gaps in uapi. Make sure all holes are stated; an anonymous
>> entry is best.
> 
> Would something like this do the trick?
> 
> struct ioam6_iptunnel_trace {
> 	struct ioam6_trace_hdr trace;
> 	__u8 mode;
> 	union { /* anonymous field only used by both the encap and auto modes */
> 		struct in6_addr tundst;
> 	};
> };

By anonymous filling of the holes I meant something like:

struct ioam6_iptunnel_trace {
	struct ioam6_trace_hdr trace;
	__u8 mode;
	__u8 :8;
	__u16 :16;

	struct in6_addr tundst;
};

Use pahole to check that struct for proper alignment of the entries as
desired (4-byte or 8-byte aligned).

> 
>>>  };
>>>  
>>> -static int nla_put_ioam6_trace(struct sk_buff *skb, int attrtype,
>>> -			       struct ioam6_trace_hdr *trace)
>>> -{
>>> -	struct ioam6_trace_hdr *data;
>>> -	struct nlattr *nla;
>>> -	int len;
>>> -
>>> -	len = sizeof(*trace);
>>> -
>>> -	nla = nla_reserve(skb, attrtype, len);
>>> -	if (!nla)
>>> -		return -EMSGSIZE;
>>> -
>>> -	data = nla_data(nla);
>>> -	memcpy(data, trace, len);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>
>> quite a bit of the change seems like refactoring from existing feature
>> to allow the new ones. Please submit refactoring changes as a
>> prerequisite patch. The patch that introduces your new feature should be
>> focused solely on what is needed to implement that feature.
> 
> +1, will do.
>