Serhey Popovych wrote: > Vincent Bernat wrote: >> ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsah...@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.
>
signature.asc
Description: OpenPGP digital signature
