| Message ID | 20180711113603.16589-1-vincent@bernat.im |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] ipaddress: fix label matching | expand |
On Wed, 11 Jul 2018 13:36:03 +0200 Vincent Bernat <vincent@bernat.im> wrote: > Since 9516823051ce, "ip addr show label lo:1" doesn't work > anymore (doesn't show any address, despite a matching label). > Reverting to return 0 instead of -1 fix the issue. > > However, the condition says: "if we filter by label [...] and the > label does NOT match the interface name". This makes little sense to > compare the label with the interface name. There is also a logic > around filter family being provided or not. The match against the > label is done by ifa_label_match_rta() in print_addrinfo() and > ipaddr_filter(). > > Just removing the condition makes "ip addr show" works as expected > with or without specifying a label, both when the label is matching > and not matching. It also works if we specify a label and the label is > the interface name. The flush operation also works as expected. > > Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()") > Signed-off-by: Vincent Bernat <vincent@bernat.im> > --- > ip/ipaddress.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/ip/ipaddress.c b/ip/ipaddress.c > index 5009bfe6d2e3..20ef6724944e 100644 > --- a/ip/ipaddress.c > +++ b/ip/ipaddress.c > @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, > if (!name) > return -1; > > - if (filter.label && > - (!filter.family || filter.family == AF_PACKET) && > - fnmatch(filter.label, name, 0)) > - return -1; > - > if (tb[IFLA_GROUP]) { > int group = rta_getattr_u32(tb[IFLA_GROUP]); > If this is a regression, it should go to iproute2 not iproute2-next. Surprised by the solution since it is removing code that was there before the commit you referenced in Fixes.
❦ 11 juillet 2018 13:03 -0700, Stephen Hemminger <stephen@networkplumber.org> : >> Since 9516823051ce, "ip addr show label lo:1" doesn't work >> anymore (doesn't show any address, despite a matching label). >> Reverting to return 0 instead of -1 fix the issue. >> >> However, the condition says: "if we filter by label [...] and the >> label does NOT match the interface name". This makes little sense to >> compare the label with the interface name. There is also a logic >> around filter family being provided or not. The match against the >> label is done by ifa_label_match_rta() in print_addrinfo() and >> ipaddr_filter(). >> >> Just removing the condition makes "ip addr show" works as expected >> with or without specifying a label, both when the label is matching >> and not matching. It also works if we specify a label and the label is >> the interface name. The flush operation also works as expected. >> >> Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()") >> Signed-off-by: Vincent Bernat <vincent@bernat.im> >> --- >> ip/ipaddress.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/ip/ipaddress.c b/ip/ipaddress.c >> index 5009bfe6d2e3..20ef6724944e 100644 >> --- a/ip/ipaddress.c >> +++ b/ip/ipaddress.c >> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, >> if (!name) >> return -1; >> >> - if (filter.label && >> - (!filter.family || filter.family == AF_PACKET) && >> - fnmatch(filter.label, name, 0)) >> - return -1; >> - >> if (tb[IFLA_GROUP]) { >> int group = rta_getattr_u32(tb[IFLA_GROUP]); >> > > If this is a regression, it should go to iproute2 not iproute2-next. > > Surprised by the solution since it is removing code that was there > before the commit you referenced in Fixes. Yes, but as I explain in the commit message, the condition does not make sense for me: why would we match the label against the interface name? This code exists since a long time.
On 7/11/18 7:36 AM, Vincent Bernat wrote: > diff --git a/ip/ipaddress.c b/ip/ipaddress.c > index 5009bfe6d2e3..20ef6724944e 100644 > --- a/ip/ipaddress.c > +++ b/ip/ipaddress.c > @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, > if (!name) > return -1; > > - if (filter.label && > - (!filter.family || filter.family == AF_PACKET) && > - fnmatch(filter.label, name, 0)) > - return -1; > - The offending commit changed the return code: 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; Vincent: can you try leaving the code as is, but change the return to 0?
❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> : >> +++ b/ip/ipaddress.c >> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, >> if (!name) >> return -1; >> >> - if (filter.label && >> - (!filter.family || filter.family == AF_PACKET) && >> - fnmatch(filter.label, name, 0)) >> - return -1; >> - > > The offending commit changed the return code: > > 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; > > > Vincent: can you try leaving the code as is, but change the return to 0? Yes, it works by just returning 0. The code still doesn't make sense.
Vincent Bernat wrote: > ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> : > >>> +++ b/ip/ipaddress.c >>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, >>> if (!name) >>> return -1; >>> >>> - if (filter.label && >>> - (!filter.family || filter.family == AF_PACKET) && >>> - fnmatch(filter.label, name, 0)) >>> - return -1; >>> - >> >> The offending commit changed the return code: >> >> 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; >> >> >> Vincent: can you try leaving the code as is, but change the return to 0? > > Yes, it works by just returning 0. The code still doesn't make sense. > I think return code is correct. Check presented by this code too because print_linkinfo() isn't static and called from ipmonitor.c where no ipaddr_filter() or similar call that filters by label present. Instead fnmatch() compares interface *name*, not label from IFA_LABEL attribute. Thus: fnmatch(pattern, string, flags) -> fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1) Assuming above I would like to see ifa_label_match_rta() instead of open coded checks for filter.label with fmatch() in print_linkinfo(). Also it might be good idea to pass @name from get_ifname_rta() (like we do in print_linkinfo()) to ifa_label_match_rta() so that we respect IFLA_IFNAME if present.
Serhey Popovych wrote: > Vincent Bernat wrote: >> ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@gmail.com> : >> >>>> +++ b/ip/ipaddress.c >>>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, >>>> if (!name) >>>> return -1; >>>> >>>> - if (filter.label && >>>> - (!filter.family || filter.family == AF_PACKET) && >>>> - fnmatch(filter.label, name, 0)) >>>> - return -1; >>>> - >>> >>> The offending commit changed the return code: >>> >>> 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; >>> >>> >>> Vincent: can you try leaving the code as is, but change the return to 0? >> >> Yes, it works by just returning 0. The code still doesn't make sense. >> > > I think return code is correct. Check presented by this code too because > print_linkinfo() isn't static and called from ipmonitor.c where no > ipaddr_filter() or similar call that filters by label present. Ok, did more deep analysis of code. Vincent, David: we should return 0 as done before 9516823051ce. This is special case to return from print_linkinfo() earlier and match only filter.ifindex and filter.up if given and not rest fields in @filter. Then call print_selected_addrinfo() without calling print_link_stats() in ipaddr_list_flush_or_save(). Later print_selected_addrinfo() calls print_addrinfo() that finally matches filter.label using ifa_label_match_rta(). > > Instead fnmatch() compares interface *name*, not label from IFA_LABEL > attribute. Thus: > > fnmatch(pattern, string, flags) -> > fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1) This still incorrect: we should not call fnmatch() with network device name. Also ip-link(8) does not say anything that label could be used to filter link output. Label is ip-address(8) specific. Therefore checking filter.family == AF_PACKET looks incorrect. AF_PACKET is ip-link(8) specific. Checking against !filter.family (AF_UNSPEC) is incorrect too: user might force address family at ip command line and we never get: ip -4 addr show label lo:1 So from the code: if (filter.label && (!filter.family || filter.family == AF_PACKET) && fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0)) return -1; We should leave only filter.label check and return 0: if (filter.label) return 0; This will ensure we exit from print_linkinfo() earlier, skip print_link_stats() and push final filtering by label to print_selected_addrinfo() and print_addrinfo(). And finally: this is regression and should be against iproute2, not -next. > > Assuming above I would like to see ifa_label_match_rta() instead of open > coded checks for filter.label with fmatch() in print_linkinfo(). > > Also it might be good idea to pass @name from get_ifname_rta() (like we > do in print_linkinfo()) to ifa_label_match_rta() so that we respect > IFLA_IFNAME if present. >
❦ 14 juillet 2018 21:54 +0300, Serhey Popovych <serhe.popovych@gmail.com> : > We should leave only filter.label check and return 0: > > if (filter.label) > return 0; > > This will ensure we exit from print_linkinfo() earlier, skip > print_link_stats() and push final filtering by label to > print_selected_addrinfo() and print_addrinfo(). > > And finally: this is regression and should be against iproute2, not -next. As you did the analysis, I let you do the patch.
diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 5009bfe6d2e3..20ef6724944e 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who, if (!name) return -1; - if (filter.label && - (!filter.family || filter.family == AF_PACKET) && - fnmatch(filter.label, name, 0)) - return -1; - if (tb[IFLA_GROUP]) { int group = rta_getattr_u32(tb[IFLA_GROUP]);
Since 9516823051ce, "ip addr show label lo:1" doesn't work anymore (doesn't show any address, despite a matching label). Reverting to return 0 instead of -1 fix the issue. However, the condition says: "if we filter by label [...] and the label does NOT match the interface name". This makes little sense to compare the label with the interface name. There is also a logic around filter family being provided or not. The match against the label is done by ifa_label_match_rta() in print_addrinfo() and ipaddr_filter(). Just removing the condition makes "ip addr show" works as expected with or without specifying a label, both when the label is matching and not matching. It also works if we specify a label and the label is the interface name. The flush operation also works as expected. Fixes: 9516823051ce ("ipaddress: Improve print_linkinfo()") Signed-off-by: Vincent Bernat <vincent@bernat.im> --- ip/ipaddress.c | 5 ----- 1 file changed, 5 deletions(-)