Re: [PATCH iproute2-next v4 7/9] utils: Introduce and use get_ifname_rta()

[David Ahern <dsahern@gmail.com>]

On 2/15/18 11:34 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_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 749178d..08d2576 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);
> 


Getting build failures with this one:
$ make clean; make -j 8
...

devlink
    CC       devlink.o
    CC       mnlg.o
    LINK     devlink
../lib/libutil.a(ll_map.o): In function `ll_remember_index':
ll_map.c:(.text+0xf6): undefined reference to `parse_rtattr'
ll_map.c:(.text+0x1c5): undefined reference to `parse_rtattr'
../lib/libutil.a(ll_map.o): In function `ll_init_map':
ll_map.c:(.text+0x47c): undefined reference to `rtnl_wilddump_request'
ll_map.c:(.text+0x493): undefined reference to `rtnl_dump_filter_nc'