[patch net-next v3 0/3] devlink: add format requirement for devlink object names

[patch net-next v3 0/3] devlink: add format requirement for devlink object names

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Fist two patches are just small adjustments to fit the format required
by the last patch.

Jiri Pirko (3):
  netdevsim: change name of fib rules resource
  devlink: replace spaces in dpipe field names
  devlink: add format requirement for devlink object names

 drivers/net/netdevsim/dev.c |  4 +-
 net/core/devlink.c          | 85 +++++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 5 deletions(-)

-- 
2.21.0

[patch net-next v3 1/3] netdevsim: change name of fib rules resource

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

To be aligned with upcoming formatting restrictions, change the name of
the resource to "fib_rules".

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- new patch
---
 drivers/net/netdevsim/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 468e157a7cb1..cb908960842b 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -154,7 +154,7 @@ static int nsim_dev_resources_register(struct devlink *devlink)
 		return err;
 	}
 
-	err = devlink_resource_register(devlink, "fib-rules", (u64)-1,
+	err = devlink_resource_register(devlink, "fib_rules", (u64)-1,
 					NSIM_RESOURCE_IPV4_FIB_RULES,
 					NSIM_RESOURCE_IPV4, &params);
 	if (err) {
@@ -180,7 +180,7 @@ static int nsim_dev_resources_register(struct devlink *devlink)
 		return err;
 	}
 
-	err = devlink_resource_register(devlink, "fib-rules", (u64)-1,
+	err = devlink_resource_register(devlink, "fib_rules", (u64)-1,
 					NSIM_RESOURCE_IPV6_FIB_RULES,
 					NSIM_RESOURCE_IPV6, &params);
 	if (err) {
-- 
2.21.0

[patch net-next v3 2/3] devlink: replace spaces in dpipe field names

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

To be aligned with upcoming formatting restrictions, replace spaces
in dpipe filed names to underscores.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- new patch
---
 net/core/devlink.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 97e9a2246929..45b6a9a964f6 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -33,7 +33,7 @@
 
 static struct devlink_dpipe_field devlink_dpipe_fields_ethernet[] = {
 	{
-		.name = "destination mac",
+		.name = "destination_mac",
 		.id = DEVLINK_DPIPE_FIELD_ETHERNET_DST_MAC,
 		.bitwidth = 48,
 	},
@@ -50,7 +50,7 @@ EXPORT_SYMBOL(devlink_dpipe_header_ethernet);
 
 static struct devlink_dpipe_field devlink_dpipe_fields_ipv4[] = {
 	{
-		.name = "destination ip",
+		.name = "destination_ip",
 		.id = DEVLINK_DPIPE_FIELD_IPV4_DST_IP,
 		.bitwidth = 32,
 	},
@@ -67,7 +67,7 @@ EXPORT_SYMBOL(devlink_dpipe_header_ipv4);
 
 static struct devlink_dpipe_field devlink_dpipe_fields_ipv6[] = {
 	{
-		.name = "destination ip",
+		.name = "destination_ip",
 		.id = DEVLINK_DPIPE_FIELD_IPV6_DST_IP,
 		.bitwidth = 128,
 	},
-- 
2.21.0

[patch net-next v3 3/3] devlink: add format requirement for devlink object names

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently, the name format is not required by the code, however it is
required during patch review. All params added until now are in-lined
with the following format:
1) lowercase characters, digits and underscored are allowed
2) underscore is neither at the beginning nor at the end and
   there is no more than one in a row.

Add checker to the code to require this format from drivers and warn if
they don't follow. This applies to params, resources, reporters,
traps, trap groups, dpipe tables and dpipe fields.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- loosen the checks to allow uppercase as well
- also check resources, reporters,
  traps, trap groups, dpipe tables and dpipe fields.
v1->v2:
- removed empty line after comment
- added check for empty string
- converted i and len to size_t and put on a single line
- s/valid_name/name_valid/ to be aligned with the rest
---
 net/core/devlink.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 45b6a9a964f6..e7d714699579 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -20,6 +20,7 @@
 #include <linux/workqueue.h>
 #include <linux/u64_stats_sync.h>
 #include <linux/timekeeping.h>
+#include <linux/ctype.h>
 #include <rdma/ib_verbs.h>
 #include <net/netlink.h>
 #include <net/genetlink.h>
@@ -31,6 +32,30 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/devlink.h>
 
+static bool devlink_obj_name_valid(const char *name)
+{
+	size_t i, len = strlen(name);
+
+	if (!len)
+		return false;
+
+	/* Name can contain lowercase/uppercase characters or digits.
+	 * Underscores are also allowed, but not at the beginning
+	 * or end of the name and not more than one in a row.
+	 */
+	for (i = 0; i < len; i++) {
+		if (isalnum(name[i]))
+			continue;
+		if (name[i] != '_')
+			return false;
+		if (i == 0 || i + 1 == len)
+			return false;
+		if (name[i - 1] == '_')
+			return false;
+	}
+	return true;
+}
+
 static struct devlink_dpipe_field devlink_dpipe_fields_ethernet[] = {
 	{
 		.name = "destination_mac",
@@ -4782,6 +4807,9 @@ devlink_health_reporter_create(struct devlink *devlink,
 		goto unlock;
 	}
 
+	if (WARN_ON(!devlink_obj_name_valid(ops->name)))
+		return ERR_PTR(-EINVAL);
+
 	if (WARN_ON(auto_recover && !ops->recover) ||
 	    WARN_ON(graceful_period && !ops->recover)) {
 		reporter = ERR_PTR(-EINVAL);
@@ -6702,6 +6730,36 @@ void devlink_sb_unregister(struct devlink *devlink, unsigned int sb_index)
 }
 EXPORT_SYMBOL_GPL(devlink_sb_unregister);
 
+static int
+devlink_dpipe_header_verify(struct devlink_dpipe_header *dpipe_header)
+{
+	int i;
+
+	if (WARN_ON(!devlink_obj_name_valid(dpipe_header->name)))
+		return -EINVAL;
+
+	for (i = 0; i < dpipe_header->fields_count; i++) {
+		const char *name = dpipe_header->fields[i].name;
+
+		if (WARN_ON(!devlink_obj_name_valid(name)))
+			return -EINVAL;
+	}
+	return 0;
+}
+
+static int
+devlink_dpipe_headers_verify(struct devlink_dpipe_headers *dpipe_headers)
+{
+	int i, err;
+
+	for (i = 0; i < dpipe_headers->headers_count; i++) {
+		err = devlink_dpipe_header_verify(dpipe_headers->headers[i]);
+		if (err)
+			return err;
+	}
+	return 0;
+}
+
 /**
  *	devlink_dpipe_headers_register - register dpipe headers
  *
@@ -6713,6 +6771,11 @@ EXPORT_SYMBOL_GPL(devlink_sb_unregister);
 int devlink_dpipe_headers_register(struct devlink *devlink,
 				   struct devlink_dpipe_headers *dpipe_headers)
 {
+	int err;
+
+	err = devlink_dpipe_headers_verify(dpipe_headers);
+	if (err)
+		return err;
 	mutex_lock(&devlink->lock);
 	devlink->dpipe_headers = dpipe_headers;
 	mutex_unlock(&devlink->lock);
@@ -6785,6 +6848,9 @@ int devlink_dpipe_table_register(struct devlink *devlink,
 	if (devlink_dpipe_table_find(&devlink->dpipe_table_list, table_name))
 		return -EEXIST;
 
+	if (WARN_ON(!devlink_obj_name_valid(table_name)))
+		return -EINVAL;
+
 	if (WARN_ON(!table_ops->size_get))
 		return -EINVAL;
 
@@ -6853,6 +6919,9 @@ int devlink_resource_register(struct devlink *devlink,
 
 	top_hierarchy = parent_resource_id == DEVLINK_RESOURCE_ID_PARENT_TOP;
 
+	if (WARN_ON(!devlink_obj_name_valid(resource_name)))
+		return -EINVAL;
+
 	mutex_lock(&devlink->lock);
 	resource = devlink_resource_find(devlink, NULL, resource_id);
 	if (resource) {
@@ -7044,6 +7113,10 @@ static int devlink_param_verify(const struct devlink_param *param)
 {
 	if (!param || !param->name || !param->supported_cmodes)
 		return -EINVAL;
+
+	if (WARN_ON(!devlink_obj_name_valid(param->name)))
+		return -EINVAL;
+
 	if (param->generic)
 		return devlink_param_generic_verify(param);
 	else
@@ -7650,6 +7723,9 @@ static int devlink_trap_verify(const struct devlink_trap *trap)
 	if (!trap || !trap->name || !trap->group.name)
 		return -EINVAL;
 
+	if (WARN_ON(!devlink_obj_name_valid(trap->name)))
+		return -EINVAL;
+
 	if (trap->generic)
 		return devlink_trap_generic_verify(trap);
 	else
@@ -7686,6 +7762,9 @@ devlink_trap_group_driver_verify(const struct devlink_trap_group *group)
 
 static int devlink_trap_group_verify(const struct devlink_trap_group *group)
 {
+	if (WARN_ON(!devlink_obj_name_valid(group->name)))
+		return -EINVAL;
+
 	if (group->generic)
 		return devlink_trap_group_generic_verify(group);
 	else
-- 
2.21.0

Re: [patch net-next v3 2/3] devlink: replace spaces in dpipe field names

[David Ahern]

On 10/21/19 8:26 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> To be aligned with upcoming formatting restrictions, replace spaces
> in dpipe filed names to underscores.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v2->v3:
> - new patch
> ---
>  net/core/devlink.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 97e9a2246929..45b6a9a964f6 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -33,7 +33,7 @@
>  
>  static struct devlink_dpipe_field devlink_dpipe_fields_ethernet[] = {
>  	{
> -		.name = "destination mac",
> +		.name = "destination_mac",
>  		.id = DEVLINK_DPIPE_FIELD_ETHERNET_DST_MAC,
>  		.bitwidth = 48,
>  	},
> @@ -50,7 +50,7 @@ EXPORT_SYMBOL(devlink_dpipe_header_ethernet);
>  
>  static struct devlink_dpipe_field devlink_dpipe_fields_ipv4[] = {
>  	{
> -		.name = "destination ip",
> +		.name = "destination_ip",
>  		.id = DEVLINK_DPIPE_FIELD_IPV4_DST_IP,
>  		.bitwidth = 32,
>  	},
> @@ -67,7 +67,7 @@ EXPORT_SYMBOL(devlink_dpipe_header_ipv4);
>  
>  static struct devlink_dpipe_field devlink_dpipe_fields_ipv6[] = {
>  	{
> -		.name = "destination ip",
> +		.name = "destination_ip",
>  		.id = DEVLINK_DPIPE_FIELD_IPV6_DST_IP,
>  		.bitwidth = 128,
>  	},
> 

These are all exposed to userspace, so changing this format breaks
existing users

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[David Ahern]

On 10/21/19 8:26 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Currently, the name format is not required by the code, however it is
> required during patch review. All params added until now are in-lined
> with the following format:
> 1) lowercase characters, digits and underscored are allowed
> 2) underscore is neither at the beginning nor at the end and
>    there is no more than one in a row.
> 
> Add checker to the code to require this format from drivers and warn if
> they don't follow. This applies to params, resources, reporters,
> traps, trap groups, dpipe tables and dpipe fields.
> 

This seems random and without any real cause. There is no reason to
exclude dash and uppercase for example in the names.

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[Andrew Lunn]

On Mon, Oct 21, 2019 at 09:20:07AM -0600, David Ahern wrote:
> On 10/21/19 8:26 AM, Jiri Pirko wrote:
> > From: Jiri Pirko <jiri@mellanox.com>
> > 
> > Currently, the name format is not required by the code, however it is
> > required during patch review. All params added until now are in-lined
> > with the following format:
> > 1) lowercase characters, digits and underscored are allowed
> > 2) underscore is neither at the beginning nor at the end and
> >    there is no more than one in a row.
> > 
> > Add checker to the code to require this format from drivers and warn if
> > they don't follow. This applies to params, resources, reporters,
> > traps, trap groups, dpipe tables and dpipe fields.
> > 
> 
> This seems random and without any real cause. There is no reason to
> exclude dash and uppercase for example in the names.

Hi David

The commit message is out of date. Upper case is now allowed.

    Andrew

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[Jiri Pirko]

Mon, Oct 21, 2019 at 05:20:07PM CEST, dsahern@gmail.com wrote:
>On 10/21/19 8:26 AM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Currently, the name format is not required by the code, however it is
>> required during patch review. All params added until now are in-lined
>> with the following format:
>> 1) lowercase characters, digits and underscored are allowed
>> 2) underscore is neither at the beginning nor at the end and
>>    there is no more than one in a row.
>> 
>> Add checker to the code to require this format from drivers and warn if
>> they don't follow. This applies to params, resources, reporters,
>> traps, trap groups, dpipe tables and dpipe fields.
>> 
>
>This seems random and without any real cause. There is no reason to
>exclude dash and uppercase for example in the names.

I forgot to update the desc. Uppercase chars are now allowed as Andrew
requested. Regarding dash, it could be allowed of course. But why isn't
"_" enough. I mean, I think it would be good to maintain allowed chars
within a limit.
>

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[David Ahern]

On 10/21/19 9:56 AM, Jiri Pirko wrote:
> 
> I forgot to update the desc. Uppercase chars are now allowed as Andrew
> requested. Regarding dash, it could be allowed of course. But why isn't
> "_" enough. I mean, I think it would be good to maintain allowed chars
> within a limit.

That's a personal style question. Is "fib-rules" less readable than
"fib_rules"? Why put such limitations in place if there is no
justifiable reason?

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[Jiri Pirko]

Mon, Oct 21, 2019 at 06:11:33PM CEST, dsahern@gmail.com wrote:
>On 10/21/19 9:56 AM, Jiri Pirko wrote:
>> 
>> I forgot to update the desc. Uppercase chars are now allowed as Andrew
>> requested. Regarding dash, it could be allowed of course. But why isn't
>> "_" enough. I mean, I think it would be good to maintain allowed chars
>> within a limit.
>
>That's a personal style question. Is "fib-rules" less readable than
>"fib_rules"? Why put such limitations in place if there is no
>justifiable reason?

You mean any limitation?

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[David Ahern]

On 10/21/19 11:59 PM, Jiri Pirko wrote:
> Mon, Oct 21, 2019 at 06:11:33PM CEST, dsahern@gmail.com wrote:
>> On 10/21/19 9:56 AM, Jiri Pirko wrote:
>>>
>>> I forgot to update the desc. Uppercase chars are now allowed as Andrew
>>> requested. Regarding dash, it could be allowed of course. But why isn't
>>> "_" enough. I mean, I think it would be good to maintain allowed chars
>>> within a limit.
>>
>> That's a personal style question. Is "fib-rules" less readable than
>> "fib_rules"? Why put such limitations in place if there is no
>> justifiable reason?
> 
> You mean any limitation?
> 

I mean why are you pushing a patch to limit what characters can be used
in names, and why are you deciding '-' is not ok but '_' is?

It just seems so random and not driven by some real limitation (e.g.,
entries in a filesystem).

Re: [patch net-next v3 3/3] devlink: add format requirement for devlink object names

[Jiri Pirko]

Thu, Oct 24, 2019 at 12:16:27AM CEST, dsahern@gmail.com wrote:
>On 10/21/19 11:59 PM, Jiri Pirko wrote:
>> Mon, Oct 21, 2019 at 06:11:33PM CEST, dsahern@gmail.com wrote:
>>> On 10/21/19 9:56 AM, Jiri Pirko wrote:
>>>>
>>>> I forgot to update the desc. Uppercase chars are now allowed as Andrew
>>>> requested. Regarding dash, it could be allowed of course. But why isn't
>>>> "_" enough. I mean, I think it would be good to maintain allowed chars
>>>> within a limit.
>>>
>>> That's a personal style question. Is "fib-rules" less readable than
>>> "fib_rules"? Why put such limitations in place if there is no
>>> justifiable reason?
>> 
>> You mean any limitation?
>> 
>
>I mean why are you pushing a patch to limit what characters can be used
>in names, and why are you deciding '-' is not ok but '_' is?

It is kind of randrom. I just wanted to have some limitations so all
things (params, ...) would look alike for all drivers. That's it.

>
>It just seems so random and not driven by some real limitation (e.g.,
>entries in a filesystem).o

Nothing like that.