[PATCH iproute2-next 00/10] RDMAtool cleanup and refactoring code

[PATCH iproute2-next 00/10] RDMAtool cleanup and refactoring code

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Hi,

The following patchset comes as a preparation to more complex code,
which will add resource tracking visibility to the rdmatool, where
the kernel part is under review of RDMA community.

Thanks

[1] https://marc.info/?l=linux-rdma&m=151412508816802&w=2

Leon Romanovsky (10):
  rdma: Reduce scope of _dev_map_lookup call
  rdma: Protect dev_map_lookup from wrong input
  rdma: Move per-device handler function to generic code
  rdma: Fix misspelled SYS_IMAGE_GUID
  rdma: Check that port index exists before operate on link layer
  rdma: Print supplied device name in case of wrong name
  rdma: Get rid of dev_map_free call
  rdma: Rename free function to be rd_cleanup
  rdma: Rename rd_free_devmap to be rd_free
  rdma: Move link execution logic to common code

 rdma/dev.c   |  28 +----------------
 rdma/link.c  |  51 +++---------------------------
 rdma/rdma.c  |   7 ++---
 rdma/rdma.h  |   5 +--
 rdma/utils.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 100 insertions(+), 91 deletions(-)

[PATCH iproute2-next 01/10] rdma: Reduce scope of _dev_map_lookup call

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

There is no external users of _dev_map_lookup function,
so let's limit its scope to be local.

Fixes: 40df8263a0f0 ("rdma: Add dev object")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/rdma.h  | 1 -
 rdma/utils.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/rdma/rdma.h b/rdma/rdma.h
index d551eb29..c07493c9 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -78,7 +78,6 @@ int rd_exec_cmd(struct rd *rd, const struct rd_cmd *c, const char *str);
  */
 void rd_free_devmap(struct rd *rd);
 struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index);
-struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name);
 
 /*
  * Netlink
diff --git a/rdma/utils.c b/rdma/utils.c
index eb4377cf..6ce1fd70 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -236,7 +236,7 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, unsigned int seq)
 	return ret;
 }
 
-struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name)
+static struct dev_map *_dev_map_lookup(struct rd *rd, const char *dev_name)
 {
 	struct dev_map *dev_map;
 
-- 
2.15.1

[PATCH iproute2-next 02/10] rdma: Protect dev_map_lookup from wrong input

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Despite the fact that all callers to dev_map_lookup are ensuring that
there is always device name prior to call to that function, it is better
and safer to check that in the dev_map_lookup itself.

Fixes: 40df8263a0f0 ("rdma: Add dev object")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rdma/utils.c b/rdma/utils.c
index 6ce1fd70..bb29fa1a 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -253,6 +253,9 @@ struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index)
 	char *dev_name;
 	char *slash;
 
+	if (rd_no_arg(rd))
+		return NULL;
+
 	dev_name = strdup(rd_argv(rd));
 	if (allow_port_index) {
 		slash = strrchr(dev_name, '/');
-- 
2.15.1

[PATCH iproute2-next 03/10] rdma: Move per-device handler function to generic code

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Most of the proposed objects are working in the scope "dev"
and will implement the same logic. Move the code to utils.c,
so other objects will be able to reuse the code.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/dev.c   | 28 +---------------------------
 rdma/rdma.h  |  1 +
 rdma/utils.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/rdma/dev.c b/rdma/dev.c
index 9fadf3ac..03ab8683 100644
--- a/rdma/dev.c
+++ b/rdma/dev.c
@@ -241,33 +241,7 @@ static int dev_one_show(struct rd *rd)
 
 static int dev_show(struct rd *rd)
 {
-	struct dev_map *dev_map;
-	int ret = 0;
-
-	if (rd->json_output)
-		jsonw_start_array(rd->jw);
-	if (rd_no_arg(rd)) {
-		list_for_each_entry(dev_map, &rd->dev_map_list, list) {
-			rd->dev_idx = dev_map->idx;
-			ret = dev_one_show(rd);
-			if (ret)
-				goto out;
-		}
-	} else {
-		dev_map = dev_map_lookup(rd, false);
-		if (!dev_map) {
-			pr_err("Wrong device name\n");
-			ret = -ENOENT;
-			goto out;
-		}
-		rd_arg_inc(rd);
-		rd->dev_idx = dev_map->idx;
-		ret = dev_one_show(rd);
-	}
-out:
-	if (rd->json_output)
-		jsonw_end_array(rd->jw);
-	return ret;
+	return rd_exec_dev(rd, dev_one_show);
 }
 
 int cmd_dev(struct rd *rd)
diff --git a/rdma/rdma.h b/rdma/rdma.h
index c07493c9..b85e3748 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -72,6 +72,7 @@ uint32_t get_port_from_argv(struct rd *rd);
 int cmd_dev(struct rd *rd);
 int cmd_link(struct rd *rd);
 int rd_exec_cmd(struct rd *rd, const struct rd_cmd *c, const char *str);
+int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd));
 
 /*
  * Device manipulation
diff --git a/rdma/utils.c b/rdma/utils.c
index bb29fa1a..5c0f021a 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -159,6 +159,37 @@ void rd_free_devmap(struct rd *rd)
 	dev_map_cleanup(rd);
 }
 
+int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd))
+{
+	struct dev_map *dev_map;
+	int ret = 0;
+
+	if (rd->json_output)
+		jsonw_start_array(rd->jw);
+	if (rd_no_arg(rd)) {
+		list_for_each_entry(dev_map, &rd->dev_map_list, list) {
+			rd->dev_idx = dev_map->idx;
+			ret = cb(rd);
+			if (ret)
+				goto out;
+		}
+	} else {
+		dev_map = dev_map_lookup(rd, false);
+		if (!dev_map) {
+			pr_err("Wrong device name\n");
+			ret = -ENOENT;
+			goto out;
+		}
+		rd_arg_inc(rd);
+		rd->dev_idx = dev_map->idx;
+		ret = cb(rd);
+	}
+out:
+	if (rd->json_output)
+		jsonw_end_array(rd->jw);
+	return ret;
+}
+
 int rd_exec_cmd(struct rd *rd, const struct rd_cmd *cmds, const char *str)
 {
 	const struct rd_cmd *c;
-- 
2.15.1

[PATCH iproute2-next 04/10] rdma: Fix misspelled SYS_IMAGE_GUID

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

SYS_IMAGE_GUIG is actually SYS_IMAGE_GUID.

Fixes: da990ab40a92 ("rdma: Add link object")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/link.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rdma/link.c b/rdma/link.c
index 3a4b00bd..f0eaccbb 100644
--- a/rdma/link.c
+++ b/rdma/link.c
@@ -30,7 +30,7 @@ static const char *caps_to_str(uint32_t idx)
 	x(PKEY_NVRAM, 8) \
 	x(LED_INFO, 9) \
 	x(SM_DISABLED, 10) \
-	x(SYS_IMAGE_GUIG, 11) \
+	x(SYS_IMAGE_GUID, 11) \
 	x(PKEY_SW_EXT_PORT_TRAP, 12) \
 	x(EXTENDED_SPEEDS, 14) \
 	x(CM, 16) \
-- 
2.15.1

[PATCH iproute2-next 05/10] rdma: Check that port index exists before operate on link layer

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Link layer operates on port layer, hence it should check
it existence before execution commands.

Fixes: da990ab40a92 ("rdma: Add link object")
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/link.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rdma/link.c b/rdma/link.c
index f0eaccbb..d9392289 100644
--- a/rdma/link.c
+++ b/rdma/link.c
@@ -277,6 +277,9 @@ static int link_one_show(struct rd *rd)
 		{ 0 }
 	};
 
+	if (!rd->port_idx)
+		return 0;
+
 	return rd_exec_cmd(rd, cmds, "parameter");
 }
 
-- 
2.15.1

[PATCH iproute2-next 06/10] rdma: Print supplied device name in case of wrong name

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rdma/utils.c b/rdma/utils.c
index 5c0f021a..998a178d 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -176,7 +176,7 @@ int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd))
 	} else {
 		dev_map = dev_map_lookup(rd, false);
 		if (!dev_map) {
-			pr_err("Wrong device name\n");
+			pr_err("Wrong device name - %s\n", rd_argv(rd));
 			ret = -ENOENT;
 			goto out;
 		}
-- 
2.15.1

[PATCH iproute2-next 07/10] rdma: Get rid of dev_map_free call

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

The dev_map_free() is called once only and it is short,
so it is better to integrate it into the caller's site.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/utils.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/rdma/utils.c b/rdma/utils.c
index 998a178d..c1069967 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -71,15 +71,6 @@ static struct dev_map *dev_map_alloc(const char *dev_name)
 	return dev_map;
 }
 
-static void dev_map_free(struct dev_map *dev_map)
-{
-	if (!dev_map)
-		return;
-
-	free(dev_map->dev_name);
-	free(dev_map);
-}
-
 static void dev_map_cleanup(struct rd *rd)
 {
 	struct dev_map *dev_map, *tmp;
@@ -87,7 +78,8 @@ static void dev_map_cleanup(struct rd *rd)
 	list_for_each_entry_safe(dev_map, tmp,
 				 &rd->dev_map_list, list) {
 		list_del(&dev_map->list);
-		dev_map_free(dev_map);
+		free(dev_map->dev_name);
+		free(dev_map);
 	}
 }
 
-- 
2.15.1

[PATCH iproute2-next 08/10] rdma: Rename free function to be rd_cleanup

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/rdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rdma/rdma.c b/rdma/rdma.c
index f9f4f2a2..a05a2fb6 100644
--- a/rdma/rdma.c
+++ b/rdma/rdma.c
@@ -70,7 +70,7 @@ static int rd_init(struct rd *rd, int argc, char **argv, char *filename)
 	return rd_recv_msg(rd, rd_dev_init_cb, rd, seq);
 }
 
-static void rd_free(struct rd *rd)
+static void rd_cleanup(struct rd *rd)
 {
 	if (rd->json_output)
 		jsonw_destroy(&rd->jw);
@@ -138,6 +138,6 @@ int main(int argc, char **argv)
 	err = rd_cmd(&rd);
 out:
 	/* Always cleanup */
-	rd_free(&rd);
+	rd_cleanup(&rd);
 	return err ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.15.1

[PATCH iproute2-next 09/10] rdma: Rename rd_free_devmap to be rd_free

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/rdma.c  | 3 +--
 rdma/rdma.h  | 2 +-
 rdma/utils.c | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rdma/rdma.c b/rdma/rdma.c
index a05a2fb6..0e8fd688 100644
--- a/rdma/rdma.c
+++ b/rdma/rdma.c
@@ -74,8 +74,7 @@ static void rd_cleanup(struct rd *rd)
 {
 	if (rd->json_output)
 		jsonw_destroy(&rd->jw);
-	free(rd->buff);
-	rd_free_devmap(rd);
+	rd_free(rd);
 }
 
 int main(int argc, char **argv)
diff --git a/rdma/rdma.h b/rdma/rdma.h
index b85e3748..afc0e413 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -73,11 +73,11 @@ int cmd_dev(struct rd *rd);
 int cmd_link(struct rd *rd);
 int rd_exec_cmd(struct rd *rd, const struct rd_cmd *c, const char *str);
 int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd));
+void rd_free(struct rd *rd);
 
 /*
  * Device manipulation
  */
-void rd_free_devmap(struct rd *rd);
 struct dev_map *dev_map_lookup(struct rd *rd, bool allow_port_index);
 
 /*
diff --git a/rdma/utils.c b/rdma/utils.c
index c1069967..d7284801 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -144,10 +144,11 @@ int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-void rd_free_devmap(struct rd *rd)
+void rd_free(struct rd *rd)
 {
 	if (!rd)
 		return;
+	free(rd->buff);
 	dev_map_cleanup(rd);
 }
 
-- 
2.15.1

[PATCH iproute2-next 10/10] rdma: Move link execution logic to common code

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 rdma/link.c  | 46 +---------------------------------------------
 rdma/rdma.h  |  1 +
 rdma/utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/rdma/link.c b/rdma/link.c
index d9392289..676cb21d 100644
--- a/rdma/link.c
+++ b/rdma/link.c
@@ -285,51 +285,7 @@ static int link_one_show(struct rd *rd)
 
 static int link_show(struct rd *rd)
 {
-	struct dev_map *dev_map;
-	uint32_t port;
-	int ret = 0;
-
-	if (rd->json_output)
-		jsonw_start_array(rd->jw);
-	if (rd_no_arg(rd)) {
-		list_for_each_entry(dev_map, &rd->dev_map_list, list) {
-			rd->dev_idx = dev_map->idx;
-			for (port = 1; port < dev_map->num_ports + 1; port++) {
-				rd->port_idx = port;
-				ret = link_one_show(rd);
-				if (ret)
-					goto out;
-			}
-		}
-
-	} else {
-		dev_map = dev_map_lookup(rd, true);
-		port = get_port_from_argv(rd);
-		if (!dev_map || port > dev_map->num_ports) {
-			pr_err("Wrong device name\n");
-			ret = -ENOENT;
-			goto out;
-		}
-		rd_arg_inc(rd);
-		rd->dev_idx = dev_map->idx;
-		rd->port_idx = port ? : 1;
-		for (; rd->port_idx < dev_map->num_ports + 1; rd->port_idx++) {
-			ret = link_one_show(rd);
-			if (ret)
-				goto out;
-			if (port)
-				/*
-				 * We got request to show link for devname
-				 * with port index.
-				 */
-				break;
-		}
-	}
-
-out:
-	if (rd->json_output)
-		jsonw_end_array(rd->jw);
-	return ret;
+	return rd_exec_link(rd, link_one_show);
 }
 
 int cmd_link(struct rd *rd)
diff --git a/rdma/rdma.h b/rdma/rdma.h
index afc0e413..8d53d3a0 100644
--- a/rdma/rdma.h
+++ b/rdma/rdma.h
@@ -73,6 +73,7 @@ int cmd_dev(struct rd *rd);
 int cmd_link(struct rd *rd);
 int rd_exec_cmd(struct rd *rd, const struct rd_cmd *c, const char *str);
 int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd));
+int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd));
 void rd_free(struct rd *rd);
 
 /*
diff --git a/rdma/utils.c b/rdma/utils.c
index d7284801..7b2001e2 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -152,6 +152,55 @@ void rd_free(struct rd *rd)
 	dev_map_cleanup(rd);
 }
 
+int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd))
+{
+	struct dev_map *dev_map;
+	uint32_t port;
+	int ret = 0;
+
+	if (rd->json_output)
+		jsonw_start_array(rd->jw);
+	if (rd_no_arg(rd)) {
+		list_for_each_entry(dev_map, &rd->dev_map_list, list) {
+			rd->dev_idx = dev_map->idx;
+			for (port = 1; port < dev_map->num_ports + 1; port++) {
+				rd->port_idx = port;
+				ret = cb(rd);
+				if (ret)
+					goto out;
+			}
+		}
+
+	} else {
+		dev_map = dev_map_lookup(rd, true);
+		port = get_port_from_argv(rd);
+		if (!dev_map || port > dev_map->num_ports) {
+			pr_err("Wrong device name\n");
+			ret = -ENOENT;
+			goto out;
+		}
+		rd_arg_inc(rd);
+		rd->dev_idx = dev_map->idx;
+		rd->port_idx = port ? : 1;
+		for (; rd->port_idx < dev_map->num_ports + 1; rd->port_idx++) {
+			ret = cb(rd);
+			if (ret)
+				goto out;
+			if (port)
+				/*
+				 * We got request to show link for devname
+				 * with port index.
+				 */
+				break;
+		}
+	}
+
+out:
+	if (rd->json_output)
+		jsonw_end_array(rd->jw);
+	return ret;
+}
+
 int rd_exec_dev(struct rd *rd, int (*cb)(struct rd *rd))
 {
 	struct dev_map *dev_map;
-- 
2.15.1

Re: [PATCH iproute2-next 00/10] RDMAtool cleanup and refactoring code

[David Ahern]

On 12/27/17 1:57 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Hi,
> 
> The following patchset comes as a preparation to more complex code,
> which will add resource tracking visibility to the rdmatool, where
> the kernel part is under review of RDMA community.
> 
> Thanks
> 
> [1] https://marc.info/?l=linux-rdma&m=151412508816802&w=2
> 
> Leon Romanovsky (10):
>   rdma: Reduce scope of _dev_map_lookup call
>   rdma: Protect dev_map_lookup from wrong input
>   rdma: Move per-device handler function to generic code
>   rdma: Fix misspelled SYS_IMAGE_GUID
>   rdma: Check that port index exists before operate on link layer
>   rdma: Print supplied device name in case of wrong name
>   rdma: Get rid of dev_map_free call
>   rdma: Rename free function to be rd_cleanup
>   rdma: Rename rd_free_devmap to be rd_free
>   rdma: Move link execution logic to common code
> 
>  rdma/dev.c   |  28 +----------------
>  rdma/link.c  |  51 +++---------------------------
>  rdma/rdma.c  |   7 ++---
>  rdma/rdma.h  |   5 +--
>  rdma/utils.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  5 files changed, 100 insertions(+), 91 deletions(-)
> 
> --
> 2.15.1
> 


Applied to net-next. Thanks, Leon.