[PATCH iproute2-next v2 0/9] With this series I propose to make print_linkinfo_brief() static in

[PATCH iproute2-next v2 0/9] With this series I propose to make print_linkinfo_brief() static in

[Serhey Popovych]

Changes presented with this series tested using following script:

\#!/bin/bash

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} |sed -n -Ee'/^(-{3}|\+{3})[[:space:]]+/!p'
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
  Rebased to current iproute2-next/master. No changes.

Thanks,
Serhii

Serhey Popovych (9):
  ipaddress: Abstract IFA_LABEL matching code
  ipaddress: ll_map: Replace ll_idx_n2a() with ll_index_to_name()
  utils: Reimplement ll_idx_n2a() and introduce ll_idx_a2n()
  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/ll_map.h |    4 +-
 include/utils.h  |    5 ++
 ip/ip_common.h   |    2 -
 ip/ipaddress.c   |  224 ++++++++++++++----------------------------------------
 ip/iplink.c      |    5 +-
 lib/Makefile     |    4 +-
 lib/ll_map.c     |   31 +++++---
 lib/utils.c      |   68 +++++++++++++++++
 9 files changed, 162 insertions(+), 202 deletions(-)

-- 
1.7.10.4

[PATCH iproute2-next v2 1/9] ipaddress: Abstract IFA_LABEL matching code

[Serhey Popovych]

There at least two places in ip/ipaddress.c where we match IFA_LABEL
against filter.label if that is given.

Get rid of "common" if () statement for inet_addr_match_rta() and
ifa_label_match_rta(): it is not common because first will check for
filter.pfx.family != AF_UNSPEC inside and second for filter.label being
non NULL.

This allows us to further simplify down code and prepare for
ll_idx_n2a() replacement with ll_index_to_name() without 80 columns
checkpatch notice.

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

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 6990b81..ad69d09 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1305,6 +1305,22 @@ static int get_filter(const char *arg)
 	return 0;
 }
 
+static int ifa_label_match_rta(int ifindex, const struct rtattr *rta)
+{
+	const char *label;
+	SPRINT_BUF(b1);
+
+	if (!filter.label)
+		return 0;
+
+	if (rta)
+		label = RTA_DATA(rta);
+	else
+		label = ll_idx_n2a(ifindex, b1);
+
+	return fnmatch(filter.label, label, 0);
+}
+
 int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		   void *arg)
 {
@@ -1343,21 +1359,13 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		return 0;
 	if ((filter.flags ^ ifa_flags) & filter.flagmask)
 		return 0;
-	if (filter.label) {
-		SPRINT_BUF(b1);
-		const char *label;
-
-		if (rta_tb[IFA_LABEL])
-			label = RTA_DATA(rta_tb[IFA_LABEL]);
-		else
-			label = ll_idx_n2a(ifa->ifa_index, b1);
-		if (fnmatch(filter.label, label, 0) != 0)
-			return 0;
-	}
 
 	if (filter.family && filter.family != ifa->ifa_family)
 		return 0;
 
+	if (ifa_label_match_rta(ifa->ifa_index, rta_tb[IFA_LABEL]))
+		return 0;
+
 	if (inet_addr_match_rta(&filter.pfx, rta_tb[IFA_LOCAL]))
 		return 0;
 
@@ -1713,25 +1721,14 @@ static void ipaddr_filter(struct nlmsg_chain *linfo, struct nlmsg_chain *ainfo)
 
 			if ((filter.flags ^ ifa_flags) & filter.flagmask)
 				continue;
-			if (filter.pfx.family || filter.label) {
-				struct rtattr *rta =
-					tb[IFA_LOCAL] ? : tb[IFA_ADDRESS];
-
-				if (inet_addr_match_rta(&filter.pfx, rta))
-					continue;
-
-				if (filter.label) {
-					SPRINT_BUF(b1);
-					const char *label;
-
-					if (tb[IFA_LABEL])
-						label = RTA_DATA(tb[IFA_LABEL]);
-					else
-						label = ll_idx_n2a(ifa->ifa_index, b1);
-					if (fnmatch(filter.label, label, 0) != 0)
-						continue;
-				}
-			}
+
+			if (ifa_label_match_rta(ifa->ifa_index, tb[IFA_LABEL]))
+				continue;
+
+			if (!tb[IFA_LOCAL])
+				tb[IFA_LOCAL] = tb[IFA_ADDRESS];
+			if (inet_addr_match_rta(&filter.pfx, tb[IFA_LOCAL]))
+				continue;
 
 			ok = 1;
 			break;
-- 
1.7.10.4

[PATCH iproute2-next v2 2/9] ipaddress: ll_map: Replace ll_idx_n2a() with ll_index_to_name()

[Serhey Popovych]

There is no reentrancy as well as deferred result usage for all cases
where ll_idx_n2a() being used: it is safe to use ll_index_to_name() that
internally calls ll_idx_n2a() with static buffer to hold result.

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

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index ad69d09..6b8295d 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -813,14 +813,13 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
 		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);
+			const char *link = ll_index_to_name(iflink);
 
 			print_string(PRINT_JSON, "link", NULL, link);
 			snprintf(buf, sizeof(buf), "%s@%s", name, link);
@@ -957,12 +956,10 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				print_int(PRINT_ANY,
 					  "link_index", "@if%d: ", iflink);
 			else {
-				SPRINT_BUF(b1);
-
 				print_string(PRINT_ANY,
 					     "link",
 					     "@%s: ",
-					     ll_idx_n2a(iflink, b1));
+					     ll_index_to_name(iflink));
 				m_flag = ll_index_to_flags(iflink);
 				m_flag = !(m_flag & IFF_UP);
 			}
@@ -984,12 +981,12 @@ int print_linkinfo(const struct sockaddr_nl *who,
 			     "qdisc %s ",
 			     rta_getattr_str(tb[IFLA_QDISC]));
 	if (tb[IFLA_MASTER]) {
-		SPRINT_BUF(b1);
+		int master = rta_getattr_u32(tb[IFLA_MASTER]);
 
 		print_string(PRINT_ANY,
 			     "master",
 			     "master %s ",
-			     ll_idx_n2a(rta_getattr_u32(tb[IFLA_MASTER]), b1));
+			     ll_index_to_name(master));
 	}
 
 	if (tb[IFLA_OPERSTATE])
@@ -1308,7 +1305,6 @@ static int get_filter(const char *arg)
 static int ifa_label_match_rta(int ifindex, const struct rtattr *rta)
 {
 	const char *label;
-	SPRINT_BUF(b1);
 
 	if (!filter.label)
 		return 0;
@@ -1316,7 +1312,7 @@ static int ifa_label_match_rta(int ifindex, const struct rtattr *rta)
 	if (rta)
 		label = RTA_DATA(rta);
 	else
-		label = ll_idx_n2a(ifindex, b1);
+		label = ll_index_to_name(ifindex);
 
 	return fnmatch(filter.label, label, 0);
 }
-- 
1.7.10.4

[PATCH iproute2-next v2 3/9] utils: Reimplement ll_idx_n2a() and introduce ll_idx_a2n()

[Serhey Popovych]

Now all users of ll_idx_n2a() replaced with ll_index_to_name() we can
move it's functionality to ll_index_to_name() and implement index to
name conversion using snprintf() and "if%u".

Use %u specifier in "if%..." template consistently: network device
indexes are always greather than zero.

Also introduce ll_idx_n2a() conterpart: ll_idx_a2n() that is used
to translate name of the "if%u" form to index using sscanf().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 include/ll_map.h |    4 +++-
 lib/ll_map.c     |   31 +++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/include/ll_map.h b/include/ll_map.h
index c8474e6..8546ff9 100644
--- a/include/ll_map.h
+++ b/include/ll_map.h
@@ -8,9 +8,11 @@ int ll_remember_index(const struct sockaddr_nl *who,
 void ll_init_map(struct rtnl_handle *rth);
 unsigned ll_name_to_index(const char *name);
 const char *ll_index_to_name(unsigned idx);
-const char *ll_idx_n2a(unsigned idx, char *buf);
 int ll_index_to_type(unsigned idx);
 int ll_index_to_flags(unsigned idx);
 unsigned namehash(const char *str);
 
+const char *ll_idx_n2a(unsigned int idx);
+unsigned int ll_idx_a2n(const char *name);
+
 #endif /* __LL_MAP_H__ */
diff --git a/lib/ll_map.c b/lib/ll_map.c
index f65614f..0afe689 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c
@@ -136,8 +136,26 @@ int ll_remember_index(const struct sockaddr_nl *who,
 	return 0;
 }
 
-const char *ll_idx_n2a(unsigned idx, char *buf)
+const char *ll_idx_n2a(unsigned int idx)
 {
+	static char buf[IFNAMSIZ];
+
+	snprintf(buf, sizeof(buf), "if%u", idx);
+	return buf;
+}
+
+unsigned int ll_idx_a2n(const char *name)
+{
+	unsigned int idx;
+
+	if (sscanf(name, "if%u", &idx) != 1)
+		return 0;
+	return idx;
+}
+
+const char *ll_index_to_name(unsigned int idx)
+{
+	static char buf[IFNAMSIZ];
 	const struct ll_cache *im;
 
 	if (idx == 0)
@@ -148,18 +166,11 @@ const char *ll_idx_n2a(unsigned idx, char *buf)
 		return im->name;
 
 	if (if_indextoname(idx, buf) == NULL)
-		snprintf(buf, IFNAMSIZ, "if%d", idx);
+		snprintf(buf, IFNAMSIZ, "if%u", idx);
 
 	return buf;
 }
 
-const char *ll_index_to_name(unsigned idx)
-{
-	static char nbuf[IFNAMSIZ];
-
-	return ll_idx_n2a(idx, nbuf);
-}
-
 int ll_index_to_type(unsigned idx)
 {
 	const struct ll_cache *im;
@@ -196,7 +207,7 @@ unsigned ll_name_to_index(const char *name)
 
 	idx = if_nametoindex(name);
 	if (idx == 0)
-		sscanf(name, "if%u", &idx);
+		idx = ll_idx_a2n(name);
 	return idx;
 }
 
-- 
1.7.10.4

[PATCH iproute2-next v2 4/9] 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_idx_n2a() as last resort to translate name to index for
     "should never happen" cases when cache shouldn't be considered

  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 6b8295d..6ada993 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -778,14 +778,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_idx_n2a(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))
+	    fnmatch(filter.label, name, 0))
 		return -1;
 
 	if (tb[IFLA_GROUP]) {
@@ -887,6 +887,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)
@@ -897,18 +898,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_idx_n2a(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]);
@@ -935,16 +940,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 5/9] ipaddress: Simplify print_linkinfo_brief() and it's usage

[Serhey Popovych]

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/ipaddress.c |   31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 6ada993..1b4586c 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -753,7 +753,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 nlmsghdr *n, void *arg)
 {
 	FILE *fp = (FILE *)arg;
 	struct ifinfomsg *ifi = NLMSG_DATA(n);
@@ -2012,24 +2012,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) == 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);
-- 
1.7.10.4

[PATCH iproute2-next v2 6/9] 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 7/9] 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_idx_n2a() as
last measure to get name in "if%u" format instead of "<nil>".

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

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 867e540..84ca873 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -183,6 +183,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 1b4586c..670d8e0 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -776,12 +776,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_idx_n2a(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) &&
@@ -903,12 +901,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_idx_n2a(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 d86c2ee..572d42a 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_idx_n2a(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 8/9] 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.

They are 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 ll_idx_n2a() 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  |   44 ++------------------------------------------
 lib/utils.c     |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 52 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 84ca873..75ddb4a 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;
@@ -250,6 +251,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 670d8e0..e14a59e 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -760,7 +760,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)
@@ -810,25 +809,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]) {
-		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_index_to_name(iflink);
-
-			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]));
@@ -936,29 +917,8 @@ 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 {
-				print_string(PRINT_ANY,
-					     "link",
-					     "@%s: ",
-					     ll_index_to_name(iflink));
-				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 572d42a..0f9523c 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -33,6 +33,7 @@
 
 #include "rt_names.h"
 #include "utils.h"
+#include "ll_map.h"
 #include "namespace.h"
 
 int resolve_hosts;
@@ -1260,6 +1261,54 @@ 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);
+
+	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 {
+					link = ll_idx_n2a(iflink);
+				}
+			} 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 9/9] 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 |   76 ++++++++------------------------------------------------
 ip/iplink.c    |    5 +---
 3 files changed, 11 insertions(+), 72 deletions(-)

diff --git a/ip/ip_common.h b/ip/ip_common.h
index 1397d99..e4e628b 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 e14a59e..e804487 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -752,63 +752,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])
@@ -868,6 +817,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;
@@ -916,6 +866,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);
@@ -948,7 +901,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,
@@ -964,8 +916,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		print_link_event(fp, rta_getattr_u32(tb[IFLA_EVENT]));
 
 	if (!filter.family || filter.family == AF_PACKET || show_details) {
-		SPRINT_BUF(b1);
-
 		print_string(PRINT_FP, NULL, "%s", _SL_);
 		print_string(PRINT_ANY,
 			     "link_type",
@@ -1065,7 +1015,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
 				     rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
 
 		if (tb[IFLA_PHYS_PORT_ID]) {
-			SPRINT_BUF(b1);
 			print_string(PRINT_ANY,
 				     "phys_port_id",
 				     "portid %s ",
@@ -1076,7 +1025,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
 		}
 
 		if (tb[IFLA_PHYS_SWITCH_ID]) {
-			SPRINT_BUF(b1);
 			print_string(PRINT_ANY,
 				     "phys_switch_id",
 				     "switchid %s ",
@@ -1115,7 +1063,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;
 }
@@ -1970,14 +1918,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 3d7f7fa..74c377c 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1075,10 +1075,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 0/9] With this series I propose to make print_linkinfo_brief() static in

[Serhey Popovych]

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

Please ignore this series: wrong subject.

> Changes presented with this series tested using following script:
> 
> \#!/bin/bash
> 
> 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} |sed -n -Ee'/^(-{3}|\+{3})[[:space:]]+/!p'
> 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
>   Rebased to current iproute2-next/master. No changes.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (9):
>   ipaddress: Abstract IFA_LABEL matching code
>   ipaddress: ll_map: Replace ll_idx_n2a() with ll_index_to_name()
>   utils: Reimplement ll_idx_n2a() and introduce ll_idx_a2n()
>   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/ll_map.h |    4 +-
>  include/utils.h  |    5 ++
>  ip/ip_common.h   |    2 -
>  ip/ipaddress.c   |  224 ++++++++++++++----------------------------------------
>  ip/iplink.c      |    5 +-
>  lib/Makefile     |    4 +-
>  lib/ll_map.c     |   31 +++++---
>  lib/utils.c      |   68 +++++++++++++++++
>  9 files changed, 162 insertions(+), 202 deletions(-)
> 



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

Re: [PATCH iproute2-next v2 2/9] ipaddress: ll_map: Replace ll_idx_n2a() with ll_index_to_name()

[Stephen Hemminger]

On Wed, 14 Feb 2018 23:33:37 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

>  	if (tb[IFLA_MASTER]) {
> -		SPRINT_BUF(b1);
> +		int master = rta_getattr_u32(tb[IFLA_MASTER]);
>  
>  		print_string(PRINT_ANY,
>  			     "master",
>  			     "master %s ",
> -			     ll_idx_n2a(rta_getattr_u32(tb[IFLA_MASTER]), b1));
> +			     ll_index_to_name(master));

Since this is an interface name, it should be printed in the correct color.
Sorry, missed that in the recent changes.