On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca <s...@queasysnail.net> wrote:
>
> 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 },
>
>
good catch. i see it, some flags are NLA_U8 and some NLA_FLAG. will fix.
>
> nit: This patch would have been easier to review if it came first in
> the series. Converting:
I considered that. But the first patch really removes some checks
making it easier for the follow-on patches...and that mostly dictated
the order.
>
> 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.
It does not change behavior...the earlier code only supported the
flags during create time and did not support changing of the flag with
changelink.
this patch adds changelink support. But if you do see any change in
behavior, pls report. thanks.
