Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
On Thu, Nov 29, 2018 at 7:55 AM Sabrina Dubroca wrote: > > 2018-11-29, 07:27:17 -0800, Roopa Prabhu wrote: > > On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca > > wrote: > > > 2018-11-28, 14:27:59 -0800, Roopa Prabhu wrote: > > > 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. > > ok. > > > > 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. > > Yeah, looks correct. I just had to go back to patch 1 and check. > A note "only flags [list] are changed, and they could not be modified > after creation of the device prior to patch 1" in the cover and/or in > this patch's commit message would have spared reviewers a bit of a > scare :) ack, sounds good. I am also going to reduce the scope of the series a bit as i am only interested in support for TTL changelink right now.
Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-29, 07:27:17 -0800, Roopa Prabhu wrote: > On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca wrote: > > 2018-11-28, 14:27:59 -0800, Roopa Prabhu wrote: > > 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. ok. > > 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. Yeah, looks correct. I just had to go back to patch 1 and check. A note "only flags [list] are changed, and they could not be modified after creation of the device prior to patch 1" in the cover and/or in this patch's commit message would have spared reviewers a bit of a scare :) -- Sabrina
Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca 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.
Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
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
[PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
From: Roopa Prabhu
Signed-off-by: Roopa Prabhu
---
drivers/net/vxlan.c | 95 -
1 file changed, 43 insertions(+), 52 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 4cb6b50..47671fd 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3374,6 +3374,23 @@ static int __vxlan_dev_create(struct net *net, struct
net_device *dev,
return err;
}
+/* 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);
if (data[IFLA_VXLAN_LABEL])
conf->label = nla_get_be32(data[IFLA_VXLAN_LABEL]) &
IPV6_FLOWLABEL_MASK;
- if (data[IFLA_VXLAN_LEARNING]) {
- if (nla_get_u8(data[IFLA_VXLAN_LEARNING]))
- conf->flags |= VXLAN_F_LEARN;
- else
- conf->flags &= ~VXLAN_F_LEARN;
- } else if (!changelink) {
+ if (data[IFLA_VXLAN_LEARNING])
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_LEARNING,
+ VXLAN_F_LEARN);
+ else if (!changelink)
/* default to learn on a new device */
conf->flags |= VXLAN_F_LEARN;
- }
if (data[IFLA_VXLAN_AGEING])
conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
- if (data[IFLA_VXLAN_PROXY]) {
- if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
- conf->flags |= VXLAN_F_PROXY;
- }
-
- if (data[IFLA_VXLAN_RSC]) {
- if (nla_get_u8(data[IFLA_VXLAN_RSC]))
- conf->flags |= VXLAN_F_RSC;
- }
-
- if (data[IFLA_VXLAN_L2MISS]) {
- if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
- conf->flags |= VXLAN_F_L2MISS;
- }
-
- if (data[IFLA_VXLAN_L3MISS]) {
- if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
- conf->flags |= VXLAN_F_L3MISS;
- }
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_PROXY, VXLAN_F_PROXY);
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_RSC, VXLAN_F_RSC);
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_L2MISS, VXLAN_F_L2MISS);
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_L3MISS, VXLAN_F_L3MISS);
if (data[IFLA_VXLAN_LIMIT]) {
if (changelink) {
@@ -3514,8 +3513,8 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct
nlattr *data[],
"Cannot change metadata flag");
return -EOPNOTSUPP;
}
- if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
- conf->flags |= VXLAN_F_COLLECT_METADATA;
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_COLLECT_METADATA,
+ VXLAN_F_COLLECT_METADATA);
}
if (data[IFLA_VXLAN_PORT_RANGE]) {
@@ -3553,34 +3552,26 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct
nlattr *data[],
conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
}
- if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
- if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
- conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
- }
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
+ VXLAN_F_UDP_ZERO_CSUM6_TX);
- if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
- if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
- conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
- }
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
+ VXLAN_F_UDP_ZERO_CSUM6_RX);
- if (data[IFLA_VXLAN_REMCSUM_TX]) {
- if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
- conf->flags |= VXLAN_F_REMCSUM_TX;
- }
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_TX,
+ VXLAN_F_REMCSUM_TX);
- if (data[IFLA_VXLAN_REMCSUM_RX]) {
- if (nla_get_u8(data[IFLA_V
