* [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 related [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
* [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 related [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 related [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