Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func

[Sabrina Dubroca <sd@queasysnail.net>]

2018-11-28, 14:27:59 -0800, Roopa Prabhu wrote:
> +/* Set/clear flags based on attribute */
> +static void vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
> +			  int attrtype, unsigned long mask)
> +{
> +	unsigned long flags;
> +
> +	if (!tb[attrtype])
> +		return;
> +
> +	if (nla_get_u8(tb[attrtype]))
> +		flags = conf->flags | mask;
> +	else
> +		flags = conf->flags & ~mask;
> +
> +	conf->flags = flags;
> +}
> +
>  static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
>  			 struct net_device *dev, struct vxlan_config *conf,
>  			 bool changelink, struct netlink_ext_ack *extack)
> @@ -3458,45 +3475,27 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
>  	if (data[IFLA_VXLAN_TTL])
>  		conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
>  
> -	if (data[IFLA_VXLAN_TTL_INHERIT])
> -		conf->flags |= VXLAN_F_TTL_INHERIT;
> +	vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
> +		      VXLAN_F_TTL_INHERIT);

IFLA_VXLAN_TTL_INHERIT is a NLA_FLAG. It doesn't have any u8 data to
get. Same for:

	[IFLA_VXLAN_GBP]	= { .type = NLA_FLAG, },
	[IFLA_VXLAN_GPE]	= { .type = NLA_FLAG, },
	[IFLA_VXLAN_REMCSUM_NOPARTIAL]	= { .type = NLA_FLAG },
	[IFLA_VXLAN_TTL_INHERIT]	= { .type = NLA_FLAG },



nit: This patch would have been easier to review if it came first in
the series. Converting:

    if (nla_get_u8(data[IFLA_VXLAN_FOO]))
        conf->flags |= VXLAN_F_FOO;

to this new helper, which in effect does

    if (nla_get_u8(data[IFLA_VXLAN_FOO]))
        conf->flags |= VXLAN_F_FOO;
    else
        conf->flags &= ~VXLAN_F_FOO;

seems to change behavior, but since you're (unless I missed one) only
applying it to flags that couldn't be changed before patch 1, it looks
fine.

-- 
Sabrina