* [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes
2018-11-28 22:27 [PATCH net-next v2 0/3] vxlan: a few minor cleanups Roopa Prabhu
@ 2018-11-28 22:27 ` Roopa Prabhu
2018-11-29 13:56 ` Sabrina Dubroca
2018-11-28 22:27 ` [PATCH net-next v2 2/3] vxlan: extack support for some changelink cases Roopa Prabhu
2018-11-28 22:27 ` [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func Roopa Prabhu
2 siblings, 1 reply; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-28 22:27 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Roopa Prabhu <roopa@cumulusnetworks.com>
We started very conservative when supporting changelink
especially because not all attribute changes could be
tested. This patch opens up a few more attributes for
changelink. The reason for choosing this set of attributes
is based on code references for these attributes. I have
tested TTL changes and did some changelink api testing
to sanity test the others.
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 36 ++++--------------------------------
1 file changed, 4 insertions(+), 32 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 9110662..73caa65 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3438,11 +3438,8 @@ 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]) {
- if (changelink)
- return -EOPNOTSUPP;
+ if (data[IFLA_VXLAN_TTL_INHERIT])
conf->flags |= VXLAN_F_TTL_INHERIT;
- }
if (data[IFLA_VXLAN_LABEL])
conf->label = nla_get_be32(data[IFLA_VXLAN_LABEL]) &
@@ -3462,29 +3459,21 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
if (data[IFLA_VXLAN_PROXY]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
conf->flags |= VXLAN_F_PROXY;
}
if (data[IFLA_VXLAN_RSC]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_RSC]))
conf->flags |= VXLAN_F_RSC;
}
if (data[IFLA_VXLAN_L2MISS]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
conf->flags |= VXLAN_F_L2MISS;
}
if (data[IFLA_VXLAN_L3MISS]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
conf->flags |= VXLAN_F_L3MISS;
}
@@ -3527,50 +3516,33 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
}
if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
}
if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
}
if (data[IFLA_VXLAN_REMCSUM_TX]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
conf->flags |= VXLAN_F_REMCSUM_TX;
}
if (data[IFLA_VXLAN_REMCSUM_RX]) {
- if (changelink)
- return -EOPNOTSUPP;
if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
conf->flags |= VXLAN_F_REMCSUM_RX;
}
- if (data[IFLA_VXLAN_GBP]) {
- if (changelink)
- return -EOPNOTSUPP;
+ if (data[IFLA_VXLAN_GBP])
conf->flags |= VXLAN_F_GBP;
- }
- if (data[IFLA_VXLAN_GPE]) {
- if (changelink)
- return -EOPNOTSUPP;
+ if (data[IFLA_VXLAN_GPE])
conf->flags |= VXLAN_F_GPE;
- }
- if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
- if (changelink)
- return -EOPNOTSUPP;
+ if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
- }
if (tb[IFLA_MTU]) {
if (changelink)
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes
2018-11-28 22:27 ` [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes Roopa Prabhu
@ 2018-11-29 13:56 ` Sabrina Dubroca
2018-11-29 15:33 ` Roopa Prabhu
0 siblings, 1 reply; 12+ messages in thread
From: Sabrina Dubroca @ 2018-11-29 13:56 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: davem, netdev
2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> We started very conservative when supporting changelink
> especially because not all attribute changes could be
> tested. This patch opens up a few more attributes for
> changelink. The reason for choosing this set of attributes
> is based on code references for these attributes. I have
> tested TTL changes and did some changelink api testing
> to sanity test the others.
>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
> drivers/net/vxlan.c | 36 ++++--------------------------------
> 1 file changed, 4 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 9110662..73caa65 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3438,11 +3438,8 @@ 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]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> + if (data[IFLA_VXLAN_TTL_INHERIT])
> conf->flags |= VXLAN_F_TTL_INHERIT;
> - }
This doesn't give us an option to disable TTL_INHERIT after it was
enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.
> - if (data[IFLA_VXLAN_GBP]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> + if (data[IFLA_VXLAN_GBP])
> conf->flags |= VXLAN_F_GBP;
> - }
>
> - if (data[IFLA_VXLAN_GPE]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> + if (data[IFLA_VXLAN_GPE])
> conf->flags |= VXLAN_F_GPE;
> - }
GPE implies running a different setup function (vxlan_raw_setup() vs
vxlan_ether_setup()), that vxlan_config_apply() only calls for
!changelink. I think this is incomplete.
I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
same socket, I'm not sure how that would work. Normally, they each try
to create a separate socket, and pass the GPE flag on to the
associated vxlan_sock. I suspect that's also a problem with rx
offload.
> - if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> - if (changelink)
> - return -EOPNOTSUPP;
> + if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
> conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> - }
>
> if (tb[IFLA_MTU]) {
> if (changelink)
> --
> 2.1.4
>
--
Sabrina
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes
2018-11-29 13:56 ` Sabrina Dubroca
@ 2018-11-29 15:33 ` Roopa Prabhu
2018-11-29 15:51 ` Sabrina Dubroca
0 siblings, 1 reply; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-29 15:33 UTC (permalink / raw)
To: sd; +Cc: David Miller, netdev
On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> > From: Roopa Prabhu <roopa@cumulusnetworks.com>
> >
> > We started very conservative when supporting changelink
> > especially because not all attribute changes could be
> > tested. This patch opens up a few more attributes for
> > changelink. The reason for choosing this set of attributes
> > is based on code references for these attributes. I have
> > tested TTL changes and did some changelink api testing
> > to sanity test the others.
> >
> > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> > ---
> > drivers/net/vxlan.c | 36 ++++--------------------------------
> > 1 file changed, 4 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 9110662..73caa65 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -3438,11 +3438,8 @@ 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]) {
> > - if (changelink)
> > - return -EOPNOTSUPP;
> > + if (data[IFLA_VXLAN_TTL_INHERIT])
> > conf->flags |= VXLAN_F_TTL_INHERIT;
> > - }
>
> This doesn't give us an option to disable TTL_INHERIT after it was
> enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.
that is provided by patch3, with the changelink patch. I can squash
them, if thats easier.
>
> > - if (data[IFLA_VXLAN_GBP]) {
> > - if (changelink)
> > - return -EOPNOTSUPP;
> > + if (data[IFLA_VXLAN_GBP])
> > conf->flags |= VXLAN_F_GBP;
> > - }
> >
> > - if (data[IFLA_VXLAN_GPE]) {
> > - if (changelink)
> > - return -EOPNOTSUPP;
> > + if (data[IFLA_VXLAN_GPE])
> > conf->flags |= VXLAN_F_GPE;
> > - }
>
> GPE implies running a different setup function (vxlan_raw_setup() vs
> vxlan_ether_setup()), that vxlan_config_apply() only calls for
> !changelink. I think this is incomplete.
>
> I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
> same socket, I'm not sure how that would work. Normally, they each try
> to create a separate socket, and pass the GPE flag on to the
> associated vxlan_sock. I suspect that's also a problem with rx
> offload.
that is good to know. I will drop the change to GPE and also the rx
offload flag and let somebody else using it
open it up for changelink. thanks for the review
> > - if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> > - if (changelink)
> > - return -EOPNOTSUPP;
> > + if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
> > conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> > - }
> >
> > if (tb[IFLA_MTU]) {
> > if (changelink)
> > --
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes
2018-11-29 15:33 ` Roopa Prabhu
@ 2018-11-29 15:51 ` Sabrina Dubroca
0 siblings, 0 replies; 12+ messages in thread
From: Sabrina Dubroca @ 2018-11-29 15:51 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: David Miller, netdev
2018-11-29, 07:33:11 -0800, Roopa Prabhu wrote:
> On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> > > From: Roopa Prabhu <roopa@cumulusnetworks.com>
> > >
> > > We started very conservative when supporting changelink
> > > especially because not all attribute changes could be
> > > tested. This patch opens up a few more attributes for
> > > changelink. The reason for choosing this set of attributes
> > > is based on code references for these attributes. I have
> > > tested TTL changes and did some changelink api testing
> > > to sanity test the others.
> > >
> > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> > > ---
> > > drivers/net/vxlan.c | 36 ++++--------------------------------
> > > 1 file changed, 4 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > > index 9110662..73caa65 100644
> > > --- a/drivers/net/vxlan.c
> > > +++ b/drivers/net/vxlan.c
> > > @@ -3438,11 +3438,8 @@ 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]) {
> > > - if (changelink)
> > > - return -EOPNOTSUPP;
> > > + if (data[IFLA_VXLAN_TTL_INHERIT])
> > > conf->flags |= VXLAN_F_TTL_INHERIT;
> > > - }
> >
> > This doesn't give us an option to disable TTL_INHERIT after it was
> > enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.
>
> that is provided by patch3, with the changelink patch. I can squash
> them, if thats easier.
Yes, I see it now. Sequential review got me :(
I think you can just add a note in the commit message, "will be fixed
in a further patch introducing the vxlan_nl2flag() helper" or similar.
> > > - if (data[IFLA_VXLAN_GBP]) {
> > > - if (changelink)
> > > - return -EOPNOTSUPP;
> > > + if (data[IFLA_VXLAN_GBP])
> > > conf->flags |= VXLAN_F_GBP;
> > > - }
> > >
> > > - if (data[IFLA_VXLAN_GPE]) {
> > > - if (changelink)
> > > - return -EOPNOTSUPP;
> > > + if (data[IFLA_VXLAN_GPE])
> > > conf->flags |= VXLAN_F_GPE;
> > > - }
> >
> > GPE implies running a different setup function (vxlan_raw_setup() vs
> > vxlan_ether_setup()), that vxlan_config_apply() only calls for
> > !changelink. I think this is incomplete.
> >
> > I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
> > same socket, I'm not sure how that would work. Normally, they each try
> > to create a separate socket, and pass the GPE flag on to the
> > associated vxlan_sock. I suspect that's also a problem with rx
> > offload.
>
> that is good to know. I will drop the change to GPE and also the rx
> offload flag and let somebody else using it
> open it up for changelink. thanks for the review
Looking at vxlan_rcv(), there's also a possible issue with the GBP
flag:
if (vs->flags & VXLAN_F_GBP)
vxlan_parse_gbp_hdr(&unparsed, skb, vs->flags, md);
I don't know why that flag must match on all tunnels for a socket, but
given that it was the reason for commit ac5132d1a03f ("vxlan: Only
bind to sockets with compatible flags enabled"), I'd leave it alone as
well.
> > > - if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> > > - if (changelink)
> > > - return -EOPNOTSUPP;
> > > + if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
> > > conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> > > - }
> > >
> > > if (tb[IFLA_MTU]) {
> > > if (changelink)
> > > --
--
Sabrina
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v2 2/3] vxlan: extack support for some changelink cases
2018-11-28 22:27 [PATCH net-next v2 0/3] vxlan: a few minor cleanups Roopa Prabhu
2018-11-28 22:27 ` [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes Roopa Prabhu
@ 2018-11-28 22:27 ` Roopa Prabhu
2018-12-01 0:31 ` kbuild test robot
2018-11-28 22:27 ` [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func Roopa Prabhu
2 siblings, 1 reply; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-28 22:27 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 76 +++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 73caa65..4cb6b50 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3376,7 +3376,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
struct net_device *dev, struct vxlan_config *conf,
- bool changelink)
+ bool changelink, struct netlink_ext_ack *extack)
{
struct vxlan_dev *vxlan = netdev_priv(dev);
@@ -3389,40 +3389,60 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
if (data[IFLA_VXLAN_ID]) {
__be32 vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
- if (changelink && (vni != conf->vni))
+ if (changelink && (vni != conf->vni)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID],
+ "Cannot change vni");
return -EOPNOTSUPP;
+ }
conf->vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
}
if (data[IFLA_VXLAN_GROUP]) {
- if (changelink && (conf->remote_ip.sa.sa_family != AF_INET))
+ if (changelink && (conf->remote_ip.sa.sa_family != AF_INET)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP],
+ "New group addr family does not match old group");
return -EOPNOTSUPP;
-
+ }
conf->remote_ip.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_GROUP]);
conf->remote_ip.sa.sa_family = AF_INET;
} else if (data[IFLA_VXLAN_GROUP6]) {
- if (!IS_ENABLED(CONFIG_IPV6))
+ if (!IS_ENABLED(CONFIG_IPV6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6],
+ "IPv6 support not enabled in the kernel");
return -EPFNOSUPPORT;
+ }
- if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6))
+ if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6],
+ "New group addr family does not match old group");
return -EOPNOTSUPP;
+ }
conf->remote_ip.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_GROUP6]);
conf->remote_ip.sa.sa_family = AF_INET6;
}
if (data[IFLA_VXLAN_LOCAL]) {
- if (changelink && (conf->saddr.sa.sa_family != AF_INET))
+ if (changelink && (conf->saddr.sa.sa_family != AF_INET)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL],
+ "New local addr family does not match old");
return -EOPNOTSUPP;
+ }
conf->saddr.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_LOCAL]);
conf->saddr.sa.sa_family = AF_INET;
} else if (data[IFLA_VXLAN_LOCAL6]) {
- if (!IS_ENABLED(CONFIG_IPV6))
+ if (!IS_ENABLED(CONFIG_IPV6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6],
+ "IPv6 support not enabled in the kernel");
return -EPFNOSUPPORT;
+ }
- if (changelink && (conf->saddr.sa.sa_family != AF_INET6))
+ if (changelink && (conf->saddr.sa.sa_family != AF_INET6)) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6],
+ "New local6 addr family does not match old");
return -EOPNOTSUPP;
+ }
/* TODO: respect scope id */
conf->saddr.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_LOCAL6]);
@@ -3479,14 +3499,21 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
}
if (data[IFLA_VXLAN_LIMIT]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LIMIT],
+ "Cannot change limit");
return -EOPNOTSUPP;
+ }
conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
}
if (data[IFLA_VXLAN_COLLECT_METADATA]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack,
+ tb[IFLA_VXLAN_COLLECT_METADATA],
+ "Cannot change metadata flag");
return -EOPNOTSUPP;
+ }
if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
conf->flags |= VXLAN_F_COLLECT_METADATA;
}
@@ -3497,20 +3524,31 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
= nla_data(data[IFLA_VXLAN_PORT_RANGE]);
conf->port_min = ntohs(p->low);
conf->port_max = ntohs(p->high);
- } else {
+ else {
+ NL_SET_ERR_MSG_ATTR(extack,
+ tb[IFLA_VXLAN_PORT_RANGE],
+ "Cannot change port range");
return -EOPNOTSUPP;
}
}
if (data[IFLA_VXLAN_PORT]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack,
+ tb[IFLA_VXLAN_PORT],
+ "Cannot change port");
return -EOPNOTSUPP;
+ }
conf->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
}
if (data[IFLA_VXLAN_UDP_CSUM]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack,
+ tb[IFLA_VXLAN_UDP_CSUM],
+ "Cannot change udp csum");
return -EOPNOTSUPP;
+ }
if (!nla_get_u8(data[IFLA_VXLAN_UDP_CSUM]))
conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
}
@@ -3545,8 +3583,12 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
if (tb[IFLA_MTU]) {
- if (changelink)
+ if (changelink) {
+ NL_SET_ERR_MSG_ATTR(extack,
+ tb[IFLA_MTU],
+ "Cannot change mtu");
return -EOPNOTSUPP;
+ }
conf->mtu = nla_get_u32(tb[IFLA_MTU]);
}
@@ -3563,7 +3605,7 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
struct vxlan_config conf;
int err;
- err = vxlan_nl2conf(tb, data, dev, &conf, false);
+ err = vxlan_nl2conf(tb, data, dev, &conf, false, extack);
if (err)
return err;
@@ -3583,7 +3625,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
int err;
err = vxlan_nl2conf(tb, data,
- dev, &conf, true);
+ dev, &conf, true, extack);
if (err)
return err;
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 2/3] vxlan: extack support for some changelink cases
2018-11-28 22:27 ` [PATCH net-next v2 2/3] vxlan: extack support for some changelink cases Roopa Prabhu
@ 2018-12-01 0:31 ` kbuild test robot
0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-12-01 0:31 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: kbuild-all, davem, netdev
[-- Attachment #1: Type: text/plain, Size: 8462 bytes --]
Hi Roopa,
I love your patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Roopa-Prabhu/vxlan-a-few-minor-cleanups/20181130-184658
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net/vxlan.c: In function 'vxlan_nl2conf':
>> drivers/net/vxlan.c:3527:3: error: expected '}' before 'else'
else {
^~~~
vim +3527 drivers/net/vxlan.c
3376
3377 static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
3378 struct net_device *dev, struct vxlan_config *conf,
3379 bool changelink, struct netlink_ext_ack *extack)
3380 {
3381 struct vxlan_dev *vxlan = netdev_priv(dev);
3382
3383 memset(conf, 0, sizeof(*conf));
3384
3385 /* if changelink operation, start with old existing cfg */
3386 if (changelink)
3387 memcpy(conf, &vxlan->cfg, sizeof(*conf));
3388
3389 if (data[IFLA_VXLAN_ID]) {
3390 __be32 vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
3391
3392 if (changelink && (vni != conf->vni)) {
3393 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_ID],
3394 "Cannot change vni");
3395 return -EOPNOTSUPP;
3396 }
3397 conf->vni = cpu_to_be32(nla_get_u32(data[IFLA_VXLAN_ID]));
3398 }
3399
3400 if (data[IFLA_VXLAN_GROUP]) {
3401 if (changelink && (conf->remote_ip.sa.sa_family != AF_INET)) {
3402 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP],
3403 "New group addr family does not match old group");
3404 return -EOPNOTSUPP;
3405 }
3406 conf->remote_ip.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_GROUP]);
3407 conf->remote_ip.sa.sa_family = AF_INET;
3408 } else if (data[IFLA_VXLAN_GROUP6]) {
3409 if (!IS_ENABLED(CONFIG_IPV6)) {
3410 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6],
3411 "IPv6 support not enabled in the kernel");
3412 return -EPFNOSUPPORT;
3413 }
3414
3415 if (changelink && (conf->remote_ip.sa.sa_family != AF_INET6)) {
3416 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_GROUP6],
3417 "New group addr family does not match old group");
3418 return -EOPNOTSUPP;
3419 }
3420
3421 conf->remote_ip.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_GROUP6]);
3422 conf->remote_ip.sa.sa_family = AF_INET6;
3423 }
3424
3425 if (data[IFLA_VXLAN_LOCAL]) {
3426 if (changelink && (conf->saddr.sa.sa_family != AF_INET)) {
3427 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL],
3428 "New local addr family does not match old");
3429 return -EOPNOTSUPP;
3430 }
3431
3432 conf->saddr.sin.sin_addr.s_addr = nla_get_in_addr(data[IFLA_VXLAN_LOCAL]);
3433 conf->saddr.sa.sa_family = AF_INET;
3434 } else if (data[IFLA_VXLAN_LOCAL6]) {
3435 if (!IS_ENABLED(CONFIG_IPV6)) {
3436 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6],
3437 "IPv6 support not enabled in the kernel");
3438 return -EPFNOSUPPORT;
3439 }
3440
3441 if (changelink && (conf->saddr.sa.sa_family != AF_INET6)) {
3442 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LOCAL6],
3443 "New local6 addr family does not match old");
3444 return -EOPNOTSUPP;
3445 }
3446
3447 /* TODO: respect scope id */
3448 conf->saddr.sin6.sin6_addr = nla_get_in6_addr(data[IFLA_VXLAN_LOCAL6]);
3449 conf->saddr.sa.sa_family = AF_INET6;
3450 }
3451
3452 if (data[IFLA_VXLAN_LINK])
3453 conf->remote_ifindex = nla_get_u32(data[IFLA_VXLAN_LINK]);
3454
3455 if (data[IFLA_VXLAN_TOS])
3456 conf->tos = nla_get_u8(data[IFLA_VXLAN_TOS]);
3457
3458 if (data[IFLA_VXLAN_TTL])
3459 conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
3460
3461 if (data[IFLA_VXLAN_TTL_INHERIT])
3462 conf->flags |= VXLAN_F_TTL_INHERIT;
3463
3464 if (data[IFLA_VXLAN_LABEL])
3465 conf->label = nla_get_be32(data[IFLA_VXLAN_LABEL]) &
3466 IPV6_FLOWLABEL_MASK;
3467
3468 if (data[IFLA_VXLAN_LEARNING]) {
3469 if (nla_get_u8(data[IFLA_VXLAN_LEARNING]))
3470 conf->flags |= VXLAN_F_LEARN;
3471 else
3472 conf->flags &= ~VXLAN_F_LEARN;
3473 } else if (!changelink) {
3474 /* default to learn on a new device */
3475 conf->flags |= VXLAN_F_LEARN;
3476 }
3477
3478 if (data[IFLA_VXLAN_AGEING])
3479 conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);
3480
3481 if (data[IFLA_VXLAN_PROXY]) {
3482 if (nla_get_u8(data[IFLA_VXLAN_PROXY]))
3483 conf->flags |= VXLAN_F_PROXY;
3484 }
3485
3486 if (data[IFLA_VXLAN_RSC]) {
3487 if (nla_get_u8(data[IFLA_VXLAN_RSC]))
3488 conf->flags |= VXLAN_F_RSC;
3489 }
3490
3491 if (data[IFLA_VXLAN_L2MISS]) {
3492 if (nla_get_u8(data[IFLA_VXLAN_L2MISS]))
3493 conf->flags |= VXLAN_F_L2MISS;
3494 }
3495
3496 if (data[IFLA_VXLAN_L3MISS]) {
3497 if (nla_get_u8(data[IFLA_VXLAN_L3MISS]))
3498 conf->flags |= VXLAN_F_L3MISS;
3499 }
3500
3501 if (data[IFLA_VXLAN_LIMIT]) {
3502 if (changelink) {
3503 NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_VXLAN_LIMIT],
3504 "Cannot change limit");
3505 return -EOPNOTSUPP;
3506 }
3507 conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
3508 }
3509
3510 if (data[IFLA_VXLAN_COLLECT_METADATA]) {
3511 if (changelink) {
3512 NL_SET_ERR_MSG_ATTR(extack,
3513 tb[IFLA_VXLAN_COLLECT_METADATA],
3514 "Cannot change metadata flag");
3515 return -EOPNOTSUPP;
3516 }
3517 if (nla_get_u8(data[IFLA_VXLAN_COLLECT_METADATA]))
3518 conf->flags |= VXLAN_F_COLLECT_METADATA;
3519 }
3520
3521 if (data[IFLA_VXLAN_PORT_RANGE]) {
3522 if (!changelink) {
3523 const struct ifla_vxlan_port_range *p
3524 = nla_data(data[IFLA_VXLAN_PORT_RANGE]);
3525 conf->port_min = ntohs(p->low);
3526 conf->port_max = ntohs(p->high);
> 3527 else {
3528 NL_SET_ERR_MSG_ATTR(extack,
3529 tb[IFLA_VXLAN_PORT_RANGE],
3530 "Cannot change port range");
3531 return -EOPNOTSUPP;
3532 }
3533 }
3534
3535 if (data[IFLA_VXLAN_PORT]) {
3536 if (changelink) {
3537 NL_SET_ERR_MSG_ATTR(extack,
3538 tb[IFLA_VXLAN_PORT],
3539 "Cannot change port");
3540 return -EOPNOTSUPP;
3541 }
3542 conf->dst_port = nla_get_be16(data[IFLA_VXLAN_PORT]);
3543 }
3544
3545 if (data[IFLA_VXLAN_UDP_CSUM]) {
3546 if (changelink) {
3547 NL_SET_ERR_MSG_ATTR(extack,
3548 tb[IFLA_VXLAN_UDP_CSUM],
3549 "Cannot change udp csum");
3550 return -EOPNOTSUPP;
3551 }
3552 if (!nla_get_u8(data[IFLA_VXLAN_UDP_CSUM]))
3553 conf->flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
3554 }
3555
3556 if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
3557 if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
3558 conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
3559 }
3560
3561 if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
3562 if (nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
3563 conf->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
3564 }
3565
3566 if (data[IFLA_VXLAN_REMCSUM_TX]) {
3567 if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
3568 conf->flags |= VXLAN_F_REMCSUM_TX;
3569 }
3570
3571 if (data[IFLA_VXLAN_REMCSUM_RX]) {
3572 if (nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
3573 conf->flags |= VXLAN_F_REMCSUM_RX;
3574 }
3575
3576 if (data[IFLA_VXLAN_GBP])
3577 conf->flags |= VXLAN_F_GBP;
3578
3579 if (data[IFLA_VXLAN_GPE])
3580 conf->flags |= VXLAN_F_GPE;
3581
3582 if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
3583 conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
3584
3585 if (tb[IFLA_MTU]) {
3586 if (changelink) {
3587 NL_SET_ERR_MSG_ATTR(extack,
3588 tb[IFLA_MTU],
3589 "Cannot change mtu");
3590 return -EOPNOTSUPP;
3591 }
3592 conf->mtu = nla_get_u32(tb[IFLA_MTU]);
3593 }
3594
3595 if (data[IFLA_VXLAN_DF])
3596 conf->df = nla_get_u8(data[IFLA_VXLAN_DF]);
3597
3598 return 0;
3599 }
3600
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66023 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-28 22:27 [PATCH net-next v2 0/3] vxlan: a few minor cleanups Roopa Prabhu
2018-11-28 22:27 ` [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes Roopa Prabhu
2018-11-28 22:27 ` [PATCH net-next v2 2/3] vxlan: extack support for some changelink cases Roopa Prabhu
@ 2018-11-28 22:27 ` Roopa Prabhu
2018-11-29 14:19 ` Sabrina Dubroca
2 siblings, 1 reply; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-28 22:27 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
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_VXLAN_REMCSUM_RX]))
- conf->flags |= VXLAN_F_REMCSUM_RX;
- }
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_RX,
+ VXLAN_F_REMCSUM_RX);
- if (data[IFLA_VXLAN_GBP])
- conf->flags |= VXLAN_F_GBP;
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_GBP,
+ VXLAN_F_GBP);
- if (data[IFLA_VXLAN_GPE])
- conf->flags |= VXLAN_F_GPE;
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_GPE,
+ VXLAN_F_GPE);
- if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
- conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
+ vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_NOPARTIAL,
+ VXLAN_F_REMCSUM_NOPARTIAL);
if (tb[IFLA_MTU]) {
if (changelink) {
--
2.1.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-28 22:27 ` [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func Roopa Prabhu
@ 2018-11-29 14:19 ` Sabrina Dubroca
2018-11-29 15:27 ` Roopa Prabhu
0 siblings, 1 reply; 12+ messages in thread
From: Sabrina Dubroca @ 2018-11-29 14:19 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: davem, netdev
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-29 14:19 ` Sabrina Dubroca
@ 2018-11-29 15:27 ` Roopa Prabhu
2018-11-29 15:55 ` Sabrina Dubroca
0 siblings, 1 reply; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-29 15:27 UTC (permalink / raw)
To: sd; +Cc: David Miller, netdev
On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca <sd@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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-29 15:27 ` Roopa Prabhu
@ 2018-11-29 15:55 ` Sabrina Dubroca
2018-11-29 16:05 ` Roopa Prabhu
0 siblings, 1 reply; 12+ messages in thread
From: Sabrina Dubroca @ 2018-11-29 15:55 UTC (permalink / raw)
To: Roopa Prabhu; +Cc: David Miller, netdev
2018-11-29, 07:27:17 -0800, Roopa Prabhu wrote:
> On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca <sd@queasysnail.net> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func
2018-11-29 15:55 ` Sabrina Dubroca
@ 2018-11-29 16:05 ` Roopa Prabhu
0 siblings, 0 replies; 12+ messages in thread
From: Roopa Prabhu @ 2018-11-29 16:05 UTC (permalink / raw)
To: sd; +Cc: David Miller, netdev
On Thu, Nov 29, 2018 at 7:55 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2018-11-29, 07:27:17 -0800, Roopa Prabhu wrote:
> > On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca <sd@queasysnail.net> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread