* [PATCH iproute2-next] ipaddress: fix label matching
@ 2018-07-11 11:36 Vincent Bernat
2018-07-11 20:03 ` Stephen Hemminger
2018-07-12 1:01 ` David Ahern
0 siblings, 2 replies; 8+ messages in thread
From: Vincent Bernat @ 2018-07-11 11:36 UTC (permalink / raw)
To: netdev, stephen, serhe.popovych; +Cc: Vincent Bernat
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]);
--
2.18.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-11 11:36 [PATCH iproute2-next] ipaddress: fix label matching Vincent Bernat
@ 2018-07-11 20:03 ` Stephen Hemminger
2018-07-11 20:26 ` Vincent Bernat
2018-07-12 1:01 ` David Ahern
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2018-07-11 20:03 UTC (permalink / raw)
To: Vincent Bernat; +Cc: netdev, serhe.popovych
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-11 20:03 ` Stephen Hemminger
@ 2018-07-11 20:26 ` Vincent Bernat
0 siblings, 0 replies; 8+ messages in thread
From: Vincent Bernat @ 2018-07-11 20:26 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, serhe.popovych
❦ 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.
--
The lunatic, the lover, and the poet,
Are of imagination all compact...
-- Wm. Shakespeare, "A Midsummer Night's Dream"
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-11 11:36 [PATCH iproute2-next] ipaddress: fix label matching Vincent Bernat
2018-07-11 20:03 ` Stephen Hemminger
@ 2018-07-12 1:01 ` David Ahern
2018-07-12 5:08 ` Vincent Bernat
1 sibling, 1 reply; 8+ messages in thread
From: David Ahern @ 2018-07-12 1:01 UTC (permalink / raw)
To: Vincent Bernat, netdev, stephen, serhe.popovych
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-12 1:01 ` David Ahern
@ 2018-07-12 5:08 ` Vincent Bernat
2018-07-14 8:27 ` Serhey Popovych
0 siblings, 1 reply; 8+ messages in thread
From: Vincent Bernat @ 2018-07-12 5:08 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, stephen, serhe.popovych
❦ 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.
--
Many pages make a thick book, except for pocket Bibles which are on very
very thin paper.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-12 5:08 ` Vincent Bernat
@ 2018-07-14 8:27 ` Serhey Popovych
2018-07-14 18:54 ` Serhey Popovych
0 siblings, 1 reply; 8+ messages in thread
From: Serhey Popovych @ 2018-07-14 8:27 UTC (permalink / raw)
To: Vincent Bernat, David Ahern; +Cc: netdev, stephen
[-- Attachment #1.1: Type: text/plain, Size: 1605 bytes --]
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.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-14 8:27 ` Serhey Popovych
@ 2018-07-14 18:54 ` Serhey Popovych
2018-07-14 18:58 ` Vincent Bernat
0 siblings, 1 reply; 8+ messages in thread
From: Serhey Popovych @ 2018-07-14 18:54 UTC (permalink / raw)
To: Vincent Bernat, David Ahern; +Cc: netdev, stephen
[-- Attachment #1.1: Type: text/plain, Size: 3198 bytes --]
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.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH iproute2-next] ipaddress: fix label matching
2018-07-14 18:54 ` Serhey Popovych
@ 2018-07-14 18:58 ` Vincent Bernat
0 siblings, 0 replies; 8+ messages in thread
From: Vincent Bernat @ 2018-07-14 18:58 UTC (permalink / raw)
To: Serhey Popovych; +Cc: David Ahern, netdev, stephen
❦ 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.
--
Watch out for off-by-one errors.
- The Elements of Programming Style (Kernighan & Plauger)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-14 19:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-11 11:36 [PATCH iproute2-next] ipaddress: fix label matching Vincent Bernat
2018-07-11 20:03 ` Stephen Hemminger
2018-07-11 20:26 ` Vincent Bernat
2018-07-12 1:01 ` David Ahern
2018-07-12 5:08 ` Vincent Bernat
2018-07-14 8:27 ` Serhey Popovych
2018-07-14 18:54 ` Serhey Popovych
2018-07-14 18:58 ` Vincent Bernat
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.