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

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

[Serhey Popovych]

With this series I propose to get rid of custom print_linkinfo_brief()
in favor of print_linkinfo() to avoid code duplication.

Changes presented with this series tested using following script:

iproute2_dir="$1"
iface='eth0.2'

pushd "$iproute2_dir" &>/dev/null

for i in new old; do
	DIR="/tmp/$i"
	mkdir -p "$DIR"

	ln -snf ip.$i ip/ip

	# normal
	ip/ip link show                  >"$DIR/ip-link-show"
	ip/ip -4 addr show               >"$DIR/ip-4-addr-show"
	ip/ip -6 addr show               >"$DIR/ip-6-addr-show"
	ip/ip addr show dev "$iface"     >"$DIR/ip-addr-show-$iface"

	# brief
	ip/ip -br link show              >"$DIR/ip-br-link-show"
	ip/ip -br -4 addr show           >"$DIR/ip-br-4-addr-show"
	ip/ip -br -6 addr show           >"$DIR/ip-br-6-addr-show"
	ip/ip -br addr show dev "$iface" >"$DIR/ip-br-addr-show-$iface"
done
rm -f ip/ip

diff -urN /tmp/{old,new}
rc=$?

popd &>/dev/null
exit $rc

Expected results : <no output>
Actual results   : <no output>

Although test coverage is far from ideal in my opinion it covers most
important aspects of the changes presented by the series.

All this work is done in prepare of iplink_get() enhancements to support
attribute parse that finally will be used to simplify ip/tunnel
RTM_GETLINK code.

As always reviews, comments, suggestions and criticism is welcome.

Thanks,
Serhii

Serhey Popovych (6):
  ipaddress: Improve print_linkinfo()
  ipaddress: Simplify print_linkinfo_brief() and it's usage
  lib: Correct object file dependencies
  utils: Introduce and use get_ifname_rta()
  utils: Introduce and use print_name_and_link() to print name@link
  ipaddress: Get rid of print_linkinfo_brief()

 bridge/link.c   |   21 ++----
 include/utils.h |    5 ++
 ip/ip_common.h  |    3 -
 ip/ipaddress.c  |  210 ++++++++++++-------------------------------------------
 ip/iplink.c     |    5 +-
 lib/Makefile    |    4 +-
 lib/utils.c     |   70 +++++++++++++++++++
 7 files changed, 129 insertions(+), 189 deletions(-)

-- 
1.7.10.4

[PATCH iproute2-next 1/6] ipaddress: Improve print_linkinfo()

[Serhey Popovych]

There are few places to improve:

  1) return -1 when entry is filtered instead of zero, which means
     accept entry: ipaddress_list_flush_or_save() the only user of this

  2) use ll_index_to_name() as last resort to translate name to index:
     it will return name in the form "if%d" in case of failure

  3) replace open coded access to IFLA_IFNAME attribute data by
     RTA_DATA() with rta_getattr_str()

  4) simplify ifname printing since name is never NULL, thanks to (2).

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/ipaddress.c |   30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 051a05f..f8fd392 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -948,14 +948,14 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
 	if (tb[IFLA_IFNAME] == NULL) {
 		fprintf(stderr, "BUG: device with ifindex %d has nil ifname\n", ifi->ifi_index);
-		name = "<nil>";
+		name = ll_index_to_name(ifi->ifi_index);
 	} else {
 		name = rta_getattr_str(tb[IFLA_IFNAME]);
 	}
 
 	if (pfilter->label &&
 	    (!pfilter->family || pfilter->family == AF_PACKET) &&
-	    fnmatch(pfilter->label, RTA_DATA(tb[IFLA_IFNAME]), 0))
+	    fnmatch(pfilter->label, name, 0))
 		return -1;
 
 	if (tb[IFLA_GROUP]) {
@@ -1057,6 +1057,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
 	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)
@@ -1067,18 +1068,22 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		return -1;
 
 	if (filter.ifindex && ifi->ifi_index != filter.ifindex)
-		return 0;
+		return -1;
 	if (filter.up && !(ifi->ifi_flags&IFF_UP))
-		return 0;
+		return -1;
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
-	if (tb[IFLA_IFNAME] == NULL)
+	if (tb[IFLA_IFNAME] == NULL) {
 		fprintf(stderr, "BUG: device with ifindex %d has nil ifname\n", ifi->ifi_index);
+		name = ll_index_to_name(ifi->ifi_index);
+	} else {
+		name = rta_getattr_str(tb[IFLA_IFNAME]);
+	}
 
 	if (filter.label &&
 	    (!filter.family || filter.family == AF_PACKET) &&
-	    fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
-		return 0;
+	    fnmatch(filter.label, name, 0))
+		return -1;
 
 	if (tb[IFLA_GROUP]) {
 		int group = rta_getattr_u32(tb[IFLA_GROUP]);
@@ -1105,16 +1110,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
 	print_int(PRINT_ANY, "ifindex", "%d: ", ifi->ifi_index);
-	if (tb[IFLA_IFNAME]) {
-		print_color_string(PRINT_ANY,
-				   COLOR_IFNAME,
-				   "ifname", "%s",
-				   rta_getattr_str(tb[IFLA_IFNAME]));
-	} else {
-		print_null(PRINT_JSON, "ifname", NULL, NULL);
-		print_color_null(PRINT_FP, COLOR_IFNAME,
-				 "ifname", "%s", "<nil>");
-	}
+	print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", "%s", name);
 
 	if (tb[IFLA_LINK]) {
 		int iflink = rta_getattr_u32(tb[IFLA_LINK]);
-- 
1.7.10.4

[PATCH iproute2-next 2/6] ipaddress: Simplify print_linkinfo_brief() and it's usage

[Serhey Popovych]

Improve print_linkinfo_brief() and it's callers:

  1) Get rid of custom @struct filter pointer @pfilter: it is NULL in
     all callers anyway and global @filter is used.

  2) Simplify calling code in ipaddr_list_flush_or_save() by
     introducing intermediate variable of @struct nlmsghdr, drop
     duplicated code: print_linkinfo_brief() never returns values other
     than <= 0 so we can move print_selected_addrinfo() outside of each
     block.

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

diff --git a/ip/ip_common.h b/ip/ip_common.h
index 3203f0c..f5adbad 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -30,8 +30,7 @@ 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,
-			 struct link_filter *filter);
+			 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 f8fd392..c15abd1 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -919,8 +919,7 @@ static void print_link_stats(FILE *fp, struct nlmsghdr *n)
 }
 
 int print_linkinfo_brief(const struct sockaddr_nl *who,
-			 struct nlmsghdr *n, void *arg,
-			 struct link_filter *pfilter)
+			 struct nlmsghdr *n, void *arg)
 {
 	FILE *fp = (FILE *)arg;
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
@@ -937,12 +936,9 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 	if (len < 0)
 		return -1;
 
-	if (!pfilter)
-		pfilter = &filter;
-
-	if (pfilter->ifindex && ifi->ifi_index != pfilter->ifindex)
+	if (filter.ifindex && ifi->ifi_index != filter.ifindex)
 		return -1;
-	if (pfilter->up && !(ifi->ifi_flags&IFF_UP))
+	if (filter.up && !(ifi->ifi_flags&IFF_UP))
 		return -1;
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
@@ -953,30 +949,30 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 		name = rta_getattr_str(tb[IFLA_IFNAME]);
 	}
 
-	if (pfilter->label &&
-	    (!pfilter->family || pfilter->family == AF_PACKET) &&
-	    fnmatch(pfilter->label, name, 0))
+	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 (pfilter->group != -1 && group != pfilter->group)
+		if (filter.group != -1 && group != filter.group)
 			return -1;
 	}
 
 	if (tb[IFLA_MASTER]) {
 		int master = rta_getattr_u32(tb[IFLA_MASTER]);
 
-		if (pfilter->master > 0 && master != pfilter->master)
+		if (filter.master > 0 && master != filter.master)
 			return -1;
-	} else if (pfilter->master > 0)
+	} else if (filter.master > 0)
 		return -1;
 
-	if (pfilter->kind && match_link_kind(tb, pfilter->kind, 0))
+	if (filter.kind && match_link_kind(tb, filter.kind, 0))
 		return -1;
 
-	if (pfilter->slave_kind && match_link_kind(tb, pfilter->slave_kind, 1))
+	if (filter.slave_kind && match_link_kind(tb, filter.slave_kind, 1))
 		return -1;
 
 	if (n->nlmsg_type == RTM_DELLINK)
@@ -1006,7 +1002,7 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-	if (pfilter->family == AF_PACKET) {
+	if (filter.family == AF_PACKET) {
 		SPRINT_BUF(b1);
 
 		if (tb[IFLA_ADDRESS]) {
@@ -1020,7 +1016,7 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 		}
 	}
 
-	if (pfilter->family == AF_PACKET) {
+	if (filter.family == AF_PACKET) {
 		print_link_flags(fp, ifi->ifi_flags, m_flag);
 		print_string(PRINT_FP, NULL, "%s", "\n");
 	}
@@ -2188,25 +2184,21 @@ static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 		ipaddr_filter(&linfo, ainfo);
 
 	for (l = linfo.head; l; l = l->next) {
-		int res = 0;
-		struct ifinfomsg *ifi = NLMSG_DATA(&l->h);
+		struct nlmsghdr *n = &l->h;
+		struct ifinfomsg *ifi = NLMSG_DATA(n);
+		int res;
 
 		open_json_object(NULL);
-		if (brief) {
-			if (print_linkinfo_brief(NULL, &l->h,
-						 stdout, NULL) == 0)
-				if (filter.family != AF_PACKET)
-					print_selected_addrinfo(ifi,
-								ainfo->head,
-								stdout);
-		} else if (no_link ||
-			   (res = print_linkinfo(NULL, &l->h, stdout)) >= 0) {
-			if (filter.family != AF_PACKET)
-				print_selected_addrinfo(ifi,
-							ainfo->head, stdout);
-			if (res > 0 && !do_link && show_stats)
-				print_link_stats(stdout, &l->h);
-		}
+		if (brief)
+			res = print_linkinfo_brief(NULL, n, stdout);
+		else if (no_link)
+			res = 0;
+		else
+			res = print_linkinfo(NULL, n, stdout);
+		if (res >= 0 && filter.family != AF_PACKET)
+			print_selected_addrinfo(ifi, ainfo->head, stdout);
+		if (res > 0 && !do_link && show_stats)
+			print_link_stats(stdout, n);
 		close_json_object();
 	}
 	fflush(stdout);
diff --git a/ip/iplink.c b/ip/iplink.c
index 230f4c5..882cd13 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1077,7 +1077,7 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 
 	open_json_object(NULL);
 	if (brief)
-		print_linkinfo_brief(NULL, answer, stdout, NULL);
+		print_linkinfo_brief(NULL, answer, stdout);
 	else
 		print_linkinfo(NULL, answer, stdout);
 	close_json_object();
-- 
1.7.10.4

[PATCH iproute2-next 3/6] lib: Correct object file dependencies

[Serhey Popovych]

Neither internal libnetlink nor libgenl depends on ll_map.o: prepare for
upcoming changes that brings much more cleaner dependency between
utils.o and ll_map.o.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 lib/Makefile |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Makefile b/lib/Makefile
index 7b34ed5..bab8cbf 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -3,11 +3,11 @@ include ../config.mk
 
 CFLAGS += -fPIC
 
-UTILOBJ = utils.o rt_names.o ll_types.o ll_proto.o ll_addr.o \
+UTILOBJ = utils.o rt_names.o ll_map.o ll_types.o ll_proto.o ll_addr.o \
 	inet_proto.o namespace.o json_writer.o json_print.o \
 	names.o color.o bpf.o exec.o fs.o
 
-NLOBJ=libgenl.o ll_map.o libnetlink.o
+NLOBJ=libgenl.o libnetlink.o
 
 all: libnetlink.a libutil.a
 
-- 
1.7.10.4

[PATCH iproute2-next 4/6] utils: Introduce and use get_ifname_rta()

[Serhey Popovych]

Be consistent in handling of IFLA_IFNAME attribute in all places: if
there is no attribute report bug to stderr and use ll_index_to_name() as
last measure to get name in "if%d" format instead of "<nil>".

Use check_ifname() to validate network device name: this catches both
unexpected return from kernel and ll_index_to_name().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 bridge/link.c   |    8 ++++----
 include/utils.h |    1 +
 ip/ipaddress.c  |   20 ++++++++------------
 lib/utils.c     |   19 +++++++++++++++++++
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/bridge/link.c b/bridge/link.c
index 870ebe0..a11cbb1 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -99,9 +99,10 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		   struct nlmsghdr *n, void *arg)
 {
 	FILE *fp = arg;
-	int len = n->nlmsg_len;
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
 	struct rtattr *tb[IFLA_MAX+1];
+	int len = n->nlmsg_len;
+	const char *name;
 
 	len -= NLMSG_LENGTH(sizeof(*ifi));
 	if (len < 0) {
@@ -117,10 +118,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 
 	parse_rtattr_flags(tb, IFLA_MAX, IFLA_RTA(ifi), len, NLA_F_NESTED);
 
-	if (tb[IFLA_IFNAME] == NULL) {
-		fprintf(stderr, "BUG: nil ifname\n");
+	name = get_ifname_rta(ifi->ifi_index, tb[IFLA_IFNAME]);
+	if (!name)
 		return -1;
-	}
 
 	if (n->nlmsg_type == RTM_DELLINK)
 		fprintf(fp, "Deleted ");
diff --git a/include/utils.h b/include/utils.h
index 0394268..5738c97 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -173,6 +173,7 @@ void duparg(const char *, const char *) __attribute__((noreturn));
 void duparg2(const char *, const char *) __attribute__((noreturn));
 int check_ifname(const char *);
 int get_ifname(char *, const char *);
+const char *get_ifname_rta(int ifindex, const struct rtattr *rta);
 int matches(const char *arg, const char *pattern);
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
 int inet_addr_match_rta(const inet_prefix *m, const struct rtattr *rta);
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index c15abd1..1797927 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -942,12 +942,10 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 		return -1;
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
-	if (tb[IFLA_IFNAME] == NULL) {
-		fprintf(stderr, "BUG: device with ifindex %d has nil ifname\n", ifi->ifi_index);
-		name = ll_index_to_name(ifi->ifi_index);
-	} else {
-		name = rta_getattr_str(tb[IFLA_IFNAME]);
-	}
+
+	name = get_ifname_rta(ifi->ifi_index, tb[IFLA_IFNAME]);
+	if (!name)
+		return -1;
 
 	if (filter.label &&
 	    (!filter.family || filter.family == AF_PACKET) &&
@@ -1069,12 +1067,10 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		return -1;
 
 	parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len);
-	if (tb[IFLA_IFNAME] == NULL) {
-		fprintf(stderr, "BUG: device with ifindex %d has nil ifname\n", ifi->ifi_index);
-		name = ll_index_to_name(ifi->ifi_index);
-	} else {
-		name = rta_getattr_str(tb[IFLA_IFNAME]);
-	}
+
+	name = get_ifname_rta(ifi->ifi_index, tb[IFLA_IFNAME]);
+	if (!name)
+		return -1;
 
 	if (filter.label &&
 	    (!filter.family || filter.family == AF_PACKET) &&
diff --git a/lib/utils.c b/lib/utils.c
index 8e15625..29e2d84 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -871,6 +871,25 @@ int get_ifname(char *buf, const char *name)
 	return ret;
 }
 
+const char *get_ifname_rta(int ifindex, const struct rtattr *rta)
+{
+	const char *name;
+
+	if (rta) {
+		name = rta_getattr_str(rta);
+	} else {
+		fprintf(stderr,
+			"BUG: device with ifindex %d has nil ifname\n",
+			ifindex);
+		name = ll_index_to_name(ifindex);
+	}
+
+	if (check_ifname(name))
+		return NULL;
+
+	return name;
+}
+
 int matches(const char *cmd, const char *pattern)
 {
 	int len = strlen(cmd);
-- 
1.7.10.4

[PATCH iproute2-next 5/6] utils: Introduce and use print_name_and_link() to print name@link

[Serhey Popovych]

There is at least three places implementing same things: two in
ipaddress.c print_linkinfo() & print_linkinfo_brief() and one in
bridge/link.c.

These two implementations diverge from each other very little:
bridge/link.c does not support JSON output at the moment and
print_linkinfo_brief() does not handle IFLA_LINK_NETNS case.

Introduce and use print_name_and_link() routine to handle name@link
output in all possible variations; respect IFLA_LINK_NETNS attribute to
handle case when link is in different namespace; use "if%d" template
for interface name instead of "<nil>" to share logic with other
code (e.g. ll_name_to_index() and ll_index_to_name()) supporting such
template.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 bridge/link.c   |   13 +++----------
 include/utils.h |    4 ++++
 ip/ipaddress.c  |   48 ++----------------------------------------------
 lib/utils.c     |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 56 deletions(-)

diff --git a/bridge/link.c b/bridge/link.c
index a11cbb1..90c9734 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -125,20 +125,13 @@ int print_linkinfo(const struct sockaddr_nl *who,
 	if (n->nlmsg_type == RTM_DELLINK)
 		fprintf(fp, "Deleted ");
 
-	fprintf(fp, "%d: %s ", ifi->ifi_index,
-		tb[IFLA_IFNAME] ? rta_getattr_str(tb[IFLA_IFNAME]) : "<nil>");
+	fprintf(fp, "%d: ", ifi->ifi_index);
+
+	print_name_and_link("%s: ", COLOR_NONE, name, tb);
 
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
 
-	if (tb[IFLA_LINK]) {
-		int iflink = rta_getattr_u32(tb[IFLA_LINK]);
-
-		fprintf(fp, "@%s: ",
-			iflink ? ll_index_to_name(iflink) : "NONE");
-	} else
-		fprintf(fp, ": ");
-
 	print_link_flags(fp, ifi->ifi_flags);
 
 	if (tb[IFLA_MTU])
diff --git a/include/utils.h b/include/utils.h
index 5738c97..d217073 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -12,6 +12,7 @@
 #include "libnetlink.h"
 #include "ll_map.h"
 #include "rtm_map.h"
+#include "json_print.h"
 
 extern int preferred_family;
 extern int human_readable;
@@ -240,6 +241,9 @@ void print_escape_buf(const __u8 *buf, size_t len, const char *escape);
 int print_timestamp(FILE *fp);
 void print_nlmsg_timestamp(FILE *fp, const struct nlmsghdr *n);
 
+unsigned int print_name_and_link(const char *fmt, enum color_attr color,
+				 const char *name, struct rtattr *tb[]);
+
 #define BIT(nr)                 (1UL << (nr))
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 1797927..5afc9d4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -926,7 +926,6 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 	struct rtattr *tb[IFLA_MAX+1];
 	int len = n->nlmsg_len;
 	const char *name;
-	char buf[32] = { 0, };
 	unsigned int m_flag = 0;
 
 	if (n->nlmsg_type != RTM_NEWLINK && n->nlmsg_type != RTM_DELLINK)
@@ -976,26 +975,7 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 	if (n->nlmsg_type == RTM_DELLINK)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
-	if (tb[IFLA_LINK]) {
-		SPRINT_BUF(b1);
-		int iflink = rta_getattr_u32(tb[IFLA_LINK]);
-
-		if (iflink == 0) {
-			snprintf(buf, sizeof(buf), "%s@NONE", name);
-			print_null(PRINT_JSON, "link", NULL, NULL);
-		} else {
-			const char *link = ll_idx_n2a(iflink, b1);
-
-			print_string(PRINT_JSON, "link", NULL, link);
-			snprintf(buf, sizeof(buf), "%s@%s", name, link);
-			m_flag = ll_index_to_flags(iflink);
-			m_flag = !(m_flag & IFF_UP);
-		}
-	} else
-		snprintf(buf, sizeof(buf), "%s", name);
-
-	print_string(PRINT_FP, NULL, "%-16s ", buf);
-	print_string(PRINT_JSON, "ifname", NULL, name);
+	m_flag = print_name_and_link("%-16s ", COLOR_NONE, name, tb);
 
 	if (tb[IFLA_OPERSTATE])
 		print_operstate(fp, rta_getattr_u8(tb[IFLA_OPERSTATE]));
@@ -1102,31 +1082,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
 	print_int(PRINT_ANY, "ifindex", "%d: ", ifi->ifi_index);
-	print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", "%s", name);
-
-	if (tb[IFLA_LINK]) {
-		int iflink = rta_getattr_u32(tb[IFLA_LINK]);
-
-		if (iflink == 0)
-			print_null(PRINT_ANY, "link", "@%s: ", "NONE");
-		else {
-			if (tb[IFLA_LINK_NETNSID])
-				print_int(PRINT_ANY,
-					  "link_index", "@if%d: ", iflink);
-			else {
-				SPRINT_BUF(b1);
-
-				print_string(PRINT_ANY,
-					     "link",
-					     "@%s: ",
-					     ll_idx_n2a(iflink, b1));
-				m_flag = ll_index_to_flags(iflink);
-				m_flag = !(m_flag & IFF_UP);
-			}
-		}
-	} else {
-		print_string(PRINT_FP, NULL, ": ", NULL);
-	}
+	m_flag = print_name_and_link("%s: ", COLOR_IFNAME, name, tb);
 	print_link_flags(fp, ifi->ifi_flags, m_flag);
 
 	if (tb[IFLA_MTU])
diff --git a/lib/utils.c b/lib/utils.c
index 29e2d84..c39ee50 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1260,6 +1260,57 @@ int print_timestamp(FILE *fp)
 	return 0;
 }
 
+unsigned int print_name_and_link(const char *fmt, enum color_attr color,
+				 const char *name, struct rtattr *tb[])
+{
+	const char *link = NULL;
+	unsigned int m_flag = 0;
+	SPRINT_BUF(b1);
+	SPRINT_BUF(b2);
+
+	if (tb[IFLA_LINK]) {
+		int iflink = rta_getattr_u32(tb[IFLA_LINK]);
+
+		if (iflink) {
+			if (tb[IFLA_LINK_NETNSID]) {
+				if (is_json_context()) {
+					print_int(PRINT_JSON,
+						  "link_index", NULL, iflink);
+				} else {
+					snprintf(b2, sizeof(b2),
+						 "if%d", iflink);
+					link = b2;
+				}
+			} else {
+				link = ll_index_to_name(iflink);
+
+				if (is_json_context()) {
+					print_string(PRINT_JSON,
+						     "link", NULL, link);
+					link = NULL;
+				}
+
+				m_flag = ll_index_to_flags(iflink);
+				m_flag = !(m_flag & IFF_UP);
+			}
+		} else {
+			if (is_json_context())
+				print_null(PRINT_JSON, "link", NULL, NULL);
+			else
+				link = "NONE";
+		}
+
+		if (link) {
+			snprintf(b1, sizeof(b1), "%s@%s", name, link);
+			name = b1;
+		}
+	}
+
+	print_color_string(PRINT_ANY, color, "ifname", fmt, name);
+
+	return m_flag;
+}
+
 int cmdlineno;
 
 /* Like glibc getline but handle continuation lines and comments */
-- 
1.7.10.4

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

[Serhey Popovych]

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(-)

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);
-- 
1.7.10.4

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

[Stephen Hemminger]

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.

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

[Serhey Popovych]

[-- Attachment #1.1: Type: text/plain, Size: 1168 bytes --]

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.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]