[patch iproute2-next v2 0/5] expose devlink instances relationships

[patch iproute2-next v2 0/5] expose devlink instances relationships

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Print out recently added attributes that expose relationships between
devlink instances. This patchset extends the outputs by
"nested_devlink" and "nested_devlink_netns" attributes.

Examples:
$ devlink dev
pci/0000:08:00.0: nested_devlink auxiliary/mlx5_core.eth.0
pci/0000:08:00.1: nested_devlink auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.0

$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 106
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
$ devlink port function set pci/0000:08:00.0/32768 state active
$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state active opstate attached roce enable nested_devlink auxiliary/mlx5_core.sf.2

# devlink dev reload auxiliary/mlx5_core.sf.2 netns ns1
$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state active opstate attached roce enable nested_devlink auxiliary/mlx5_core.sf.2 nested_devlink_netns ns1

---
v1->v2:
- patch #2 was added
- patch #3 uses new helper added by patch #2, typo is fixed

Jiri Pirko (5):
  devlink: update headers
  ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c
  devlink: introduce support for netns id for nested handle
  devlink: print nested handle for port function
  devlink: print nested devlink handle for devlink dev

 devlink/devlink.c            | 108 +++++++++++++++++++++++++++++++++--
 include/namespace.h          |   4 ++
 include/uapi/linux/devlink.h |   1 +
 ip/ipnetns.c                 |  45 +--------------
 lib/namespace.c              |  49 ++++++++++++++++
 5 files changed, 159 insertions(+), 48 deletions(-)

-- 
2.41.0

[patch iproute2-next v2 1/5] devlink: update headers

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Update the devlink headers to recent net-next.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/uapi/linux/devlink.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 8b9b98e75059..6c4721270910 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -680,6 +680,7 @@ enum devlink_port_function_attr {
 	DEVLINK_PORT_FN_ATTR_STATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_OPSTATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_CAPS,	/* bitfield32 */
+	DEVLINK_PORT_FN_ATTR_DEVLINK,	/* nested */
 
 	__DEVLINK_PORT_FUNCTION_ATTR_MAX,
 	DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1
-- 
2.41.0

[patch iproute2-next v2 2/5] ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In order to be able to reuse get_netnsid_from_name() function outside of
ip code, move the internals to lib/namespace.c to a new function called
netns_netnsid_from_name().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- new patch
---
 include/namespace.h |  4 ++++
 ip/ipnetns.c        | 45 +----------------------------------------
 lib/namespace.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 44 deletions(-)

diff --git a/include/namespace.h b/include/namespace.h
index e47f9b5d49d1..f564a574d200 100644
--- a/include/namespace.h
+++ b/include/namespace.h
@@ -8,6 +8,8 @@
 #include <sys/syscall.h>
 #include <errno.h>
 
+#include "namespace.h"
+
 #ifndef NETNS_RUN_DIR
 #define NETNS_RUN_DIR "/var/run/netns"
 #endif
@@ -58,4 +60,6 @@ struct netns_func {
 	void *arg;
 };
 
+int netns_netnsid_from_name(struct rtnl_handle *rtnl, const char *name);
+
 #endif /* __NAMESPACE_H__ */
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 9d996832aef8..642873432a4b 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -105,52 +105,9 @@ static int ipnetns_have_nsid(void)
 
 int get_netnsid_from_name(const char *name)
 {
-	struct {
-		struct nlmsghdr n;
-		struct rtgenmsg g;
-		char            buf[1024];
-	} req = {
-		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)),
-		.n.nlmsg_flags = NLM_F_REQUEST,
-		.n.nlmsg_type = RTM_GETNSID,
-		.g.rtgen_family = AF_UNSPEC,
-	};
-	struct nlmsghdr *answer;
-	struct rtattr *tb[NETNSA_MAX + 1];
-	struct rtgenmsg *rthdr;
-	int len, fd, ret = -1;
-
 	netns_nsid_socket_init();
 
-	fd = netns_get_fd(name);
-	if (fd < 0)
-		return fd;
-
-	addattr32(&req.n, 1024, NETNSA_FD, fd);
-	if (rtnl_talk(&rtnsh, &req.n, &answer) < 0) {
-		close(fd);
-		return -2;
-	}
-	close(fd);
-
-	/* Validate message and parse attributes */
-	if (answer->nlmsg_type == NLMSG_ERROR)
-		goto out;
-
-	rthdr = NLMSG_DATA(answer);
-	len = answer->nlmsg_len - NLMSG_SPACE(sizeof(*rthdr));
-	if (len < 0)
-		goto out;
-
-	parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len);
-
-	if (tb[NETNSA_NSID]) {
-		ret = rta_getattr_s32(tb[NETNSA_NSID]);
-	}
-
-out:
-	free(answer);
-	return ret;
+	return netns_netnsid_from_name(&rtnsh, name);
 }
 
 struct nsid_cache {
diff --git a/lib/namespace.c b/lib/namespace.c
index 1202fa85f97d..39981d835aa5 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -7,9 +7,11 @@
 #include <fcntl.h>
 #include <dirent.h>
 #include <limits.h>
+#include <linux/net_namespace.h>
 
 #include "utils.h"
 #include "namespace.h"
+#include "libnetlink.h"
 
 static void bind_etc(const char *name)
 {
@@ -139,3 +141,50 @@ int netns_foreach(int (*func)(char *nsname, void *arg), void *arg)
 	closedir(dir);
 	return 0;
 }
+
+int netns_netnsid_from_name(struct rtnl_handle *rtnl, const char *name)
+{
+	struct {
+		struct nlmsghdr n;
+		struct rtgenmsg g;
+		char            buf[1024];
+	} req = {
+		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg)),
+		.n.nlmsg_flags = NLM_F_REQUEST,
+		.n.nlmsg_type = RTM_GETNSID,
+		.g.rtgen_family = AF_UNSPEC,
+	};
+	struct nlmsghdr *answer;
+	struct rtattr *tb[NETNSA_MAX + 1];
+	struct rtgenmsg *rthdr;
+	int len, fd, ret = -1;
+
+	fd = netns_get_fd(name);
+	if (fd < 0)
+		return fd;
+
+	addattr32(&req.n, 1024, NETNSA_FD, fd);
+	if (rtnl_talk(rtnl, &req.n, &answer) < 0) {
+		close(fd);
+		return -2;
+	}
+	close(fd);
+
+	/* Validate message and parse attributes */
+	if (answer->nlmsg_type == NLMSG_ERROR)
+		goto out;
+
+	rthdr = NLMSG_DATA(answer);
+	len = answer->nlmsg_len - NLMSG_SPACE(sizeof(*rthdr));
+	if (len < 0)
+		goto out;
+
+	parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len);
+
+	if (tb[NETNSA_NSID])
+		ret = rta_getattr_s32(tb[NETNSA_NSID]);
+
+out:
+	free(answer);
+	return ret;
+}
-- 
2.41.0

Re: [patch iproute2-next v2 2/5] ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c

[David Ahern]

On 9/19/23 5:56 AM, Jiri Pirko wrote:
> diff --git a/lib/namespace.c b/lib/namespace.c
> index 1202fa85f97d..39981d835aa5 100644
> --- a/lib/namespace.c
> +++ b/lib/namespace.c
> @@ -7,9 +7,11 @@
>  #include <fcntl.h>
>  #include <dirent.h>
>  #include <limits.h>
> +#include <linux/net_namespace.h>
>  
>  #include "utils.h"
>  #include "namespace.h"
> +#include "libnetlink.h"
>  
>  static void bind_etc(const char *name)
>  {
> @@ -139,3 +141,50 @@ int netns_foreach(int (*func)(char *nsname, void *arg), void *arg)
>  	closedir(dir);
>  	return 0;
>  }
> +
> +int netns_netnsid_from_name(struct rtnl_handle *rtnl, const char *name)

just netns_id_from_name or netns_name_to_id. See comment in next patch ....

Re: [patch iproute2-next v2 2/5] ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c

[Jiri Pirko]

Tue, Sep 19, 2023 at 04:03:26PM CEST, dsahern@gmail.com wrote:
>On 9/19/23 5:56 AM, Jiri Pirko wrote:
>> diff --git a/lib/namespace.c b/lib/namespace.c
>> index 1202fa85f97d..39981d835aa5 100644
>> --- a/lib/namespace.c
>> +++ b/lib/namespace.c
>> @@ -7,9 +7,11 @@
>>  #include <fcntl.h>
>>  #include <dirent.h>
>>  #include <limits.h>
>> +#include <linux/net_namespace.h>
>>  
>>  #include "utils.h"
>>  #include "namespace.h"
>> +#include "libnetlink.h"
>>  
>>  static void bind_etc(const char *name)
>>  {
>> @@ -139,3 +141,50 @@ int netns_foreach(int (*func)(char *nsname, void *arg), void *arg)
>>  	closedir(dir);
>>  	return 0;
>>  }
>> +
>> +int netns_netnsid_from_name(struct rtnl_handle *rtnl, const char *name)
>
>just netns_id_from_name or netns_name_to_id. See comment in next patch ....

Sure.

>

[patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Nested handle may contain DEVLINK_ATTR_NETNS_ID attribute that indicates
the network namespace where the nested devlink instance resides. Process
this converting to netns name if possible and print to user.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- use previously introduced netns_netnsid_from_name() instead of code
  duplication for the same function.
- s/nesns_name_by_id_func/netns_name_by_id_func/
---
 devlink/devlink.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index d1795f616ca0..cf5d466bfc9d 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -24,6 +24,7 @@
 #include <linux/genetlink.h>
 #include <linux/devlink.h>
 #include <linux/netlink.h>
+#include <linux/net_namespace.h>
 #include <libmnl/libmnl.h>
 #include <netinet/ether.h>
 #include <sys/select.h>
@@ -722,6 +723,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES] = MNL_TYPE_NESTED,
 	[DEVLINK_ATTR_NESTED_DEVLINK] = MNL_TYPE_NESTED,
 	[DEVLINK_ATTR_SELFTESTS] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_NETNS_ID] = MNL_TYPE_U32,
 };
 
 static const enum mnl_attr_data_type
@@ -2723,6 +2725,40 @@ static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
 	       !cmp_arr_last_handle(dl, bus_name, dev_name);
 }
 
+struct netns_name_by_id_ctx {
+	int32_t id;
+	char *name;
+	struct rtnl_handle *rth;
+};
+
+static int netns_name_by_id_func(char *nsname, void *arg)
+{
+	struct netns_name_by_id_ctx *ctx = arg;
+	int32_t ret;
+
+	ret = netns_netnsid_from_name(ctx->rth, nsname);
+	if (ret < 0 || ret != ctx->id)
+		return 0;
+	ctx->name = strdup(nsname);
+	return 1;
+}
+
+static char *netns_name_by_id(int32_t id)
+{
+	struct rtnl_handle rth;
+	struct netns_name_by_id_ctx ctx = {
+		.id = id,
+		.rth = &rth,
+	};
+
+	if (rtnl_open(&rth, 0) < 0)
+		return NULL;
+	netns_foreach(netns_name_by_id_func, &ctx);
+	rtnl_close(&rth);
+
+	return ctx.name;
+}
+
 static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
@@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
 	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
 		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
 	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
+
+	if (tb[DEVLINK_ATTR_NETNS_ID]) {
+		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
+
+		if (id >= 0) {
+			char *name = netns_name_by_id(id);
+
+			if (name) {
+				print_string(PRINT_ANY,
+					     "nested_devlink_netns",
+					     " nested_devlink_netns %s", name);
+				free(name);
+			} else {
+				print_int(PRINT_ANY,
+					  "nested_devlink_netnsid",
+					  " nested_devlink_netnsid %d", id);
+			}
+		} else {
+			print_string(PRINT_FP, NULL,
+				     " nested_devlink_netnsid %s", "unknown");
+			print_int(PRINT_JSON,
+				  "nested_devlink_netnsid", NULL, id);
+		}
+	}
 }
 
 static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
-- 
2.41.0

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[David Ahern]

On 9/19/23 5:56 AM, Jiri Pirko wrote:
> @@ -2723,6 +2725,40 @@ static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
>  	       !cmp_arr_last_handle(dl, bus_name, dev_name);
>  }
>  
> +struct netns_name_by_id_ctx {
> +	int32_t id;
> +	char *name;
> +	struct rtnl_handle *rth;
> +};
> +
> +static int netns_name_by_id_func(char *nsname, void *arg)
> +{
> +	struct netns_name_by_id_ctx *ctx = arg;
> +	int32_t ret;
> +
> +	ret = netns_netnsid_from_name(ctx->rth, nsname);
> +	if (ret < 0 || ret != ctx->id)
> +		return 0;
> +	ctx->name = strdup(nsname);
> +	return 1;
> +}
> +
> +static char *netns_name_by_id(int32_t id)
> +{
> +	struct rtnl_handle rth;
> +	struct netns_name_by_id_ctx ctx = {
> +		.id = id,
> +		.rth = &rth,
> +	};
> +
> +	if (rtnl_open(&rth, 0) < 0)
> +		return NULL;
> +	netns_foreach(netns_name_by_id_func, &ctx);
> +	rtnl_close(&rth);
> +
> +	return ctx.name;
> +}
> +

The above is not devlink specific, so it should go in lib/namespace.c as
well.

Name wise it should be consistent with the last patch, so either
netns_id_to_name or netns_name_from_id based on the name from the
refactoring in patch 2.


>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>  {
>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
> +
> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
> +
> +		if (id >= 0) {
> +			char *name = netns_name_by_id(id);
> +
> +			if (name) {
> +				print_string(PRINT_ANY,
> +					     "nested_devlink_netns",
> +					     " nested_devlink_netns %s", name);
> +				free(name);
> +			} else {
> +				print_int(PRINT_ANY,
> +					  "nested_devlink_netnsid",
> +					  " nested_devlink_netnsid %d", id);
> +			}
> +		} else {
> +			print_string(PRINT_FP, NULL,
> +				     " nested_devlink_netnsid %s", "unknown");
> +			print_int(PRINT_JSON,
> +				  "nested_devlink_netnsid", NULL, id);
> +		}

Also, devlink in the name here provides no addititional value (devlink
is the command name) and why add 'nested'? The attribute is just
NETNS_ID, so why not just 'netnsid' here.


> +	}
>  }
>  
>  static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Tue, Sep 19, 2023 at 04:03:27PM CEST, dsahern@gmail.com wrote:
>On 9/19/23 5:56 AM, Jiri Pirko wrote:
>> @@ -2723,6 +2725,40 @@ static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
>>  	       !cmp_arr_last_handle(dl, bus_name, dev_name);
>>  }
>>  
>> +struct netns_name_by_id_ctx {
>> +	int32_t id;
>> +	char *name;
>> +	struct rtnl_handle *rth;
>> +};
>> +
>> +static int netns_name_by_id_func(char *nsname, void *arg)
>> +{
>> +	struct netns_name_by_id_ctx *ctx = arg;
>> +	int32_t ret;
>> +
>> +	ret = netns_netnsid_from_name(ctx->rth, nsname);
>> +	if (ret < 0 || ret != ctx->id)
>> +		return 0;
>> +	ctx->name = strdup(nsname);
>> +	return 1;
>> +}
>> +
>> +static char *netns_name_by_id(int32_t id)
>> +{
>> +	struct rtnl_handle rth;
>> +	struct netns_name_by_id_ctx ctx = {
>> +		.id = id,
>> +		.rth = &rth,
>> +	};
>> +
>> +	if (rtnl_open(&rth, 0) < 0)
>> +		return NULL;
>> +	netns_foreach(netns_name_by_id_func, &ctx);
>> +	rtnl_close(&rth);
>> +
>> +	return ctx.name;
>> +}
>> +
>
>The above is not devlink specific, so it should go in lib/namespace.c as
>well.
>
>Name wise it should be consistent with the last patch, so either
>netns_id_to_name or netns_name_from_id based on the name from the
>refactoring in patch 2.

Okay.

>
>
>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>  {
>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>> +
>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>> +
>> +		if (id >= 0) {
>> +			char *name = netns_name_by_id(id);
>> +
>> +			if (name) {
>> +				print_string(PRINT_ANY,
>> +					     "nested_devlink_netns",
>> +					     " nested_devlink_netns %s", name);
>> +				free(name);
>> +			} else {
>> +				print_int(PRINT_ANY,
>> +					  "nested_devlink_netnsid",
>> +					  " nested_devlink_netnsid %d", id);
>> +			}
>> +		} else {
>> +			print_string(PRINT_FP, NULL,
>> +				     " nested_devlink_netnsid %s", "unknown");
>> +			print_int(PRINT_JSON,
>> +				  "nested_devlink_netnsid", NULL, id);
>> +		}
>
>Also, devlink in the name here provides no addititional value (devlink
>is the command name) and why add 'nested'? The attribute is just
>NETNS_ID, so why not just 'netnsid' here.

Well, it is a netnsid of the nested devlink instance, not the object
(e.g. port) itself. Omitting that would be misleading. Any idea how to
do this differently?


>
>
>> +	}
>>  }
>>  
>>  static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
>

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[David Ahern]

On 9/19/23 11:19 AM, Jiri Pirko wrote:
>>
>>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>  {
>>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>>> +
>>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>>> +
>>> +		if (id >= 0) {
>>> +			char *name = netns_name_by_id(id);
>>> +
>>> +			if (name) {
>>> +				print_string(PRINT_ANY,
>>> +					     "nested_devlink_netns",
>>> +					     " nested_devlink_netns %s", name);
>>> +				free(name);
>>> +			} else {
>>> +				print_int(PRINT_ANY,
>>> +					  "nested_devlink_netnsid",
>>> +					  " nested_devlink_netnsid %d", id);
>>> +			}
>>> +		} else {
>>> +			print_string(PRINT_FP, NULL,
>>> +				     " nested_devlink_netnsid %s", "unknown");
>>> +			print_int(PRINT_JSON,
>>> +				  "nested_devlink_netnsid", NULL, id);
>>> +		}
>> Also, devlink in the name here provides no addititional value (devlink
>> is the command name) and why add 'nested'? The attribute is just
>> NETNS_ID, so why not just 'netnsid' here.
> Well, it is a netnsid of the nested devlink instance, not the object
> (e.g. port) itself. Omitting that would be misleading. Any idea how to
> do this differently?
> 
> 

The attribute is a namespace id, and the value is a namespace id. Given
that, the name here should be netnsid (or nsid - we did a horrible job
with consistency across iproute2 commands). I have not followed the
kernel patches to understand what you mean by nested devlink instance.

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Tue, Sep 19, 2023 at 08:48:29PM CEST, dsahern@gmail.com wrote:
>On 9/19/23 11:19 AM, Jiri Pirko wrote:
>>>
>>>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>  {
>>>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>>>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>>>> +
>>>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>>>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>>>> +
>>>> +		if (id >= 0) {
>>>> +			char *name = netns_name_by_id(id);
>>>> +
>>>> +			if (name) {
>>>> +				print_string(PRINT_ANY,
>>>> +					     "nested_devlink_netns",
>>>> +					     " nested_devlink_netns %s", name);
>>>> +				free(name);
>>>> +			} else {
>>>> +				print_int(PRINT_ANY,
>>>> +					  "nested_devlink_netnsid",
>>>> +					  " nested_devlink_netnsid %d", id);
>>>> +			}
>>>> +		} else {
>>>> +			print_string(PRINT_FP, NULL,
>>>> +				     " nested_devlink_netnsid %s", "unknown");
>>>> +			print_int(PRINT_JSON,
>>>> +				  "nested_devlink_netnsid", NULL, id);
>>>> +		}
>>> Also, devlink in the name here provides no addititional value (devlink
>>> is the command name) and why add 'nested'? The attribute is just
>>> NETNS_ID, so why not just 'netnsid' here.
>> Well, it is a netnsid of the nested devlink instance, not the object
>> (e.g. port) itself. Omitting that would be misleading. Any idea how to
>> do this differently?
>> 
>> 
>
>The attribute is a namespace id, and the value is a namespace id. Given
>that, the name here should be netnsid (or nsid - we did a horrible job
>with consistency across iproute2 commands). I have not followed the
>kernel patches to understand what you mean by nested devlink instance.

Please do that. Again, the netnsid is related to the nested instance.
Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
as you suggest is wrong. Another possibility would be do nest this into
object, but:
1) I didn't find nice way to do that
2) We would break linecards as they expose nested_devlink already

IDK :/

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Wed, Sep 20, 2023 at 09:30:01AM CEST, jiri@resnulli.us wrote:
>Tue, Sep 19, 2023 at 08:48:29PM CEST, dsahern@gmail.com wrote:
>>On 9/19/23 11:19 AM, Jiri Pirko wrote:
>>>>
>>>>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>>  {
>>>>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>>>>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>>>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>>>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>>>>> +
>>>>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>>>>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>>>>> +
>>>>> +		if (id >= 0) {
>>>>> +			char *name = netns_name_by_id(id);
>>>>> +
>>>>> +			if (name) {
>>>>> +				print_string(PRINT_ANY,
>>>>> +					     "nested_devlink_netns",
>>>>> +					     " nested_devlink_netns %s", name);
>>>>> +				free(name);
>>>>> +			} else {
>>>>> +				print_int(PRINT_ANY,
>>>>> +					  "nested_devlink_netnsid",
>>>>> +					  " nested_devlink_netnsid %d", id);
>>>>> +			}
>>>>> +		} else {
>>>>> +			print_string(PRINT_FP, NULL,
>>>>> +				     " nested_devlink_netnsid %s", "unknown");
>>>>> +			print_int(PRINT_JSON,
>>>>> +				  "nested_devlink_netnsid", NULL, id);
>>>>> +		}
>>>> Also, devlink in the name here provides no addititional value (devlink
>>>> is the command name) and why add 'nested'? The attribute is just
>>>> NETNS_ID, so why not just 'netnsid' here.
>>> Well, it is a netnsid of the nested devlink instance, not the object
>>> (e.g. port) itself. Omitting that would be misleading. Any idea how to
>>> do this differently?
>>> 
>>> 
>>
>>The attribute is a namespace id, and the value is a namespace id. Given
>>that, the name here should be netnsid (or nsid - we did a horrible job
>>with consistency across iproute2 commands). I have not followed the
>>kernel patches to understand what you mean by nested devlink instance.
>
>Please do that. Again, the netnsid is related to the nested instance.
>Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>as you suggest is wrong. Another possibility would be do nest this into
>object, but:
>1) I didn't find nice way to do that
>2) We would break linecards as they expose nested_devlink already

Did you have a chance to check this? I have v3 ready for submission with
the other changes you requested.

Thanks!


>
>IDK :/

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[David Ahern]

On 9/29/23 5:30 AM, Jiri Pirko wrote:
>>> The attribute is a namespace id, and the value is a namespace id. Given
>>> that, the name here should be netnsid (or nsid - we did a horrible job
>>> with consistency across iproute2 commands). I have not followed the
>>> kernel patches to understand what you mean by nested devlink instance.
>>
>> Please do that. Again, the netnsid is related to the nested instance.
>> Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>> as you suggest is wrong. Another possibility would be do nest this into
>> object, but:
>> 1) I didn't find nice way to do that
>> 2) We would break linecards as they expose nested_devlink already

well, that just shows I make mistakes as a reviewer. These really long
command lines are really taxing.

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Tue, Oct 03, 2023 at 06:37:31PM CEST, dsahern@gmail.com wrote:
>On 9/29/23 5:30 AM, Jiri Pirko wrote:
>>>> The attribute is a namespace id, and the value is a namespace id. Given
>>>> that, the name here should be netnsid (or nsid - we did a horrible job
>>>> with consistency across iproute2 commands). I have not followed the
>>>> kernel patches to understand what you mean by nested devlink instance.
>>>
>>> Please do that. Again, the netnsid is related to the nested instance.
>>> Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>>> as you suggest is wrong. Another possibility would be do nest this into
>>> object, but:
>>> 1) I didn't find nice way to do that
>>> 2) We would break linecards as they expose nested_devlink already
>
>well, that just shows I make mistakes as a reviewer. These really long
>command lines are really taxing.

So what do you suggest?

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[David Ahern]

On 10/3/23 11:17 AM, Jiri Pirko wrote:
> Tue, Oct 03, 2023 at 06:37:31PM CEST, dsahern@gmail.com wrote:
>> On 9/29/23 5:30 AM, Jiri Pirko wrote:
>>>>> The attribute is a namespace id, and the value is a namespace id. Given
>>>>> that, the name here should be netnsid (or nsid - we did a horrible job
>>>>> with consistency across iproute2 commands). I have not followed the
>>>>> kernel patches to understand what you mean by nested devlink instance.
>>>>
>>>> Please do that. Again, the netnsid is related to the nested instance.
>>>> Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>>>> as you suggest is wrong. Another possibility would be do nest this into
>>>> object, but:
>>>> 1) I didn't find nice way to do that
>>>> 2) We would break linecards as they expose nested_devlink already
>>
>> well, that just shows I make mistakes as a reviewer. These really long
>> command lines are really taxing.
> 
> So what do you suggest?

That you learn how to make up shorter names, leveraging established
abbreviations for example. This one new parameter is 22 chars. How do
you expect these command lines and responses to fit on a reasonable
width terminal? I have been saying this now for many years about devlink
commands - excessively long attribute names combined with duplicate
terms in a command line. Not user friendly.

Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Wed, Oct 04, 2023 at 05:20:46PM CEST, dsahern@gmail.com wrote:
>On 10/3/23 11:17 AM, Jiri Pirko wrote:
>> Tue, Oct 03, 2023 at 06:37:31PM CEST, dsahern@gmail.com wrote:
>>> On 9/29/23 5:30 AM, Jiri Pirko wrote:
>>>>>> The attribute is a namespace id, and the value is a namespace id. Given
>>>>>> that, the name here should be netnsid (or nsid - we did a horrible job
>>>>>> with consistency across iproute2 commands). I have not followed the
>>>>>> kernel patches to understand what you mean by nested devlink instance.
>>>>>
>>>>> Please do that. Again, the netnsid is related to the nested instance.
>>>>> Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>>>>> as you suggest is wrong. Another possibility would be do nest this into
>>>>> object, but:
>>>>> 1) I didn't find nice way to do that
>>>>> 2) We would break linecards as they expose nested_devlink already
>>>
>>> well, that just shows I make mistakes as a reviewer. These really long
>>> command lines are really taxing.
>> 
>> So what do you suggest?
>
>That you learn how to make up shorter names, leveraging established
>abbreviations for example. This one new parameter is 22 chars. How do
>you expect these command lines and responses to fit on a reasonable
>width terminal? I have been saying this now for many years about devlink
>commands - excessively long attribute names combined with duplicate
>terms in a command line. Not user friendly.

The problem is not the length, the problem is how to group nested
devlink handle and netnsid. Anyway..

[patch iproute2-next v2 4/5] devlink: print nested handle for port function

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

If port function contains nested handle attribute, print it.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 devlink/devlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index cf5d466bfc9d..b30e4fd8e282 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -772,6 +772,7 @@ static const enum mnl_attr_data_type
 devlink_function_policy[DEVLINK_PORT_FUNCTION_ATTR_MAX + 1] = {
 	[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR ] = MNL_TYPE_BINARY,
 	[DEVLINK_PORT_FN_ATTR_STATE] = MNL_TYPE_U8,
+	[DEVLINK_PORT_FN_ATTR_DEVLINK] = MNL_TYPE_NESTED,
 };
 
 static int function_attr_cb(const struct nlattr *attr, void *data)
@@ -4830,6 +4831,8 @@ static void pr_out_port_function(struct dl *dl, struct nlattr **tb_port)
 				     port_fn_caps->value & DEVLINK_PORT_FN_CAP_MIGRATABLE ?
 				     "enable" : "disable");
 	}
+	if (tb[DEVLINK_PORT_FN_ATTR_DEVLINK])
+		pr_out_nested_handle(tb[DEVLINK_PORT_FN_ATTR_DEVLINK]);
 
 	if (!dl->json_output)
 		__pr_out_indent_dec();
-- 
2.41.0

[patch iproute2-next v2 5/5] devlink: print nested devlink handle for devlink dev

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Devlink dev may contain one or more nested devlink instances. If one is
present, print it out simple. If more are present (there is no
such case in current kernel, but may be in theory in the future),
print them in array.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index b30e4fd8e282..06c1fbaa2404 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -3844,13 +3844,50 @@ static void pr_out_reload_data(struct dl *dl, struct nlattr **tb)
 	pr_out_object_end(dl);
 }
 
+static void pr_out_dev_nested(struct dl *dl, const struct nlmsghdr *nlh)
+{
+	struct nlattr *attr, *attr2;
+	int count = 0;
+
+	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+		if (mnl_attr_get_type(attr) == DEVLINK_ATTR_NESTED_DEVLINK) {
+			count++;
+			attr2 = attr;
+		}
+	}
+	if (!count) {
+		return;
+	} else if (count == 1) {
+		pr_out_nested_handle(attr2);
+		return;
+	}
+
+	pr_out_array_start(dl, "nested_devlinks");
+	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+		if (mnl_attr_get_type(attr) == DEVLINK_ATTR_NESTED_DEVLINK) {
+			check_indent_newline(dl);
+			if (dl->json_output)
+				open_json_object(NULL);
+			check_indent_newline(dl);
+			pr_out_nested_handle(attr);
+			if (dl->json_output)
+				close_json_object();
+			else
+				__pr_out_newline();
+		}
+	}
+	pr_out_array_end(dl);
+}
 
-static void pr_out_dev(struct dl *dl, struct nlattr **tb)
+static void pr_out_dev(struct dl *dl, const struct nlmsghdr *nlh,
+		       struct nlattr **tb)
 {
 	if ((tb[DEVLINK_ATTR_RELOAD_FAILED] && mnl_attr_get_u8(tb[DEVLINK_ATTR_RELOAD_FAILED])) ||
-	    (tb[DEVLINK_ATTR_DEV_STATS] && dl->stats)) {
+	    (tb[DEVLINK_ATTR_DEV_STATS] && dl->stats) ||
+	     tb[DEVLINK_ATTR_NESTED_DEVLINK]) {
 		__pr_out_handle_start(dl, tb, true, false);
 		pr_out_reload_data(dl, tb);
+		pr_out_dev_nested(dl, nlh);
 		pr_out_handle_end(dl);
 	} else {
 		pr_out_handle(dl, tb);
@@ -3867,7 +3904,7 @@ static int cmd_dev_show_cb(const struct nlmsghdr *nlh, void *data)
 	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME])
 		return MNL_CB_ERROR;
 
-	pr_out_dev(dl, tb);
+	pr_out_dev(dl, nlh, tb);
 	return MNL_CB_OK;
 }
 
@@ -6783,7 +6820,7 @@ static int cmd_mon_show_cb(const struct nlmsghdr *nlh, void *data)
 			return MNL_CB_ERROR;
 		pr_out_mon_header(genl->cmd);
 		dl->stats = true;
-		pr_out_dev(dl, tb);
+		pr_out_dev(dl, nlh, tb);
 		pr_out_mon_footer();
 		break;
 	case DEVLINK_CMD_PORT_GET: /* fall through */
-- 
2.41.0