Re: [PATCH iproute2 V3 3/4] rdma: Add link object

Tue, Jul 04, 2017 at 09:55:40AM CEST, leon@kernel.org wrote:

Not exported yet?

+	 */
+	static const char *link_caps[64] = {

[]

+		"UNKNOWN",
+		"SM",
+		"NOTICE",
+		"TRAP",
+		"OPT_IPD",
+		"AUTO_MIGR",
+		"SL_MAP",
+		"MKEY_NVRAM",
+		"PKEY_NVRAM",
+		"LED_INFO",
+		"SM_DISABLED",
+		"SYS_IMAGE_GUID",
+		"PKEY_SW_EXT_PORT_TRAP",
+		"UNKNOWN",
+		"EXTENDED_SPEEDS",
+		"UNKNOWN",
+		"CM",
+		"SNMP_TUNNEL",
+		"REINIT",
+		"DEVICE_MGMT",
+		"VENDOR_CLASS",
+		"DR_NOTICE",
+		"CAP_MASK_NOTICE",
+		"BOOT_MGMT",
+		"LINK_LATENCY",
+		"CLIENT_REG",
+		"IP_BASED_GIDS",
+	};
+
+	if (!tb[RDMA_NLDEV_ATTR_CAP_FLAGS])
+		return;
+
+	caps = mnl_attr_get_u64(tb[RDMA_NLDEV_ATTR_CAP_FLAGS]);
+
+	pr_out("\n    caps: <");
+	for (idx = 0; idx < 64; idx++) {
+		if (caps & 0x1) {
+			pr_out("%s", link_caps[idx]?link_caps[idx]:"UNKNONW");

"link_caps[idx] ? link_caps[idx] : "UNKNOWN""

note the s/UNKNONW/UNKNOWN/

+			if (caps >> 0x1)
+				pr_out(", ");
+		}
+		caps >>= 0x1;

Interesting. 

+	}
+
+	pr_out(">");
+}
+
+static void link_print_subnet_prefix(struct nlattr **tb)
+{
+	uint64_t subnet_prefix;
+	uint16_t sp[4];
+
+	if (!tb[RDMA_NLDEV_ATTR_SUBNET_PREFIX])
+		return;
+
+	subnet_prefix = mnl_attr_get_u64(tb[RDMA_NLDEV_ATTR_SUBNET_PREFIX]);
+	memcpy(sp, &subnet_prefix, sizeof(uint64_t));
+	pr_out("subnet_prefix %04x:%04x:%04x:%04x ", sp[3], sp[2], sp[1], sp[0]);

You have similar pr_out helper in the previous patch. Perhaps you can
re-use it?

+}
+
+static void link_print_lid(struct nlattr **tb)
+{
+	if (!tb[RDMA_NLDEV_ATTR_LID])
+		return;
+
+	pr_out("lid %u ",
+	       mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_LID]));
+}
+
+static void link_print_sm_lid(struct nlattr **tb)
+{
+

Avoid the extra empty line.

+	if (!tb[RDMA_NLDEV_ATTR_SM_LID])
+		return;
+
+	pr_out("sm_lid %u ",
+	       mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_SM_LID]));
+}
+
+static void link_print_lmc(struct nlattr **tb)
+{
+	if (!tb[RDMA_NLDEV_ATTR_LMC])
+		return;
+
+	pr_out("lmc %u ", mnl_attr_get_u8(tb[RDMA_NLDEV_ATTR_LMC]));
+}
+
+static void link_print_state(struct nlattr **tb)
+{
+	uint8_t state;
+	/*
+	 * FIXME: move to index exported by the kernel

Again, can't this be fixed now?

+	 */
+	static const char *str[] = {
+		"NOP",
+		"DOWN",
+		"INIT",
+		"ARMED",
+		"ACTIVE",
+		"ACTIVE_DEFER",
+	};
+
+	if (!tb[RDMA_NLDEV_ATTR_PORT_STATE])
+		return;
+
+	state = mnl_attr_get_u8(tb[RDMA_NLDEV_ATTR_PORT_STATE]);
+
+	if (state < 6 )

Magic?

+		pr_out("state %s ", str[state]);
+	else
+		pr_out("state UNKNOWN ");
+}
+
+static void link_print_phys_state(struct nlattr **tb)
+{
+	uint8_t phys_state;
+	/*
+	 * FIXME: move to index exported by the kernel

Again, can't this be fixed now?

+	 */
+	static const char *str[] = {
+		"UNKNOWN",
+		"SLEEP",
+		"POLLING",
+		"DISABLED",
+		"PORT_CONFIGURATION_TRAINING",
+		"LINK_UP",
+		"LINK_ERROR_RECOVER",
+		"PHY_TEST",
+	};
+
+	if (!tb[RDMA_NLDEV_ATTR_PORT_PHYS_STATE])
+		return;
+
+	phys_state = mnl_attr_get_u8(tb[RDMA_NLDEV_ATTR_PORT_PHYS_STATE]);
+	if (phys_state < 8)

Magic?

+		pr_out("physical_state %s ", str[phys_state]);
+	else
+		pr_out("physical_state UNKNOWN ");
+}
+
+static int link_parse_cb(const struct nlmsghdr *nlh, void *data)
+{
+	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX] = {};
+	struct rdma *rd = data;
+
+	mnl_attr_parse(nlh, 0, rd_attr_cb, tb);
+	if (!tb[RDMA_NLDEV_ATTR_DEV_INDEX] || !tb[RDMA_NLDEV_ATTR_DEV_NAME])
+		return MNL_CB_ERROR;
+
+	if (!tb[RDMA_NLDEV_ATTR_PORT_INDEX]) {
+		pr_err("This tool doesn't support switches yet\n");
+		return MNL_CB_ERROR;
+	}
+
+	pr_out("%u/%u: %s/%u: ",
+	       mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]),
+	       mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]),
+	       mnl_attr_get_str(tb[RDMA_NLDEV_ATTR_DEV_NAME]),
+	       mnl_attr_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]));
+	link_print_subnet_prefix(tb);
+	link_print_lid(tb);
+	link_print_sm_lid(tb);
+	link_print_lmc(tb);
+	link_print_state(tb);
+	link_print_phys_state(tb);
+	if (rd->show_details)
+		link_print_caps(tb);
+
+	pr_out("\n");
+	return MNL_CB_OK;
+}
+
+static int link_no_args(struct rdma *rd)
+{
+	uint32_t seq;
+	int ret;
+
+	rdma_prepare_msg(rd, RDMA_NLDEV_CMD_PORT_GET, &seq, (NLM_F_REQUEST | NLM_F_ACK));
+	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd->dev_idx);
+	mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_PORT_INDEX, rd->port_idx);
+	if ((ret = rdma_send_msg(rd)))
+		return ret;
+
+	return rdma_recv_msg(rd, link_parse_cb, rd, seq);
+}
+
+static int link_one_show(struct rdma *rd)
+{
+	const struct rdma_cmd cmds[] = {
+		{ NULL,		link_no_args},
+		{ 0 }
+	};
+
+	return rdma_exec_cmd(rd, cmds, "parameter");
+

Avoid the extra empty line.

+}
+
+static int link_show(struct rdma *rd)
+{
+	struct dev_map *dev_map;
+	uint32_t port;
+	int ret = 0;
+
+	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)
+					return ret;
+			}
+		}
+
+	}
+	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");
+			return -ENOENT;
+		}
+		rd_arg_inc(rd);
+		rd->port_idx = port ? :1;

"port ? : 1"

+		for (; port < dev_map->num_ports + 1; port++, rd->port_idx++) {
+			ret = link_one_show(rd);
+			if (ret)
+				return ret;
+			if (port)
+				/*
+				 * We got request to show link for devname
+				 * without port index.
+				 */
+				break;
+		}
+
+	}
+	return ret;

	You can do return 0 here and avoid ret initialization.