[PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[Nicolas Dichtel]

As the comment said, this attribute defines the originator of the rule,
it's not really a (network) protocol.
Let's rename it accordingly to avoid confusion (difference between
FRA_PROTOCOL and FRA_IP_PROTO was not obvious).

CC: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

FRA_PROTOCOL exists only in net-next for now, thus it's still possible to
rename it.

 drivers/net/vrf.c              |  4 ++--
 include/net/fib_rules.h        |  4 ++--
 include/uapi/linux/fib_rules.h |  2 +-
 net/core/fib_rules.c           | 14 +++++++-------
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index c6be49d3a9eb..8f559b74404d 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -1146,7 +1146,7 @@ static inline size_t vrf_fib_rule_nl_size(void)
 	sz  = NLMSG_ALIGN(sizeof(struct fib_rule_hdr));
 	sz += nla_total_size(sizeof(u8));	/* FRA_L3MDEV */
 	sz += nla_total_size(sizeof(u32));	/* FRA_PRIORITY */
-	sz += nla_total_size(sizeof(u8));       /* FRA_PROTOCOL */
+	sz += nla_total_size(sizeof(u8));       /* FRA_ORIGINATOR */
 
 	return sz;
 }
@@ -1177,7 +1177,7 @@ static int vrf_fib_rule(const struct net_device *dev, __u8 family, bool add_it)
 	frh->family = family;
 	frh->action = FR_ACT_TO_TBL;
 
-	if (nla_put_u8(skb, FRA_PROTOCOL, RTPROT_KERNEL))
+	if (nla_put_u8(skb, FRA_ORIGINATOR, RTPROT_KERNEL))
 		goto nla_put_failure;
 
 	if (nla_put_u8(skb, FRA_L3MDEV, 1))
diff --git a/include/net/fib_rules.h b/include/net/fib_rules.h
index e5cfcfc7dd93..8ff89beea845 100644
--- a/include/net/fib_rules.h
+++ b/include/net/fib_rules.h
@@ -26,7 +26,7 @@ struct fib_rule {
 	u32			table;
 	u8			action;
 	u8			l3mdev;
-	u8                      proto;
+	u8                      originator;
 	u8			ip_proto;
 	u32			target;
 	__be64			tun_id;
@@ -113,7 +113,7 @@ struct fib_rule_notifier_info {
 	[FRA_GOTO]	= { .type = NLA_U32 }, \
 	[FRA_L3MDEV]	= { .type = NLA_U8 }, \
 	[FRA_UID_RANGE]	= { .len = sizeof(struct fib_rule_uid_range) }, \
-	[FRA_PROTOCOL]  = { .type = NLA_U8 }, \
+	[FRA_ORIGINATOR] = { .type = NLA_U8 }, \
 	[FRA_IP_PROTO]  = { .type = NLA_U8 }, \
 	[FRA_SPORT_RANGE] = { .len = sizeof(struct fib_rule_port_range) }, \
 	[FRA_DPORT_RANGE] = { .len = sizeof(struct fib_rule_port_range) }
diff --git a/include/uapi/linux/fib_rules.h b/include/uapi/linux/fib_rules.h
index 232df14e1287..6bb950a9144b 100644
--- a/include/uapi/linux/fib_rules.h
+++ b/include/uapi/linux/fib_rules.h
@@ -63,7 +63,7 @@ enum {
 	FRA_PAD,
 	FRA_L3MDEV,	/* iif or oif is l3mdev goto its table */
 	FRA_UID_RANGE,	/* UID range */
-	FRA_PROTOCOL,   /* Originator of the rule */
+	FRA_ORIGINATOR, /* Originator of the rule */
 	FRA_IP_PROTO,	/* ip proto */
 	FRA_SPORT_RANGE, /* sport */
 	FRA_DPORT_RANGE, /* dport */
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index f6f04fc0f629..c81dc0600dc4 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -55,7 +55,7 @@ int fib_default_rule_add(struct fib_rules_ops *ops,
 	r->pref = pref;
 	r->table = table;
 	r->flags = flags;
-	r->proto = RTPROT_KERNEL;
+	r->originator = RTPROT_KERNEL;
 	r->fr_net = ops->fro_net;
 	r->uid_range = fib_kuid_range_unset;
 
@@ -505,8 +505,8 @@ int fib_nl_newrule(struct sk_buff *skb, struct nlmsghdr *nlh,
 	rule->pref = tb[FRA_PRIORITY] ? nla_get_u32(tb[FRA_PRIORITY])
 	                              : fib_default_rule_pref(ops);
 
-	rule->proto = tb[FRA_PROTOCOL] ?
-		nla_get_u8(tb[FRA_PROTOCOL]) : RTPROT_UNSPEC;
+	rule->originator = tb[FRA_ORIGINATOR] ?
+		nla_get_u8(tb[FRA_ORIGINATOR]) : RTPROT_UNSPEC;
 
 	if (tb[FRA_IIFNAME]) {
 		struct net_device *dev;
@@ -736,8 +736,8 @@ int fib_nl_delrule(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	list_for_each_entry(rule, &ops->rules_list, list) {
-		if (tb[FRA_PROTOCOL] &&
-		    (rule->proto != nla_get_u8(tb[FRA_PROTOCOL])))
+		if (tb[FRA_ORIGINATOR] &&
+		    (rule->originator != nla_get_u8(tb[FRA_ORIGINATOR])))
 			continue;
 
 		if (frh->action && (frh->action != rule->action))
@@ -870,7 +870,7 @@ static inline size_t fib_rule_nlmsg_size(struct fib_rules_ops *ops,
 			 + nla_total_size(4) /* FRA_FWMASK */
 			 + nla_total_size_64bit(8) /* FRA_TUN_ID */
 			 + nla_total_size(sizeof(struct fib_kuid_range))
-			 + nla_total_size(1) /* FRA_PROTOCOL */
+			 + nla_total_size(1) /* FRA_ORIGINATOR */
 			 + nla_total_size(1) /* FRA_IP_PROTO */
 			 + nla_total_size(sizeof(struct fib_rule_port_range)) /* FRA_SPORT_RANGE */
 			 + nla_total_size(sizeof(struct fib_rule_port_range)); /* FRA_DPORT_RANGE */
@@ -904,7 +904,7 @@ static int fib_nl_fill_rule(struct sk_buff *skb, struct fib_rule *rule,
 	frh->action = rule->action;
 	frh->flags = rule->flags;
 
-	if (nla_put_u8(skb, FRA_PROTOCOL, rule->proto))
+	if (nla_put_u8(skb, FRA_ORIGINATOR, rule->originator))
 		goto nla_put_failure;
 
 	if (rule->action == FR_ACT_GOTO &&
-- 
2.15.1

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[David Ahern]

On 3/20/18 11:04 AM, Nicolas Dichtel wrote:
> As the comment said, this attribute defines the originator of the rule,
> it's not really a (network) protocol.
> Let's rename it accordingly to avoid confusion (difference between
> FRA_PROTOCOL and FRA_IP_PROTO was not obvious).
> 
> CC: Donald Sharp <sharpd@cumulusnetworks.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> FRA_PROTOCOL exists only in net-next for now, thus it's still possible to
> rename it.
> 
>  drivers/net/vrf.c              |  4 ++--
>  include/net/fib_rules.h        |  4 ++--
>  include/uapi/linux/fib_rules.h |  2 +-
>  net/core/fib_rules.c           | 14 +++++++-------
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
>

This protocol is meant to be analogous to rtm_protocol. Changing the
name to FRA_ORIGINATOR loses that connection.

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[Nicolas Dichtel]

Le 20/03/2018 à 18:27, David Ahern a écrit :
> On 3/20/18 11:04 AM, Nicolas Dichtel wrote:
>> As the comment said, this attribute defines the originator of the rule,
>> it's not really a (network) protocol.
>> Let's rename it accordingly to avoid confusion (difference between
>> FRA_PROTOCOL and FRA_IP_PROTO was not obvious).
>>
>> CC: Donald Sharp <sharpd@cumulusnetworks.com>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>
>> FRA_PROTOCOL exists only in net-next for now, thus it's still possible to
>> rename it.
>>
>>  drivers/net/vrf.c              |  4 ++--
>>  include/net/fib_rules.h        |  4 ++--
>>  include/uapi/linux/fib_rules.h |  2 +-
>>  net/core/fib_rules.c           | 14 +++++++-------
>>  4 files changed, 12 insertions(+), 12 deletions(-)
>>
>>
> 
> This protocol is meant to be analogous to rtm_protocol. Changing the
> name to FRA_ORIGINATOR loses that connection.
I understand your concerns. But I think the connection still exists after the
patch because the values used for this field are RTPROT_*
rtm_protocol is here from ages and we cannot change that. Moreover, FRA_*
attributes are usually used as a selector or a target, which is not the case for
a route. Thus I think it's important to carrefully choose the name.


Regards,
Nicolas

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[David Ahern]

On 3/21/18 3:24 AM, Nicolas Dichtel wrote:
> Le 20/03/2018 à 18:27, David Ahern a écrit :
>> On 3/20/18 11:04 AM, Nicolas Dichtel wrote:
>>> As the comment said, this attribute defines the originator of the rule,
>>> it's not really a (network) protocol.
>>> Let's rename it accordingly to avoid confusion (difference between
>>> FRA_PROTOCOL and FRA_IP_PROTO was not obvious).
>>>
>>> CC: Donald Sharp <sharpd@cumulusnetworks.com>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>> ---
>>>
>>> FRA_PROTOCOL exists only in net-next for now, thus it's still possible to
>>> rename it.
>>>
>>>  drivers/net/vrf.c              |  4 ++--
>>>  include/net/fib_rules.h        |  4 ++--
>>>  include/uapi/linux/fib_rules.h |  2 +-
>>>  net/core/fib_rules.c           | 14 +++++++-------
>>>  4 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>>
>>
>> This protocol is meant to be analogous to rtm_protocol. Changing the
>> name to FRA_ORIGINATOR loses that connection.
> I understand your concerns. But I think the connection still exists after the
> patch because the values used for this field are RTPROT_*
> rtm_protocol is here from ages and we cannot change that. Moreover, FRA_*
> attributes are usually used as a selector or a target, which is not the case for
> a route. Thus I think it's important to carrefully choose the name.

The rule->proto value is not used as a selector. It is passed in, stored
on a rule and returned to userspace. It is book keeping only so an admin
has some idea of which program installed the rule.

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Wed, 21 Mar 2018 09:00:09 -0600

> The rule->proto value is not used as a selector. It is passed in, stored
> on a rule and returned to userspace. It is book keeping only so an admin
> has some idea of which program installed the rule.

This is how I understand this value to work as well.

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[Nicolas Dichtel]

Le 21/03/2018 à 16:15, David Miller a écrit :
> From: David Ahern <dsahern@gmail.com>
> Date: Wed, 21 Mar 2018 09:00:09 -0600
> 
>> The rule->proto value is not used as a selector. It is passed in, stored
>> on a rule and returned to userspace. It is book keeping only so an admin
>> has some idea of which program installed the rule.
> 
> This is how I understand this value to work as well.
> 
Me too, and it's the reason why I propose to rename it, to make this clear.
'originator' is a lot more explicit than 'proto'.

Re: [PATCH net-next] fib_rules: rename FRA_PROTOCOL to FRA_ORIGINATOR

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 20 Mar 2018 18:04:53 +0100

> As the comment said, this attribute defines the originator of the rule,
> it's not really a (network) protocol.
> Let's rename it accordingly to avoid confusion (difference between
> FRA_PROTOCOL and FRA_IP_PROTO was not obvious).
> 
> CC: Donald Sharp <sharpd@cumulusnetworks.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> FRA_PROTOCOL exists only in net-next for now, thus it's still possible to
> rename it.

I think by renaming this, and thus losing the connection with how the
term "protocol" is used in our other routing and rule interfaces and
datastructures, this will be creating more confusion than it will be
fixing.

So I won't be applying this.

Thanks.