diff mbox series

[iproute2-next,6/6] ipaddress: Get rid of print_linkinfo_brief()

Message ID 1517331168-4132-7-git-send-email-serhe.popovych@gmail.com
State Superseded, archived
Delegated to: David Ahern
Headers show
Series ipaddress: Get rid of print_linkinfo_brief() | expand

Commit Message

Serhey Popovych Jan. 30, 2018, 4:52 p.m. UTC 
It's functionality can be fully accomplished by print_linkinfo(): no
need to duplicate code.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/ip_common.h |    2 -
 ip/ipaddress.c |  126 ++++++++++++++------------------------------------------
 ip/iplink.c    |    5 +--
 3 files changed, 33 insertions(+), 100 deletions(-)

Comments

Stephen Hemminger Jan. 30, 2018, 5:24 p.m. UTC | #1 
On Tue, 30 Jan 2018 18:52:48 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> +	if (brief) {
> +		print_name_and_link("%-16s ", COLOR_NONE, name, tb);
> +
> +		if (tb[IFLA_OPERSTATE])
> +			print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
> +
> +		if (filter.family == AF_PACKET) {
> +			if (tb[IFLA_ADDRESS]) {
> +				struct rtattr *rta = tb[IFLA_ADDRESS];
> +
> +				print_color_string(PRINT_ANY,
> +						   COLOR_MAC,
> +						   "address",
> +						   "%s ",
> +						   ll_addr_n2a(RTA_DATA(rta),
> +							       RTA_PAYLOAD(rta),
> +							       ifi->ifi_type,
> +							       b1, sizeof(b1)));
> +			}
> +
> +			print_link_flags(fp, ifi->ifi_flags, m_flag);
> +			print_string(PRINT_FP, NULL, "%s", "\n");
> +		}
> +
> +		fflush(fp);
> +		return 0;
> +	}

To keep function shorter and therefore more readable, why not:

	if (brief)
		return print_linkinfo_brief(fp, ifi, tb);

And put this if branch in new version of print_linkinfo_brief.
Serhey Popovych Jan. 30, 2018, 6:10 p.m. UTC | #2 
Stephen Hemminger wrote:
> On Tue, 30 Jan 2018 18:52:48 +0200
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> +	if (brief) {
>> +		print_name_and_link("%-16s ", COLOR_NONE, name, tb);
>> +
>> +		if (tb[IFLA_OPERSTATE])
>> +			print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
>> +
>> +		if (filter.family == AF_PACKET) {
>> +			if (tb[IFLA_ADDRESS]) {
>> +				struct rtattr *rta = tb[IFLA_ADDRESS];
>> +
>> +				print_color_string(PRINT_ANY,
>> +						   COLOR_MAC,
>> +						   "address",
>> +						   "%s ",
>> +						   ll_addr_n2a(RTA_DATA(rta),
>> +							       RTA_PAYLOAD(rta),
>> +							       ifi->ifi_type,
>> +							       b1, sizeof(b1)));
>> +			}
>> +
>> +			print_link_flags(fp, ifi->ifi_flags, m_flag);
>> +			print_string(PRINT_FP, NULL, "%s", "\n");
>> +		}
>> +
>> +		fflush(fp);
>> +		return 0;
>> +	}
> 
> To keep function shorter and therefore more readable, why not:
> 
> 	if (brief)
> 		return print_linkinfo_brief(fp, ifi, tb);
> 
> And put this if branch in new version of print_linkinfo_brief.
> 

Agree, will make it static and branch as suggested. Thanks.
Addressed in v2.
diff mbox series

Patch

diff --git a/ip/ip_common.h b/ip/ip_common.h
index f5adbad..a7bbf1d 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -29,8 +29,6 @@  struct link_filter {
 int get_operstate(const char *name);
 int print_linkinfo(const struct sockaddr_nl *who,
 		   struct nlmsghdr *n, void *arg);
-int print_linkinfo_brief(const struct sockaddr_nl *who,
-			 struct nlmsghdr *n, void *arg);
 int print_addrinfo(const struct sockaddr_nl *who,
 		   struct nlmsghdr *n, void *arg);
 int print_addrlabel(const struct sockaddr_nl *who,
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5afc9d4..88e61e4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -918,90 +918,6 @@  static void print_link_stats(FILE *fp, struct nlmsghdr *n)
 	fprintf(fp, "%s", _SL_);
 }
 
-int print_linkinfo_brief(const struct sockaddr_nl *who,
-			 struct nlmsghdr *n, void *arg)
-{
-	FILE *fp = (FILE *)arg;
-	struct ifinfomsg *ifi = NLMSG_DATA(n);
-	struct rtattr *tb[IFLA_MAX+1];
-	int len = n->nlmsg_len;
-	const char *name;
-	unsigned int m_flag = 0;
-
-	if (n->nlmsg_type != RTM_NEWLINK && n->nlmsg_type != RTM_DELLINK)
-		return -1;
-
-	len -= NLMSG_LENGTH(sizeof(*ifi));
-	if (len < 0)
-		return -1;
-
-	if (filter.ifindex && ifi->ifi_index != filter.ifindex)
-		return -1;
-	if (filter.up && !(ifi->ifi_flags&IFF_UP))
-		return -1;
-
-	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
-
-	name = get_ifname_rta(ifi->ifi_index, tb[IFLA_IFNAME]);
-	if (!name)
-		return -1;
-
-	if (filter.label &&
-	    (!filter.family || filter.family == AF_PACKET) &&
-	    fnmatch(filter.label, name, 0))
-		return -1;
-
-	if (tb[IFLA_GROUP]) {
-		int group = rta_getattr_u32(tb[IFLA_GROUP]);
-
-		if (filter.group != -1 && group != filter.group)
-			return -1;
-	}
-
-	if (tb[IFLA_MASTER]) {
-		int master = rta_getattr_u32(tb[IFLA_MASTER]);
-
-		if (filter.master > 0 && master != filter.master)
-			return -1;
-	} else if (filter.master > 0)
-		return -1;
-
-	if (filter.kind && match_link_kind(tb, filter.kind, 0))
-		return -1;
-
-	if (filter.slave_kind && match_link_kind(tb, filter.slave_kind, 1))
-		return -1;
-
-	if (n->nlmsg_type == RTM_DELLINK)
-		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
-
-	m_flag = print_name_and_link("%-16s ", COLOR_NONE, name, tb);
-
-	if (tb[IFLA_OPERSTATE])
-		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
-
-	if (filter.family == AF_PACKET) {
-		SPRINT_BUF(b1);
-
-		if (tb[IFLA_ADDRESS]) {
-			print_color_string(PRINT_ANY, COLOR_MAC,
-					   "address", "%s ",
-					   ll_addr_n2a(
-						   RTA_DATA(tb[IFLA_ADDRESS]),
-						   RTA_PAYLOAD(tb[IFLA_ADDRESS]),
-						   ifi->ifi_type,
-						   b1, sizeof(b1)));
-		}
-	}
-
-	if (filter.family == AF_PACKET) {
-		print_link_flags(fp, ifi->ifi_flags, m_flag);
-		print_string(PRINT_FP, NULL, "%s", "\n");
-	}
-	fflush(fp);
-	return 0;
-}
-
 static const char *link_events[] = {
 	[IFLA_EVENT_NONE] = "NONE",
 	[IFLA_EVENT_REBOOT] = "REBOOT",
@@ -1033,6 +949,7 @@  int print_linkinfo(const struct sockaddr_nl *who,
 	int len = n->nlmsg_len;
 	const char *name;
 	unsigned int m_flag = 0;
+	SPRINT_BUF(b1);
 
 	if (n->nlmsg_type != RTM_NEWLINK && n->nlmsg_type != RTM_DELLINK)
 		return 0;
@@ -1081,6 +998,34 @@  int print_linkinfo(const struct sockaddr_nl *who,
 	if (n->nlmsg_type == RTM_DELLINK)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
+	if (brief) {
+		print_name_and_link("%-16s ", COLOR_NONE, name, tb);
+
+		if (tb[IFLA_OPERSTATE])
+			print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
+
+		if (filter.family == AF_PACKET) {
+			if (tb[IFLA_ADDRESS]) {
+				struct rtattr *rta = tb[IFLA_ADDRESS];
+
+				print_color_string(PRINT_ANY,
+						   COLOR_MAC,
+						   "address",
+						   "%s ",
+						   ll_addr_n2a(RTA_DATA(rta),
+							       RTA_PAYLOAD(rta),
+							       ifi->ifi_type,
+							       b1, sizeof(b1)));
+			}
+
+			print_link_flags(fp, ifi->ifi_flags, m_flag);
+			print_string(PRINT_FP, NULL, "%s", "\n");
+		}
+
+		fflush(fp);
+		return 0;
+	}
+
 	print_int(PRINT_ANY, "ifindex", "%d: ", ifi->ifi_index);
 	m_flag = print_name_and_link("%s: ", COLOR_IFNAME, name, tb);
 	print_link_flags(fp, ifi->ifi_flags, m_flag);
@@ -1097,8 +1042,6 @@  int print_linkinfo(const struct sockaddr_nl *who,
 			     "qdisc %s ",
 			     rta_getattr_str(tb[IFLA_QDISC]));
 	if (tb[IFLA_MASTER]) {
-		SPRINT_BUF(b1);
-
 		print_string(PRINT_ANY,
 			     "master",
 			     "master %s ",
@@ -1112,7 +1055,6 @@  int print_linkinfo(const struct sockaddr_nl *who,
 		print_linkmode(fp, tb[IFLA_LINKMODE]);
 
 	if (tb[IFLA_GROUP]) {
-		SPRINT_BUF(b1);
 		int group = rta_getattr_u32(tb[IFLA_GROUP]);
 
 		print_string(PRINT_ANY,
@@ -1279,7 +1221,7 @@  int print_linkinfo(const struct sockaddr_nl *who,
 		close_json_array(PRINT_JSON, NULL);
 	}
 
-	print_string(PRINT_FP, NULL, "\n", NULL);
+	print_string(PRINT_FP, NULL, "%s", "\n");
 	fflush(fp);
 	return 1;
 }
@@ -2138,14 +2080,10 @@  static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 	for (l = linfo.head; l; l = l->next) {
 		struct nlmsghdr *n = &l->h;
 		struct ifinfomsg *ifi = NLMSG_DATA(n);
-		int res;
+		int res = 0;
 
 		open_json_object(NULL);
-		if (brief)
-			res = print_linkinfo_brief(NULL, n, stdout);
-		else if (no_link)
-			res = 0;
-		else
+		if (brief || !no_link)
 			res = print_linkinfo(NULL, n, stdout);
 		if (res >= 0 && filter.family != AF_PACKET)
 			print_selected_addrinfo(ifi, ainfo->head, stdout);
diff --git a/ip/iplink.c b/ip/iplink.c
index 882cd13..6f8f7cd 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1076,10 +1076,7 @@  int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 		return -2;
 
 	open_json_object(NULL);
-	if (brief)
-		print_linkinfo_brief(NULL, answer, stdout);
-	else
-		print_linkinfo(NULL, answer, stdout);
+	print_linkinfo(NULL, answer, stdout);
 	close_json_object();
 
 	free(answer);