Re: [PATCH v3 09/12] l2tp: Add netlink control API for L2TP

[Patrick McHardy <kaber@trash.net>]

James Chapman wrote:
> +static struct nla_policy l2tp_nl_policy[L2TP_ATTR_MAX + 1] = {
> +	[L2TP_ATTR_NONE]		= { .type = NLA_UNSPEC, },
> +	[L2TP_ATTR_PW_TYPE]		= { .type = NLA_U16, },
> +	[L2TP_ATTR_ENCAP_TYPE]		= { .type = NLA_U16, },
> +	[L2TP_ATTR_OFFSET]		= { .type = NLA_U16, },
> +	[L2TP_ATTR_DATA_SEQ]		= { .type = NLA_U8, },
> +	[L2TP_ATTR_L2SPEC_TYPE]		= { .type = NLA_U8, },
> +	[L2TP_ATTR_L2SPEC_LEN]		= { .type = NLA_U8, },
> +	[L2TP_ATTR_PROTO_VERSION]	= { .type = NLA_U8, },
> +	[L2TP_ATTR_CONN_ID]		= { .type = NLA_U32, },
> +	[L2TP_ATTR_PEER_CONN_ID]	= { .type = NLA_U32, },
> +	[L2TP_ATTR_SESSION_ID]		= { .type = NLA_U32, },
> +	[L2TP_ATTR_PEER_SESSION_ID]	= { .type = NLA_U32, },
> +	[L2TP_ATTR_UDP_CSUM]		= { .type = NLA_FLAG, },
> +	[L2TP_ATTR_VLAN_ID]		= { .type = NLA_U16, },
> +	[L2TP_ATTR_DEBUG]		= { .type = NLA_U32, },
> +	[L2TP_ATTR_RECV_SEQ]		= { .type = NLA_FLAG, },
> +	[L2TP_ATTR_SEND_SEQ]		= { .type = NLA_FLAG, },
> +	[L2TP_ATTR_LNS_MODE]		= { .type = NLA_FLAG, },
> +	[L2TP_ATTR_USING_IPSEC]		= { .type = NLA_FLAG, },

Please don't use NLA_FLAG, it diverges from the usual netlink
attribute semantics since you either can't unset the flag in
case you check for the presence of the attribute (the attribute
can only encode "flag set") or you'll have to send the current
value in every message even if you don't intend to change it.
In this case it can't be unset.

A better way is to use something like this:

struct l2tp_flags {
	__u32	value;
	__u32	mask;
};

and set mask to the bits you intend to change.