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

[PATCH iproute2-next v2 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.

v2
  Make print_linkinfo_brief() static instead of inlining it's code into
  print_linkinfo(). Better for review, better for code style, compiler
  will optimize this anyway.

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: Make print_linkinfo_brief() static

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

-- 
1.7.10.4

[PATCH iproute2-next v2 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 v2 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 v2 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 v2 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 v2 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 v2 6/6] ipaddress: Make print_linkinfo_brief() static

[Serhey Popovych]

It shares lot of code with print_linkinfo(): drop duplicated part,
change parameters list, make it static and call from print_linkinfo()
after common path.

While there move SPRINT_BUF() to the function scope from blocks to
avoid duplication and use "%s" to print "\n" to help compiler optimize
exit for both print_linkinfo_brief() and normal paths.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/ip_common.h |    2 --
 ip/ipaddress.c |   74 ++++++++------------------------------------------------
 ip/iplink.c    |    5 +---
 3 files changed, 11 insertions(+), 70 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..450d3cc 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -918,63 +918,12 @@ 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)
+static int print_linkinfo_brief(FILE *fp, const char *name,
+				const struct ifinfomsg *ifi,
+				struct rtattr *tb[])
 {
-	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])
@@ -1033,6 +982,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 +1031,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
 	if (n->nlmsg_type == RTM_DELLINK)
 		print_bool(PRINT_ANY, "deleted", "Deleted ", true);
 
+	if (brief)
+		return print_linkinfo_brief(fp, name, ifi, tb);
+
 	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 +1050,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 +1063,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 +1229,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 +2088,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 v2 1/6] ipaddress: Improve print_linkinfo()

[David Ahern]

On 1/30/18 11:09 AM, Serhey Popovych wrote:

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

This is one of those "should never happen checks" since the kernel
always adds IFLA_IFNAME. Going to a cache to get a name for the index
when the existing message is missing that attribute seems wrong. I
realize the expectation is that the cache is empty today so if%d is
returned, but that could change and it is the idea of consulting a cache
that I think is wrong.

If that intention is to have a name I think it is safer to just have it
set to if%d here (or in a helper that the cache also uses).

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

same here


> +	} 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]);
> 

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

[David Ahern]

On 1/30/18 11:09 AM, Serhey Popovych wrote:
> 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.
> 

Looks like I somehow dropped a vrf change that was going to use that.
Send a standalone patch to revert 63891c70137f (straight revert does not
work so some fixup is needed) and then a follow on for the second part
of this change.

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

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

[David Ahern]

On 1/30/18 11:09 AM, Serhey Popovych wrote:
> 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().

same comment as patch 1. IFLA_IFNAME is always sent by the kernel and
doing a lookup in a cache is wrong.

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

[David Ahern]

On 1/30/18 11:09 AM, Serhey Popovych wrote:
> 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);

It only needs tb[IFLA_LINK] so just pass it. Makes the arg list
consistent with the function name too.

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

[David Ahern]

On 1/30/18 11:09 AM, Serhey Popovych wrote:
> With this series I propose to get rid of custom print_linkinfo_brief()
> in favor of print_linkinfo() to avoid code duplication.
> 

Cover letter needs updating in light of the change to patch 6

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

[Serhey Popovych]

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

David Ahern wrote:
> On 1/30/18 11:09 AM, Serhey Popovych wrote:
> 
>> 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);
> 
> This is one of those "should never happen checks" since the kernel
> always adds IFLA_IFNAME. Going to a cache to get a name for the index
> when the existing message is missing that attribute seems wrong. I
> realize the expectation is that the cache is empty today so if%d is
> returned, but that could change and it is the idea of consulting a cache
> that I think is wrong.
> 
> If that intention is to have a name I think it is safer to just have it
> set to if%d here (or in a helper that the cache also uses).

In my opinion in print_*() functions it is expected that cache isn't
empty. That's actually point where we benefit from cache.

I understand side effects this change may introduce and your concerns
about them. Will present small set of changes to export "if%d" template
from lib/ll_map.c for use here.

Using "<nil>" of something other, even for "should never happen" case
seems wrong to me.

> 
>>  	} 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);
> 
> same here
> 
> 
>> +	} 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]);
>>
> 



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

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

[Serhey Popovych]

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

David Ahern wrote:
> On 1/30/18 11:09 AM, Serhey Popovych wrote:
>> 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.
>>
> 
> Looks like I somehow dropped a vrf change that was going to use that.
> Send a standalone patch to revert 63891c70137f (straight revert does not
> work so some fixup is needed) and then a follow on for the second part
> of this change.

Accepted, will send revert of described commit separately and then
rebase whole series on top of it.

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



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

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

[Serhey Popovych]

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

David Ahern wrote:
> On 1/30/18 11:09 AM, Serhey Popovych wrote:
>> 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);
> 
> It only needs tb[IFLA_LINK] so just pass it. Makes the arg list
> consistent with the function name too.
> 

Unfortunately not only: it uses IFLA_LINK_NETNSID too. May be adding
"_rta" suffix to this routine as we did in the past when introducing
get_addr_rta() and similar routines? In my opinion this is preferred
than adding one more parameters to the routine.

On the other hand tb[IFLA_LINK] taken by rta_getaddr_u32() (but actually
it is "int" with values strictly greater than zero as implemented in
kernel), so making function to catch missing tb[IFLA_LINK] would require
some helper and may broke "should never happen" case when
(int)rta_getattr_u32(tb[IFLA_LINK]) < 0.

Currently we may call ll_index_to_name() with negative iflink.


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

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

[Serhey Popovych]

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

David Ahern wrote:

> On 1/30/18 11:09 AM, Serhey Popovych wrote:
>> With this series I propose to get rid of custom print_linkinfo_brief()
>> in favor of print_linkinfo() to avoid code duplication.
>>
> 
> Cover letter needs updating in light of the change to patch 6
> 

Agree, will resend series with different cover letter and patch series
after small dependency series and revert of commit 63891c70137f ("ip
address: Change print_linkinfo_brief to take filter as an input").


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

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

[David Ahern]

On 2/1/18 4:09 AM, Serhey Popovych wrote:
> David Ahern wrote:
>>> 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);
>>
>> It only needs tb[IFLA_LINK] so just pass it. Makes the arg list
>> consistent with the function name too.
>>
> 
> Unfortunately not only: it uses IFLA_LINK_NETNSID too. May be adding
> "_rta" suffix to this routine as we did in the past when introducing
> get_addr_rta() and similar routines? In my opinion this is preferred
> than adding one more parameters to the routine.

ok, tb arg is fine. I missed the IFLA_LINK_NETNSID case.