2020-08-27 06:52:06

by Fabian Frédérick

Subject: [PATCH 5/7 net-next] vxlan: add VXLAN_NL2FLAG macro

Replace common flag assignment with a macro.
This could yet be simplified with changelink/supported but it would
remove clarity

Signed-off-by: Fabian Frederick <[email protected]>
---
drivers/net/vxlan.c | 113 +++++---------------------------------------
include/net/vxlan.h | 10 ++++
2 files changed, 23 insertions(+), 100 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1e9ab1002281c..e9b561b9d23e1 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4034,14 +4034,7 @@ 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]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
- VXLAN_F_TTL_INHERIT, changelink, false,
- extack);
- if (err)
- return err;
-
- }
+ VXLAN_NL2FLAG(IFLA_VXLAN_TTL_INHERIT, VXLAN_F_TTL_INHERIT, changelink, false);

if (data[IFLA_VXLAN_LABEL])
conf->label = nla_get_be32(data[IFLA_VXLAN_LABEL]) &
@@ -4061,37 +4054,10 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
if (data[IFLA_VXLAN_AGEING])
conf->age_interval = nla_get_u32(data[IFLA_VXLAN_AGEING]);

- if (data[IFLA_VXLAN_PROXY]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_PROXY,
- VXLAN_F_PROXY, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_RSC]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_RSC,
- VXLAN_F_RSC, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_L2MISS]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L2MISS,
- VXLAN_F_L2MISS, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_L3MISS]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_L3MISS,
- VXLAN_F_L3MISS, changelink, false,
- extack);
- if (err)
- return err;
- }
+ VXLAN_NL2FLAG(IFLA_VXLAN_PROXY, VXLAN_F_PROXY, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_RSC, VXLAN_F_RSC, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_L2MISS, VXLAN_F_L2MISS, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_L3MISS, VXLAN_F_L3MISS, changelink, false);

if (data[IFLA_VXLAN_LIMIT]) {
if (changelink) {
@@ -4102,13 +4068,7 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
conf->addrmax = nla_get_u32(data[IFLA_VXLAN_LIMIT]);
}

- if (data[IFLA_VXLAN_COLLECT_METADATA]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_COLLECT_METADATA,
- VXLAN_F_COLLECT_METADATA, changelink, false,
- extack);
- if (err)
- return err;
- }
+ VXLAN_NL2FLAG(IFLA_VXLAN_COLLECT_METADATA, VXLAN_F_COLLECT_METADATA, changelink, false);

if (data[IFLA_VXLAN_PORT_RANGE]) {
if (!changelink) {
@@ -4142,60 +4102,13 @@ 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]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
- VXLAN_F_UDP_ZERO_CSUM6_TX, changelink,
- false, extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
- VXLAN_F_UDP_ZERO_CSUM6_RX, changelink,
- false, extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_REMCSUM_TX]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_TX,
- VXLAN_F_REMCSUM_TX, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_REMCSUM_RX]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_RX,
- VXLAN_F_REMCSUM_RX, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_GBP]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GBP,
- VXLAN_F_GBP, changelink, false, extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_GPE]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_GPE,
- VXLAN_F_GPE, changelink, false,
- extack);
- if (err)
- return err;
- }
-
- if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
- err = vxlan_nl2flag(conf, data, IFLA_VXLAN_REMCSUM_NOPARTIAL,
- VXLAN_F_REMCSUM_NOPARTIAL, changelink,
- false, extack);
- if (err)
- return err;
- }
+ VXLAN_NL2FLAG(IFLA_VXLAN_UDP_ZERO_CSUM6_TX, VXLAN_F_UDP_ZERO_CSUM6_TX, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_UDP_ZERO_CSUM6_RX, VXLAN_F_UDP_ZERO_CSUM6_RX, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_REMCSUM_TX, IFLA_VXLAN_REMCSUM_TX, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_REMCSUM_RX, VXLAN_F_REMCSUM_RX, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_GBP, VXLAN_F_GBP, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_GPE, VXLAN_F_GPE, changelink, false);
+ VXLAN_NL2FLAG(IFLA_VXLAN_REMCSUM_NOPARTIAL, VXLAN_F_REMCSUM_NOPARTIAL, changelink, false);

if (tb[IFLA_MTU]) {
if (changelink) {
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 3a41627cbdfe5..8a56b7a0f75f9 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -290,6 +290,16 @@ struct vxlan_dev {
VXLAN_F_UDP_ZERO_CSUM6_RX | \
VXLAN_F_COLLECT_METADATA)

+
+#define VXLAN_NL2FLAG(iflag, flag, changelink, changelink_supported) { \
+ if (data[iflag]) { \
+ err = vxlan_nl2flag(conf, data, iflag, flag, changelink, \
+ changelink_supported, extack); \
+ if (err) \
+ return err; \
+ } \
+}
+
struct net_device *vxlan_dev_create(struct net *net, const char *name,
u8 name_assign_type, struct vxlan_config *conf);

--
2.27.0

2020-08-27 09:51:38

by Michal Kubecek

Subject: Re: [PATCH 5/7 net-next] vxlan: add VXLAN_NL2FLAG macro

On Thu, Aug 27, 2020 at 08:50:19AM +0200, Fabian Frederick wrote:
> Replace common flag assignment with a macro.
> This could yet be simplified with changelink/supported but it would
> remove clarity
>
> Signed-off-by: Fabian Frederick <[email protected]>
> ---
[...]
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 3a41627cbdfe5..8a56b7a0f75f9 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -290,6 +290,16 @@ struct vxlan_dev {
> VXLAN_F_UDP_ZERO_CSUM6_RX | \
> VXLAN_F_COLLECT_METADATA)
>
> +
> +#define VXLAN_NL2FLAG(iflag, flag, changelink, changelink_supported) { \
> + if (data[iflag]) { \
> + err = vxlan_nl2flag(conf, data, iflag, flag, changelink, \
> + changelink_supported, extack); \
> + if (err) \
> + return err; \
> + } \
> +}
> +

Hiding a goto or return in a macro is generally discouraged as it may
confuse people reading or updating the code. See e.g. commit
94f826b8076e ("net: fix a potential rcu_read_lock() imbalance in
rt6_fill_node()") for an example of such problem - which was likely the
trigger for removal of NLA_PUT() and related macros shortly after.

Michal

> struct net_device *vxlan_dev_create(struct net *net, const char *name,
> u8 name_assign_type, struct vxlan_config *conf);
>
> --
> 2.27.0
>

2020-08-27 14:33:27

by David Miller

Subject: Re: [PATCH 5/7 net-next] vxlan: add VXLAN_NL2FLAG macro

From: Michal Kubecek <[email protected]>
Date: Thu, 27 Aug 2020 11:50:25 +0200

> On Thu, Aug 27, 2020 at 08:50:19AM +0200, Fabian Frederick wrote:
>> Replace common flag assignment with a macro.
>> This could yet be simplified with changelink/supported but it would
>> remove clarity
>>
>> Signed-off-by: Fabian Frederick <[email protected]>
>> ---
> [...]
>> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
>> index 3a41627cbdfe5..8a56b7a0f75f9 100644
>> --- a/include/net/vxlan.h
>> +++ b/include/net/vxlan.h
>> @@ -290,6 +290,16 @@ struct vxlan_dev {
>> VXLAN_F_UDP_ZERO_CSUM6_RX | \
>> VXLAN_F_COLLECT_METADATA)
>>
>> +
>> +#define VXLAN_NL2FLAG(iflag, flag, changelink, changelink_supported) { \
>> + if (data[iflag]) { \
>> + err = vxlan_nl2flag(conf, data, iflag, flag, changelink, \
>> + changelink_supported, extack); \
>> + if (err) \
>> + return err; \
>> + } \
>> +}
>> +
>
> Hiding a goto or return in a macro is generally discouraged as it may
> confuse people reading or updating the code.

Agreed, please don't do this.