Re: [PATCH iproute2-next v4 7/9] utils: Introduce and use get_ifname_rta()
David Ahern wrote:
> 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 "".
>>
>> Use check_ifname() to validate network device name: this catches both
>> unexpected return from kernel and ll_idx_n2a().
>>
>> Signed-off-by: Serhey Popovych
>> ---
>> 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_re
Re: [PATCH iproute2-next v4 7/9] utils: Introduce and use get_ifname_rta()
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 "".
>
> Use check_ifname() to validate network device name: this catches both
> unexpected return from kernel and ll_idx_n2a().
>
> Signed-off-by: Serhey Popovych
> ---
> 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'
[PATCH iproute2-next v4 7/9] utils: Introduce and use get_ifname_rta()
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 "".
Use check_ifname() to validate network device name: this catches both
unexpected return from kernel and ll_idx_n2a().
Signed-off-by: Serhey Popovych
---
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);
--
1.7.10.4
