| Message ID | 1518719648-30134-8-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Ahern |
| Headers | show |
| Series | ipaddress: Make print_linkinfo_brief() static | expand |
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'
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 "<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' > > Corrected, sorry for that. It is actually caused by "lib: Correct object file dependencies".
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);
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(-)