Skip to contentSkip to conversation

RDMAtool cleanup and refactoring code

PATCH Applied Leon Romanovsky 2 participants 12 messages Dec 27, 2017

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover RDMAtool cleanup and refactoring code

From: Leon Romanovsky <[email protected]>

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(-)
author
#1 lore raw

PATCH 1/10 rdma: Reduce scope of _dev_map_lookup call

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
---
 rdma/rdma.h  | 1 -
 rdma/utils.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)
rdma/rdma.h +0 -1
index d551eb29..c07493c9 100644
@@ -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
rdma/utils.c +1 -1
index eb4377cf..6ce1fd70 100644
@@ -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;
Signature · 1 line
2.15.1
author
#2 lore raw

PATCH 2/10 rdma: Protect dev_map_lookup from wrong input

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
---
 rdma/utils.c | 3 +++
 1 file changed, 3 insertions(+)
rdma/utils.c +3 -0
index 6ce1fd70..bb29fa1a 100644
@@ -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, '/');
Signature · 1 line
2.15.1
author
#3 lore raw

PATCH 3/10 rdma: Move per-device handler function to generic code

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
---
 rdma/dev.c   | 28 +---------------------------
 rdma/rdma.h  |  1 +
 rdma/utils.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 27 deletions(-)
rdma/dev.c +1 -27
index 9fadf3ac..03ab8683 100644
@@ -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)
rdma/rdma.h +1 -0
index c07493c9..b85e3748 100644
@@ -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
rdma/utils.c +31 -0
index bb29fa1a..5c0f021a 100644
@@ -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;
Signature · 1 line
2.15.1
author
#4 lore raw

PATCH 4/10 rdma: Fix misspelled SYS_IMAGE_GUID

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

SYS_IMAGE_GUIG is actually SYS_IMAGE_GUID.

Fixes da990ab40a92 ("rdma: Add link object")
Signed-off-by Leon Romanovsky <[email protected]>
---
 rdma/link.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
rdma/link.c +1 -1
index 3a4b00bd..f0eaccbb 100644
@@ -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) \
Signature · 1 line
2.15.1
author
#5 lore raw

PATCH 5/10 rdma: Check that port index exists before operate on link layer

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
---
 rdma/link.c | 3 +++
 1 file changed, 3 insertions(+)
rdma/link.c +3 -0
index f0eaccbb..d9392289 100644
@@ -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");
 }
Signature · 1 line
2.15.1
author
#6 lore raw

PATCH 6/10 rdma: Print supplied device name in case of wrong name

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>
---
 rdma/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
rdma/utils.c +1 -1
index 5c0f021a..998a178d 100644
@@ -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;
 		}
Signature · 1 line
2.15.1
author
#7 lore raw

PATCH 7/10 rdma: Get rid of dev_map_free call

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>
---
 rdma/utils.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
rdma/utils.c +2 -10
index 998a178d..c1069967 100644
@@ -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);
 	}
 }
Signature · 1 line
2.15.1
author
#8 lore raw

PATCH 8/10 rdma: Rename free function to be rd_cleanup

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>
---
 rdma/rdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
rdma/rdma.c +2 -2
index f9f4f2a2..a05a2fb6 100644
@@ -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;
 }
Signature · 1 line
2.15.1
author
#9 lore raw

PATCH 9/10 rdma: Rename rd_free_devmap to be rd_free

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>
---
 rdma/rdma.c  | 3 +--
 rdma/rdma.h  | 2 +-
 rdma/utils.c | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)
rdma/rdma.c +1 -2
index a05a2fb6..0e8fd688 100644
@@ -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)
rdma/rdma.h +1 -1
index b85e3748..afc0e413 100644
@@ -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);
 
 /*
rdma/utils.c +2 -1
index c1069967..d7284801 100644
@@ -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);
 }
Signature · 1 line
2.15.1
author
#10 lore raw

PATCH 10/10 rdma: Move link execution logic to common code

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>
---
 rdma/link.c  | 46 +---------------------------------------------
 rdma/rdma.h  |  1 +
 rdma/utils.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 45 deletions(-)
rdma/link.c +1 -45
index d9392289..676cb21d 100644
@@ -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)
rdma/rdma.h +1 -0
index afc0e413..8d53d3a0 100644
@@ -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);
 
 /*
rdma/utils.c +49 -0

55 lines not on this page · raw message

Signature · 1 line
2.15.1
Applied
#11 lore raw
In reply to Leon Romanovsky
Quoted 31 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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(-)
Quoted 3 lines

--
2.15.1

Applied to net-next. Thanks, Leon.