[patch iproute2-next v3 0/6] expose devlink instances relationships

[patch iproute2-next v3 0/6] 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" attributes.

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

$ devlink dev -j -p
{
    "dev": {
        "pci/0000:08:00.0": {
            "nested_devlink": {
                "auxiliary/mlx5_core.eth.0": {}
            }
        },
        "auxiliary/mlx5_core.eth.0": {},
        "pci/0000:08:00.1": {
            "nested_devlink": {
                "auxiliary/mlx5_core.eth.1": {}
            }
        },
        "auxiliary/mlx5_core.eth.1": {}
    }
}

$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 106
pci/0000:08:00.0/32768: type eth netdev eth2 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 eth2 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 port show pci/0000:08:00.0/32768 -j -p
{
    "port": {
        "pci/0000:08:00.0/32768": {
            "type": "eth",
            "netdev": "eth2",
            "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 eth2 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: netns ns1
$ devlink port show pci/0000:08:00.0/32768 -j -p
{
    "port": {
        "pci/0000:08:00.0/32768": {
            "type": "eth",
            "netdev": "eth2",
            "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": {
                        "netns": "ns1"
                    }
                }
            }
        }
    }
}

---
v2->v3:
- the output format is changed to treat the nested handle in a similar
  way as the dev_handle/port_handle allowing to contain attrs, like
  netns. See examples
- couple of details
- see individual patches for more details
v1->v2:
- patch #2 was added
- patch #3 uses new helper added by patch #2, typo is fixed

Jiri Pirko (6):
  ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c
  devlink: do conditional new line print in pr_out_port_handle_end()
  devlink: extend pr_out_nested_handle() to print object
  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   | 124 +++++++++++++++++++++++++++++++++++---------
 include/namespace.h |   5 ++
 ip/ipnetns.c        |  45 +---------------
 lib/namespace.c     |  83 +++++++++++++++++++++++++++++
 4 files changed, 189 insertions(+), 68 deletions(-)

-- 
2.41.0

[patch iproute2-next v3 1/6] 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_id_from_name().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- s/netns_netnsid_from_name/netns_id_from_name/
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..e860a4b8ee5b 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_id_from_name(struct rtnl_handle *rtnl, const char *name);
+
 #endif /* __NAMESPACE_H__ */
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 9d996832aef8..0ae46a874a0c 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_id_from_name(&rtnsh, name);
 }
 
 struct nsid_cache {
diff --git a/lib/namespace.c b/lib/namespace.c
index 1202fa85f97d..f03f4bbabceb 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_id_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

[patch iproute2-next v3 2/6] devlink: do conditional new line print in pr_out_port_handle_end()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Instead of printing out new line unconditionally, use __pr_out_newline()
to print it only when needed avoiding double prints.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- new patch
---
 devlink/devlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 3baad355759e..c18a4a4fbc5a 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -2974,7 +2974,7 @@ static void pr_out_port_handle_end(struct dl *dl)
 	if (dl->json_output)
 		close_json_object();
 	else
-		pr_out("\n");
+		__pr_out_newline();
 }
 
 static void pr_out_region_chunk_start(struct dl *dl, uint64_t addr)
-- 
2.41.0

[patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

For existing pr_out_nested_handle() user (line card), the output stays
the same. For the new users, introduce __pr_out_nested_handle()
to allow to print devlink instance as object allowing to carry
attributes in it (like netns).

Note that as __pr_out_handle_start() and pr_out_handle_end() are newly
used, the function is moved below the definitions.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- new patch
---
 devlink/devlink.c | 51 +++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index c18a4a4fbc5a..f7325477f271 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -2747,25 +2747,6 @@ static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
 	       !cmp_arr_last_handle(dl, bus_name, dev_name);
 }
 
-static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
-{
-	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
-	char buf[64];
-	int err;
-
-	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
-	if (err != MNL_CB_OK)
-		return;
-
-	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
-	    !tb[DEVLINK_ATTR_DEV_NAME])
-		return;
-
-	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);
-}
-
 static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
 				  bool content, bool array)
 {
@@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
 		__pr_out_newline();
 }
 
+static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
+				   bool is_object)
+{
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
+	int err;
+
+	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
+	if (err != MNL_CB_OK)
+		return;
+
+	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
+	    !tb[DEVLINK_ATTR_DEV_NAME])
+		return;
+
+	if (!is_object) {
+		char buf[64];
+
+		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);
+		return;
+	}
+
+	__pr_out_handle_start(dl, tb, false, false);
+	pr_out_handle_end(dl);
+}
+
+static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
+{
+	__pr_out_nested_handle(NULL, nla_nested_dl, false);
+}
+
 static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
 				     const char *dev_name, uint32_t port_index)
 {
-- 
2.41.0

[patch iproute2-next v3 4/6] 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>
---
v2->v3:
- moved netns_name_by_id() into lib/namespace.c
- s/netns_name_by_id/netns_name_from_id/
- rebased on top of new patch "devlink: extend pr_out_nested_handle() to
  print object"
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   | 23 ++++++++++++++++++++++-
 include/namespace.h |  1 +
 lib/namespace.c     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index f7325477f271..7ba2d0dcac72 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
@@ -2865,7 +2867,26 @@ static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
 		return;
 	}
 
-	__pr_out_handle_start(dl, tb, false, false);
+	__pr_out_handle_start(dl, tb, tb[DEVLINK_ATTR_NETNS_ID], false);
+	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_from_id(id);
+
+			if (name) {
+				print_string(PRINT_ANY, "netns",
+					     " netns %s", name);
+				free(name);
+			} else {
+				print_int(PRINT_ANY, "netnsid",
+					  " netnsid %d", id);
+			}
+		} else {
+			print_string(PRINT_FP, NULL, " netnsid %s", "unknown");
+			print_int(PRINT_JSON, "netnsid", NULL, id);
+		}
+	}
 	pr_out_handle_end(dl);
 }
 
diff --git a/include/namespace.h b/include/namespace.h
index e860a4b8ee5b..fbdfbc9a1c3b 100644
--- a/include/namespace.h
+++ b/include/namespace.h
@@ -61,5 +61,6 @@ struct netns_func {
 };
 
 int netns_id_from_name(struct rtnl_handle *rtnl, const char *name);
+char *netns_name_from_id(int32_t id);
 
 #endif /* __NAMESPACE_H__ */
diff --git a/lib/namespace.c b/lib/namespace.c
index f03f4bbabceb..d3aeb9658e73 100644
--- a/lib/namespace.c
+++ b/lib/namespace.c
@@ -188,3 +188,37 @@ out:
 	free(answer);
 	return ret;
 }
+
+struct netns_name_from_id_ctx {
+	int32_t id;
+	char *name;
+	struct rtnl_handle *rth;
+};
+
+static int netns_name_from_id_func(char *nsname, void *arg)
+{
+	struct netns_name_from_id_ctx *ctx = arg;
+	int32_t ret;
+
+	ret = netns_id_from_name(ctx->rth, nsname);
+	if (ret < 0 || ret != ctx->id)
+		return 0;
+	ctx->name = strdup(nsname);
+	return 1;
+}
+
+char *netns_name_from_id(int32_t id)
+{
+	struct rtnl_handle rth;
+	struct netns_name_from_id_ctx ctx = {
+		.id = id,
+		.rth = &rth,
+	};
+
+	if (rtnl_open(&rth, 0) < 0)
+		return NULL;
+	netns_foreach(netns_name_from_id_func, &ctx);
+	rtnl_close(&rth);
+
+	return ctx.name;
+}
-- 
2.41.0

[patch iproute2-next v3 5/6] 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>
---
v2->v3:
- rebased on top of new patch "devlink: extend pr_out_nested_handle() to
  print object"
---
 devlink/devlink.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 7ba2d0dcac72..90e8872b07ef 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)
@@ -2895,6 +2896,22 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
 	__pr_out_nested_handle(NULL, nla_nested_dl, false);
 }
 
+static void pr_out_nested_handle_obj(struct dl *dl,
+				     struct nlattr *nla_nested_dl,
+				     bool obj_start, bool obj_end)
+{
+	if (obj_start) {
+		pr_out_object_start(dl, "nested_devlink");
+		check_indent_newline(dl);
+	}
+	__pr_out_nested_handle(dl, nla_nested_dl, true);
+	if (obj_end) {
+		if (!dl->json_output)
+			__pr_out_indent_dec();
+		pr_out_object_end(dl);
+	}
+}
+
 static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
 				     const char *dev_name, uint32_t port_index)
 {
@@ -4837,6 +4854,9 @@ static void pr_out_port_function(struct dl *dl, struct nlattr **tb_port)
 				     port_fn_caps->value & DEVLINK_PORT_FN_CAP_IPSEC_PACKET ?
 				     "enable" : "disable");
 	}
+	if (tb[DEVLINK_PORT_FN_ATTR_DEVLINK])
+		pr_out_nested_handle_obj(dl, tb[DEVLINK_PORT_FN_ATTR_DEVLINK],
+					 true, true);
 
 	if (!dl->json_output)
 		__pr_out_indent_dec();
-- 
2.41.0

[patch iproute2-next v3 6/6] 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.
Print them using previously introduced pr_out_nested_handle_obj()
helper.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- rebased on top of new patch "devlink: extend pr_out_nested_handle() to
  print object" and previous patch to use pr_out_nested_handle_obj()
---
 devlink/devlink.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 90e8872b07ef..09ac347051f4 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -3858,13 +3858,35 @@ 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)
+{
+	int i = 0, count = 0;
+	struct nlattr *attr;
+
+	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+		if (mnl_attr_get_type(attr) == DEVLINK_ATTR_NESTED_DEVLINK)
+			count++;
+	}
+	if (!count)
+		return;
+
+	mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+		if (mnl_attr_get_type(attr) != DEVLINK_ATTR_NESTED_DEVLINK)
+			continue;
+		pr_out_nested_handle_obj(dl, attr, i == 0, i == count - 1);
+		i++;
+	}
+}
 
-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);
@@ -3881,7 +3903,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;
 }
 
@@ -6808,7 +6830,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

Re: [patch iproute2-next v3 1/6] ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c

[David Ahern]

On 10/24/23 4:03 AM, Jiri Pirko wrote:
> diff --git a/include/namespace.h b/include/namespace.h
> index e47f9b5d49d1..e860a4b8ee5b 100644
> --- a/include/namespace.h
> +++ b/include/namespace.h
> @@ -8,6 +8,8 @@
>  #include <sys/syscall.h>
>  #include <errno.h>
>  
> +#include "namespace.h"

??? including this file into itself?

> +
>  #ifndef NETNS_RUN_DIR
>  #define NETNS_RUN_DIR "/var/run/netns"
>  #endif
> @@ -58,4 +60,6 @@ struct netns_func {
>  	void *arg;
>  };
>  
> +int netns_id_from_name(struct rtnl_handle *rtnl, const char *name);
> +
>  #endif /* __NAMESPACE_H__ */

Re: [patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[David Ahern]

On 10/24/23 4:04 AM, Jiri Pirko wrote:
> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>  		__pr_out_newline();
>  }
>  
> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
> +				   bool is_object)
> +{
> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
> +	int err;
> +
> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
> +	if (err != MNL_CB_OK)
> +		return;
> +
> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
> +	    !tb[DEVLINK_ATTR_DEV_NAME])
> +		return;
> +
> +	if (!is_object) {
> +		char buf[64];
> +
> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));

buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not
see limits on bus name length, so how can you guarantee it is always <
47 characters?

Make this snprintf, check the return and make sure buf is null terminated.

> +		print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
> +		return;
> +	}
> +
> +	__pr_out_handle_start(dl, tb, false, false);
> +	pr_out_handle_end(dl);
> +}
> +
> +static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
> +{
> +	__pr_out_nested_handle(NULL, nla_nested_dl, false);
> +}
> +
>  static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
>  				     const char *dev_name, uint32_t port_index)
>  {

Re: [patch iproute2-next v3 4/6] devlink: introduce support for netns id for nested handle

[David Ahern]

On 10/24/23 4:04 AM, Jiri Pirko wrote:
> 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>
> ---
> v2->v3:
> - moved netns_name_by_id() into lib/namespace.c
> - s/netns_name_by_id/netns_name_from_id/
> - rebased on top of new patch "devlink: extend pr_out_nested_handle() to
>   print object"
> 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   | 23 ++++++++++++++++++++++-
>  include/namespace.h |  1 +
>  lib/namespace.c     | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/devlink/devlink.c b/devlink/devlink.c
> index f7325477f271..7ba2d0dcac72 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
> @@ -2865,7 +2867,26 @@ static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>  		return;
>  	}
>  
> -	__pr_out_handle_start(dl, tb, false, false);
> +	__pr_out_handle_start(dl, tb, tb[DEVLINK_ATTR_NETNS_ID], false);
> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
> +
> +		if (id >= 0) {

why does the kernel side even add DEVLINK_ATTR_NETNS_ID if
peernet2id_alloc fails?


> +			char *name = netns_name_from_id(id);
> +
> +			if (name) {
> +				print_string(PRINT_ANY, "netns",
> +					     " netns %s", name);
> +				free(name);
> +			} else {
> +				print_int(PRINT_ANY, "netnsid",
> +					  " netnsid %d", id);
> +			}
> +		} else {
> +			print_string(PRINT_FP, NULL, " netnsid %s", "unknown");
> +			print_int(PRINT_JSON, "netnsid", NULL, id);

ie., how is -1 useful to userspace?


> +		}
> +	}
>  	pr_out_handle_end(dl);
>  }
>  

Re: [patch iproute2-next v3 1/6] ip/ipnetns: move internals of get_netnsid_from_name() into namespace.c

[Jiri Pirko]

Thu, Oct 26, 2023 at 06:56:20PM CEST, dsahern@gmail.com wrote:
>On 10/24/23 4:03 AM, Jiri Pirko wrote:
>> diff --git a/include/namespace.h b/include/namespace.h
>> index e47f9b5d49d1..e860a4b8ee5b 100644
>> --- a/include/namespace.h
>> +++ b/include/namespace.h
>> @@ -8,6 +8,8 @@
>>  #include <sys/syscall.h>
>>  #include <errno.h>
>>  
>> +#include "namespace.h"
>
>??? including this file into itself?

Interesting. Must have been done during code move. Will fix.


>
>> +
>>  #ifndef NETNS_RUN_DIR
>>  #define NETNS_RUN_DIR "/var/run/netns"
>>  #endif
>> @@ -58,4 +60,6 @@ struct netns_func {
>>  	void *arg;
>>  };
>>  
>> +int netns_id_from_name(struct rtnl_handle *rtnl, const char *name);
>> +
>>  #endif /* __NAMESPACE_H__ */
>

Re: [patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[Jiri Pirko]

Thu, Oct 26, 2023 at 07:03:30PM CEST, dsahern@gmail.com wrote:
>On 10/24/23 4:04 AM, Jiri Pirko wrote:
>> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>>  		__pr_out_newline();
>>  }
>>  
>> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>> +				   bool is_object)
>> +{
>> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> +	int err;
>> +
>> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
>> +	if (err != MNL_CB_OK)
>> +		return;
>> +
>> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
>> +	    !tb[DEVLINK_ATTR_DEV_NAME])
>> +		return;
>> +
>> +	if (!is_object) {
>> +		char buf[64];
>> +
>> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>
>buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not

IFNAMSIZ is irrelevant here.


>see limits on bus name length, so how can you guarantee it is always <
>47 characters?
>
>Make this snprintf, check the return and make sure buf is null terminated.

I will fix it in separate patch, as this is just copied here.


>
>> +		print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>> +		return;
>> +	}
>> +
>> +	__pr_out_handle_start(dl, tb, false, false);
>> +	pr_out_handle_end(dl);
>> +}
>> +
>> +static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>> +{
>> +	__pr_out_nested_handle(NULL, nla_nested_dl, false);
>> +}
>> +
>>  static bool cmp_arr_last_port_handle(struct dl *dl, const char *bus_name,
>>  				     const char *dev_name, uint32_t port_index)
>>  {
>

Re: [patch iproute2-next v3 4/6] devlink: introduce support for netns id for nested handle

[Jiri Pirko]

Thu, Oct 26, 2023 at 07:08:04PM CEST, dsahern@gmail.com wrote:
>On 10/24/23 4:04 AM, Jiri Pirko wrote:
>> 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>
>> ---
>> v2->v3:
>> - moved netns_name_by_id() into lib/namespace.c
>> - s/netns_name_by_id/netns_name_from_id/
>> - rebased on top of new patch "devlink: extend pr_out_nested_handle() to
>>   print object"
>> 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   | 23 ++++++++++++++++++++++-
>>  include/namespace.h |  1 +
>>  lib/namespace.c     | 34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 57 insertions(+), 1 deletion(-)
>> 
>> diff --git a/devlink/devlink.c b/devlink/devlink.c
>> index f7325477f271..7ba2d0dcac72 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
>> @@ -2865,7 +2867,26 @@ static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>>  		return;
>>  	}
>>  
>> -	__pr_out_handle_start(dl, tb, false, false);
>> +	__pr_out_handle_start(dl, tb, tb[DEVLINK_ATTR_NETNS_ID], false);
>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>> +
>> +		if (id >= 0) {
>
>why does the kernel side even add DEVLINK_ATTR_NETNS_ID if
>peernet2id_alloc fails?

Yep. So the user knows that this is in a different namespace. This
logic is copied from iplink


>
>
>> +			char *name = netns_name_from_id(id);
>> +
>> +			if (name) {
>> +				print_string(PRINT_ANY, "netns",
>> +					     " netns %s", name);
>> +				free(name);
>> +			} else {
>> +				print_int(PRINT_ANY, "netnsid",
>> +					  " netnsid %d", id);
>> +			}
>> +		} else {
>> +			print_string(PRINT_FP, NULL, " netnsid %s", "unknown");
>> +			print_int(PRINT_JSON, "netnsid", NULL, id);
>
>ie., how is -1 useful to userspace?

The userspace knows that this device is in a different netnamespace, but
it is unknown as peernet2id_alloc failed. Should be really a rare
corner case when system is under memory pressure.


>
>
>> +		}
>> +	}
>>  	pr_out_handle_end(dl);
>>  }
>>  
>
>

Re: [patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[Petr Machata]

David Ahern <dsahern@gmail.com> writes:

> On 10/24/23 4:04 AM, Jiri Pirko wrote:
>> @@ -2861,6 +2842,38 @@ static void pr_out_selftests_handle_end(struct dl *dl)
>>  		__pr_out_newline();
>>  }
>>  
>> +static void __pr_out_nested_handle(struct dl *dl, struct nlattr *nla_nested_dl,
>> +				   bool is_object)
>> +{
>> +	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> +	int err;
>> +
>> +	err = mnl_attr_parse_nested(nla_nested_dl, attr_cb, tb);
>> +	if (err != MNL_CB_OK)
>> +		return;
>> +
>> +	if (!tb[DEVLINK_ATTR_BUS_NAME] ||
>> +	    !tb[DEVLINK_ATTR_DEV_NAME])
>> +		return;
>> +
>> +	if (!is_object) {
>> +		char buf[64];
>> +
>> +		sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>> +			mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>
> buf[64] - 1 for null terminator - 16 for IFNAMSIZ leaves 47. I do not
> see limits on bus name length, so how can you guarantee it is always <
> 47 characters?
>
> Make this snprintf, check the return and make sure buf is null terminated.

I was wondering whether somehing like this might make sense in the
iproute2 library:

	#define alloca_sprintf(FMT, ...) ({					\
		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
		char *xasprintf_buf = alloca(xasprintf_n);			\
		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
		xasprintf_buf;							\
	})

	void foo() {
		const char *buf = alloca_sprintf("%x %y %z", etc.);
		printf(... buf ...);
	}

I'm not really happy with it -- because of alloca vs. array, and because
of the double evaluation. But all those SPRINT_BUF's peppered everywhere
make me uneasy every time I read or write them.

Or maybe roll something custom asprintf-like that can reuse and/or
realloc a passed-in buffer?

The sprintf story is pretty bad in iproute2 right now, IMHO.

Re: [patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[David Ahern]

On 10/27/23 7:12 AM, Petr Machata wrote:
> I was wondering whether somehing like this might make sense in the
> iproute2 library:
> 
> 	#define alloca_sprintf(FMT, ...) ({					\
> 		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
> 		char *xasprintf_buf = alloca(xasprintf_n);			\
> 		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
> 		xasprintf_buf;							\
> 	})
> 
> 	void foo() {
> 		const char *buf = alloca_sprintf("%x %y %z", etc.);
> 		printf(... buf ...);
> 	}
> 
> I'm not really happy with it -- because of alloca vs. array, and because
> of the double evaluation. But all those SPRINT_BUF's peppered everywhere
> make me uneasy every time I read or write them.

agreed.

> 
> Or maybe roll something custom asprintf-like that can reuse and/or
> realloc a passed-in buffer?
> 
> The sprintf story is pretty bad in iproute2 right now, IMHO.

It is a bit of a mess. If you have a few cycles, want to send an RFC?
Just pick 1 or 2 to convert to show intent with a new design.

Re: [patch iproute2-next v3 3/6] devlink: extend pr_out_nested_handle() to print object

[Petr Machata]

David Ahern <dsahern@gmail.com> writes:

> On 10/27/23 7:12 AM, Petr Machata wrote:
>> I was wondering whether somehing like this might make sense in the
>> iproute2 library:
>> 
>> 	#define alloca_sprintf(FMT, ...) ({					\
>> 		int xasprintf_n = snprintf(NULL, 0, (FMT), __VA_ARGS__);	\
>> 		char *xasprintf_buf = alloca(xasprintf_n);			\
>> 		sprintf(xasprintf_buf, (FMT), __VA_ARGS__);			\
>> 		xasprintf_buf;							\
>> 	})
>> 
>> 	void foo() {
>> 		const char *buf = alloca_sprintf("%x %y %z", etc.);
>> 		printf(... buf ...);
>> 	}
>> 
>> I'm not really happy with it -- because of alloca vs. array, and because
>> of the double evaluation. But all those SPRINT_BUF's peppered everywhere
>> make me uneasy every time I read or write them.
>
> agreed.
>
>> 
>> Or maybe roll something custom asprintf-like that can reuse and/or
>> realloc a passed-in buffer?
>> 
>> The sprintf story is pretty bad in iproute2 right now, IMHO.
>
> It is a bit of a mess. If you have a few cycles, want to send an RFC?
> Just pick 1 or 2 to convert to show intent with a new design.

I picked at it a bit over the weekend, but came up with nothing that I
find comfortable proposing. The static buffer approach has some major
advantages: nothing ever fails and nothing ever needs cleanups. This
keeps the client code tidy and compact. Anything dynamic adds points of
failure and cleanups, which in C means more client-side boilerplate.
Anyway, I'll pick at it some more and see I find anything.