Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces
@ 2019-11-05 21:17 Jakub Kicinski
  2019-11-05 21:17 ` [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID Jakub Kicinski
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Jakub Kicinski @ 2019-11-05 21:17 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, oss-drivers, jiri, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID
  2019-11-05 21:17 [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces Jakub Kicinski
@ 2019-11-05 21:17 ` Jakub Kicinski
  2019-11-06  8:51   ` Jiri Pirko
  2019-11-05 21:17 ` [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required Jakub Kicinski
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2019-11-05 21:17 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, oss-drivers, jiri, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required
  2019-11-05 21:17 [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces Jakub Kicinski
  2019-11-05 21:17 ` [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID Jakub Kicinski
@ 2019-11-05 21:17 ` Jakub Kicinski
  2019-11-06  8:52   ` Jiri Pirko
  2019-11-05 21:17 ` [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes Jakub Kicinski
  2019-11-09  0:40 ` [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces David Ahern
  3 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2019-11-05 21:17 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, oss-drivers, jiri, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes
  2019-11-05 21:17 [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces Jakub Kicinski
  2019-11-05 21:17 ` [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID Jakub Kicinski
  2019-11-05 21:17 ` [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required Jakub Kicinski
@ 2019-11-05 21:17 ` Jakub Kicinski
  2019-11-06  8:53   ` Jiri Pirko
  2019-11-09  0:40 ` [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces David Ahern
  3 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2019-11-05 21:17 UTC (permalink / raw)
  To: dsahern; +Cc: stephen, netdev, oss-drivers, jiri, 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID
  2019-11-05 21:17 ` [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID Jakub Kicinski
@ 2019-11-06  8:51   ` Jiri Pirko
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2019-11-06  8:51 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: dsahern, stephen, netdev, oss-drivers

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required
  2019-11-05 21:17 ` [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required Jakub Kicinski
@ 2019-11-06  8:52   ` Jiri Pirko
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2019-11-06  8:52 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: dsahern, stephen, netdev, oss-drivers

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes
  2019-11-05 21:17 ` [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes Jakub Kicinski
@ 2019-11-06  8:53   ` Jiri Pirko
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Pirko @ 2019-11-06  8:53 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: dsahern, stephen, netdev, oss-drivers

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces
  2019-11-05 21:17 [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces Jakub Kicinski
                   ` (2 preceding siblings ...)
  2019-11-05 21:17 ` [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes Jakub Kicinski
@ 2019-11-09  0:40 ` David Ahern
  3 siblings, 0 replies; 8+ messages in thread
From: David Ahern @ 2019-11-09  0:40 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: stephen, netdev, oss-drivers, jiri

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,

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 21:17 [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces Jakub Kicinski
2019-11-05 21:17 ` [PATCH iproute2-next 1/3] devlink: fix referencing namespace by PID Jakub Kicinski
2019-11-06  8:51   ` Jiri Pirko
2019-11-05 21:17 ` [PATCH iproute2-next 2/3] devlink: catch missing strings in dl_args_required Jakub Kicinski
2019-11-06  8:52   ` Jiri Pirko
2019-11-05 21:17 ` [PATCH iproute2-next 3/3] devlink: allow full range of resource sizes Jakub Kicinski
2019-11-06  8:53   ` Jiri Pirko
2019-11-09  0:40 ` [PATCH iproute2-next 0/3] devlink: improve parameter checking, resources and namespaces David Ahern

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git