| Message ID | 1517335761-22095-2-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Ahern |
| Headers | show |
| Series | ipaddress: Get rid of print_linkinfo_brief() | expand |
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]); >
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]); >> >
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]);
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(-)