| Message ID | 20191018160726.18901-1-jiri@resnulli.us |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] devlink: add format requirement for devlink param names | expand |
On Fri, 18 Oct 2019 18:07:26 +0200, 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. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> Looks good, I could nit pick that length of 0 could also be disallowed since we're checking validity, but why would I :) Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
On Fri, 18 Oct 2019 18:07:26 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > +static bool devlink_param_valid_name(const char *name) > +{ > + int len = strlen(name); > + int i; > + > + /* Name can contain lowercase 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++) { Very minor stuff. 1. since strlen technically returns size_t why not make both i and len type size_t 2. no blank line after comment and before loop?
On Fri, 18 Oct 2019 18:07:26 +0200 Jiri Pirko <jiri@resnulli.us> wrote: > +static bool devlink_param_valid_name(const char *name) > +{ > + int len = strlen(name); > + int i; > + > + /* Name can contain lowercase 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 (islower(name[i]) || isdigit(name[i])) > + continue; > + if (name[i] != '_') > + return false; > + if (i == 0 || i + 1 == len) > + return false; > + if (name[i - 1] == '_') > + return false; > + } > + return true; > +} You might want to also impose a maximum length on name, and not allow slash in name (if you ever plan to use sysfs).
Fri, Oct 18, 2019 at 06:35:09PM CEST, stephen@networkplumber.org wrote: >On Fri, 18 Oct 2019 18:07:26 +0200 >Jiri Pirko <jiri@resnulli.us> wrote: > >> +static bool devlink_param_valid_name(const char *name) >> +{ >> + int len = strlen(name); >> + int i; >> + >> + /* Name can contain lowercase 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 (islower(name[i]) || isdigit(name[i])) >> + continue; >> + if (name[i] != '_') >> + return false; >> + if (i == 0 || i + 1 == len) >> + return false; >> + if (name[i - 1] == '_') >> + return false; >> + } >> + return true; >> +} > >You might want to also impose a maximum length on name, Well I don't really see why. >and not allow slash in name (if you ever plan to use sysfs). They are not allowed. Only islower, isdigit, '_'. That's it.
Fri, Oct 18, 2019 at 06:33:22PM CEST, stephen@networkplumber.org wrote: >On Fri, 18 Oct 2019 18:07:26 +0200 >Jiri Pirko <jiri@resnulli.us> wrote: > >> +static bool devlink_param_valid_name(const char *name) >> +{ >> + int len = strlen(name); >> + int i; >> + >> + /* Name can contain lowercase 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++) { > >Very minor stuff. > 1. since strlen technically returns size_t why not make both i and len type size_t > 2. no blank line after comment and before loop? Ok.
Fri, Oct 18, 2019 at 06:25:50PM CEST, jakub.kicinski@netronome.com wrote: >On Fri, 18 Oct 2019 18:07:26 +0200, 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. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> > >Looks good, I could nit pick that length of 0 could also be disallowed >since we're checking validity, but why would I :) Okay. > >Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
On Fri, Oct 18, 2019 at 06:07:26PM +0200, 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. Hi Jiri Could you add a reference to where these requirements are derived from. What can go wrong if they are ignored? I assume it is something to do with sysfs? Andrew
Fri, Oct 18, 2019 at 07:43:04PM CEST, andrew@lunn.ch wrote: >On Fri, Oct 18, 2019 at 06:07:26PM +0200, 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. > >Hi Jiri > >Could you add a reference to where these requirements are derived >from. What can go wrong if they are ignored? I assume it is something Well, no reference. All existing params, both generic and driver specific are following this format. I just wanted to make that required so all params are looking similar. >to do with sysfs? No, why would you think so? > > Andrew
On Fri, Oct 18, 2019 at 10:08:22PM +0200, Jiri Pirko wrote: > Fri, Oct 18, 2019 at 07:43:04PM CEST, andrew@lunn.ch wrote: > >On Fri, Oct 18, 2019 at 06:07:26PM +0200, 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. > > > >Hi Jiri > > > >Could you add a reference to where these requirements are derived > >from. What can go wrong if they are ignored? I assume it is something > > Well, no reference. All existing params, both generic and driver > specific are following this format. I just wanted to make that required > so all params are looking similar. > > > >to do with sysfs? > > No, why would you think so? I was not expecting it to be totally arbitrary. I thought you would have a real technical reason. Spaces often cause problems, as well as / etc. I've had problems with hwmon device names breaking assumptions in the user space code, etc. I was expecting something like this. I don't really like the all lower case restriction. It makes it hard to be consistent. All Marvell Docs refer to the Address Translation Unit as ATU. I don't think there is any reference to atu. I would prefer to be consistent with the documentation and use ATU. But that is against your arbitrary rules. Andrew
On Fri, 18 Oct 2019 22:27:48 +0200, Andrew Lunn wrote: > I don't really like the all lower case restriction. It makes it hard > to be consistent. All Marvell Docs refer to the Address Translation > Unit as ATU. I don't think there is any reference to atu. I would > prefer to be consistent with the documentation and use ATU. But that > is against your arbitrary rules. So is MTU yet all command line params we take as input or output use lower case. I'm with Jiri.
Fri, Oct 18, 2019 at 10:27:48PM CEST, andrew@lunn.ch wrote: >On Fri, Oct 18, 2019 at 10:08:22PM +0200, Jiri Pirko wrote: >> Fri, Oct 18, 2019 at 07:43:04PM CEST, andrew@lunn.ch wrote: >> >On Fri, Oct 18, 2019 at 06:07:26PM +0200, 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. >> > >> >Hi Jiri >> > >> >Could you add a reference to where these requirements are derived >> >from. What can go wrong if they are ignored? I assume it is something >> >> Well, no reference. All existing params, both generic and driver >> specific are following this format. I just wanted to make that required >> so all params are looking similar. >> >> >> >to do with sysfs? >> >> No, why would you think so? > >I was not expecting it to be totally arbitrary. I thought you would >have a real technical reason. Spaces often cause problems, as well as >/ etc. > >I've had problems with hwmon device names breaking assumptions in the >user space code, etc. I was expecting something like this. > >I don't really like the all lower case restriction. It makes it hard >to be consistent. All Marvell Docs refer to the Address Translation >Unit as ATU. I don't think there is any reference to atu. I would >prefer to be consistent with the documentation and use ATU. But that >is against your arbitrary rules. There are already params with abbrs that could be upper case. So if we do uppercase now, it would be inconsistent. I just want the format to be as simple as possible. lowercase, digits and "_" is very simple and can accomodate everything. > > Andrew
diff --git a/net/core/devlink.c b/net/core/devlink.c index 97e9a2246929..5969cab5bc31 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> @@ -7040,10 +7041,37 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink, } EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister); +static bool devlink_param_valid_name(const char *name) +{ + int len = strlen(name); + int i; + + /* Name can contain lowercase 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 (islower(name[i]) || isdigit(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 int devlink_param_verify(const struct devlink_param *param) { if (!param || !param->name || !param->supported_cmodes) return -EINVAL; + + if (WARN_ON(!devlink_param_valid_name(param->name))) + return -EINVAL; + if (param->generic) return devlink_param_generic_verify(param); else