Re: [PATCH iproute2-next] ipaddress: fix label matching
❦ 14 juillet 2018 21:54 +0300, Serhey Popovych : > 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. -- Watch out for off-by-one errors. - The Elements of Programming Style (Kernighan & Plauger)
Re: [PATCH iproute2-next] ipaddress: fix label matching
Serhey Popovych wrote:
> Vincent Bernat wrote:
>> ❦ 11 juillet 2018 21:01 -0400, David Ahern :
>>
+++ 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
Re: [PATCH iproute2-next] ipaddress: fix label matching
Vincent Bernat wrote:
> ❦ 11 juillet 2018 21:01 -0400, David Ahern :
>
>>> +++ 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.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH iproute2-next] ipaddress: fix label matching
❦ 11 juillet 2018 21:01 -0400, David Ahern : >> +++ 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. -- Many pages make a thick book, except for pocket Bibles which are on very very thin paper.
Re: [PATCH iproute2-next] ipaddress: fix label matching
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?
Re: [PATCH iproute2-next] ipaddress: fix label matching
❦ 11 juillet 2018 13:03 -0700, Stephen Hemminger :
>> 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
>> ---
>> 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.
--
The lunatic, the lover, and the poet,
Are of imagination all compact...
-- Wm. Shakespeare, "A Midsummer Night's Dream"
Re: [PATCH iproute2-next] ipaddress: fix label matching
On Wed, 11 Jul 2018 13:36:03 +0200
Vincent Bernat 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
> ---
> 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.
[PATCH iproute2-next] ipaddress: fix label matching
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
---
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]);
--
2.18.0
