Re: [PATCH iproute2-next v2 1/6] ipaddress: Improve print_linkinfo()
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 = ""; >> +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 "" 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_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", ""); >> -} >> +print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", "%s", name); >> >> if (tb[IFLA_LINK]) { >> int iflink = rta_getattr_u32(tb[IFLA_LINK]); >> > signature.asc Description: OpenPGP digital signature
Re: [PATCH iproute2-next v2 1/6] ipaddress: Improve print_linkinfo()
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 = ""; > + 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_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", ""); > - } > + print_color_string(PRINT_ANY, COLOR_IFNAME, "ifname", "%s", name); > > if (tb[IFLA_LINK]) { > int iflink = rta_getattr_u32(tb[IFLA_LINK]); >
[PATCH iproute2-next v2 1/6] ipaddress: Improve print_linkinfo()
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--- ip/ipaddress.c | 30 +- 1 file changed, 13 insertions(+), 17 deletions(-) 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 = ""; + 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_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", ""); - } + 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