[PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces

[PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces

[Jakub Kicinski]

Hi!

This set brings small fixes/improvements for devlink tool.
First the netns reference by PID is fixed (code is in -next only).
Next an extra check is added to catch bugs where new feature forgot
to add required parameters to the error string table.
Last but not least allow full range of resource sizes.

These changes are required to fix kernel's devlink.sh test.

Jakub Kicinski (3):
  devlink: fix referencing namespace by PID
  devlink: catch missing strings in dl_args_required
  devlink: allow full range of resource sizes

 devlink/devlink.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID

[Jakub Kicinski]

netns parameter for devlink reload is supposed to take PID
as well as string name. However, the PID parsing has two
bugs:
 - the opts->netns member is unsigned so the < 0
   condition is always false;
 - the parameter list is not rewinded after parsing as
   a name, so parsing as a pid uses the wrong argument.

Fixes: 08e8e1ca3e05 ("devlink: extend reload command to add support for network namespace change")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 9c96d05ea666..682f832a064c 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -345,6 +345,12 @@ static void dl_arg_inc(struct dl *dl)
 	dl->argv++;
 }
 
+static void dl_arg_dec(struct dl *dl)
+{
+	dl->argc++;
+	dl->argv--;
+}
+
 static char *dl_argv_next(struct dl *dl)
 {
 	char *ret;
@@ -1460,7 +1466,8 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 			if (err)
 				return err;
 			opts->netns = netns_get_fd(netns_str);
-			if (opts->netns < 0) {
+			if ((int)opts->netns < 0) {
+				dl_arg_dec(dl);
 				err = dl_argv_uint32_t(dl, &opts->netns);
 				if (err)
 					return err;
-- 
2.23.0

[PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required

[Jakub Kicinski]

Currently if dl_args_required doesn't contain a string
for a given option the fact that the option is missing
is silently ignored.

Add a catch-all case and print a generic error.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 682f832a064c..e05a2336787a 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1157,6 +1157,10 @@ static int dl_args_finding_required_validate(uint64_t o_required,
 			return -EINVAL;
 		}
 	}
+	if (o_required & ~o_found) {
+		pr_err("BUG: unknown argument required but not found\n");
+		return -EINVAL;
+	}
 	return 0;
 }
 
-- 
2.23.0

[PATCH iproute2-next 3/3] devlink: allow full range of resource sizes

[Jakub Kicinski]

Resource size is a 64 bit attribute at netlink level.
Make the command line argument 64 bit as well.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index e05a2336787a..ea3f992ee0d7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -283,7 +283,7 @@ struct dl_opts {
 	bool dpipe_counters_enable;
 	bool eswitch_encap_mode;
 	const char *resource_path;
-	uint32_t resource_size;
+	uint64_t resource_size;
 	uint32_t resource_id;
 	bool resource_id_valid;
 	const char *param_name;
@@ -1348,7 +1348,7 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
 		} else if (dl_argv_match(dl, "size") &&
 			   (o_all & DL_OPT_RESOURCE_SIZE)) {
 			dl_arg_inc(dl);
-			err = dl_argv_uint32_t(dl, &opts->resource_size);
+			err = dl_argv_uint64_t(dl, &opts->resource_size);
 			if (err)
 				return err;
 			o_found |= DL_OPT_RESOURCE_SIZE;
-- 
2.23.0

Re: [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID

[Jiri Pirko]

Tue, Nov 05, 2019 at 10:17:05PM CET, jakub.kicinski@netronome.com wrote:
>netns parameter for devlink reload is supposed to take PID
>as well as string name. However, the PID parsing has two
>bugs:
> - the opts->netns member is unsigned so the < 0
>   condition is always false;
> - the parameter list is not rewinded after parsing as
>   a name, so parsing as a pid uses the wrong argument.
>
>Fixes: 08e8e1ca3e05 ("devlink: extend reload command to add support for network namespace change")
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Thanks for fixing this.

Re: [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required

[Jiri Pirko]

Tue, Nov 05, 2019 at 10:17:06PM CET, jakub.kicinski@netronome.com wrote:
>Currently if dl_args_required doesn't contain a string
>for a given option the fact that the option is missing
>is silently ignored.
>
>Add a catch-all case and print a generic error.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes

[Jiri Pirko]

Tue, Nov 05, 2019 at 10:17:07PM CET, jakub.kicinski@netronome.com wrote:
>Resource size is a 64 bit attribute at netlink level.
>Make the command line argument 64 bit as well.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

This can be:
Fixes: 8cd644095842 ("devlink: Add support for devlink resource abstraction")

Anyway:
Acked-by: Jiri Pirko <jiri@mellanox.com>

Thanks!

>---
> devlink/devlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index e05a2336787a..ea3f992ee0d7 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -283,7 +283,7 @@ struct dl_opts {
> 	bool dpipe_counters_enable;
> 	bool eswitch_encap_mode;
> 	const char *resource_path;
>-	uint32_t resource_size;
>+	uint64_t resource_size;
> 	uint32_t resource_id;
> 	bool resource_id_valid;
> 	const char *param_name;
>@@ -1348,7 +1348,7 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 		} else if (dl_argv_match(dl, "size") &&
> 			   (o_all & DL_OPT_RESOURCE_SIZE)) {
> 			dl_arg_inc(dl);
>-			err = dl_argv_uint32_t(dl, &opts->resource_size);
>+			err = dl_argv_uint64_t(dl, &opts->resource_size);
> 			if (err)
> 				return err;
> 			o_found |= DL_OPT_RESOURCE_SIZE;
>-- 
>2.23.0
>

Re: [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces

[David Ahern]

On 11/5/19 2:17 PM, Jakub Kicinski wrote:
> Hi!
> 
> This set brings small fixes/improvements for devlink tool.
> First the netns reference by PID is fixed (code is in -next only).
> Next an extra check is added to catch bugs where new feature forgot
> to add required parameters to the error string table.
> Last but not least allow full range of resource sizes.
> 
> These changes are required to fix kernel's devlink.sh test.
> 
> Jakub Kicinski (3):
>   devlink: fix referencing namespace by PID
>   devlink: catch missing strings in dl_args_required
>   devlink: allow full range of resource sizes
> 
>  devlink/devlink.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 

Applied to iproute2-next. Thanks,