From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH iproute2] ipaddress: Fix and make consistent label match handling Date: Wed, 18 Jul 2018 15:54:16 -0700 Message-ID: <20180718155416.11c9ad26@xeon-e3> References: <1531604194-12136-1-git-send-email-serhe.popovych@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, vincent@bernat.im, dsahern@gmail.com To: Serhey Popovych Return-path: Received: from mail-pl0-f67.google.com ([209.85.160.67]:34889 "EHLO mail-pl0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728929AbeGRXkK (ORCPT ); Wed, 18 Jul 2018 19:40:10 -0400 Received: by mail-pl0-f67.google.com with SMTP id w3-v6so2692923plq.2 for ; Wed, 18 Jul 2018 16:00:04 -0700 (PDT) In-Reply-To: <1531604194-12136-1-git-send-email-serhe.popovych@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, 15 Jul 2018 00:36:34 +0300 Serhey Popovych wrote: > Since commit 9516823051ce ("ipaddress: Improve print_linkinfo()") we > return -1 instead of 0 when ip-address(8) label does not match network > device name as we did before change. This causes regression when trying > to output ip address matching label: > > # ip addr add 192.168.192.1/24 dev lo label lo:1 > # ip addr show label lo:1 > > > This is special case and return 0 from print_linkinfo() earlier to match > only filter.ifindex and filter.up if given, but 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 IFA_LABEL attribute in netlink buffer with filter.label using > ifa_label_match_rta(). > > On the other hand there is three conditions checked in print_linkinfo() > to determine label special case: > > 1) filter.label != NULL > 2) filter.family == AF_UNSPEC || filter.family == AF_PACKET > 3) fnmatch(filter.label, name, 0) > > With 1) it is ok to check if filtering by label is on by given pattern > in @filter.label. > > Since label is IPv4 specific and AF_PACKET is for printing ip-link(8) > information (see ipaddr_link_list()::ipaddress.c as example) checking > for AF_PACKET in 2) doesn't take much sense: better to defer these > checks to print_addrinfo() determine valid combinations before calling > ifa_label_match_rta() to finally match IFA_LABEL to pattern in > filter.label. > > For 3) we have following call for test case: > > fnmatch(pattern, string, flags) -> > fnmatch(filter.label, name, 0) -> > fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1) or non-zero on error > > To support special case in print_linkinfo() for filtering by label we > only need to check if label pattern is given in filter.label and return > 0 to skip print_link_stats() in ipaddr_list_flush_or_save(): actual > filtering will be done in print_addrinfo(). > > Before commit 9516823051ce ("ipaddress: Improve print_linkinfo()"): > ------------------------------------------------------------------- > > $ ip addr sh label lo > 1: lo: mtu 65536 qdisc noqueue state UNKNOWN \ > group default qlen 1000 > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > fnmatch("lo", "lo", 0) == 0 > link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 > inet 127.0.0.1/8 scope host lo > valid_lft forever preferred_lft forever > inet6 ::1/128 scope host > valid_lft forever preferred_lft forever > $ ip addr show label 'lo:*' > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > $ ip addr sh label lo:1 > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > $ ip -4 addr sh label lo:1 > 1: lo: mtu 65536 qdisc noqueue state UNKNOWN \ > group default qlen 1000 > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > filter.family == AF_INET > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > > After this change applied: > -------------------------- > > $ ip/ip addr show label lo > inet 127.0.0.1/8 scope host lo > valid_lft forever preferred_lft forever > inet6 ::1/128 scope host > valid_lft forever preferred_lft forever > $ ip/ip addr show label 'lo:*' > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > $ ip/ip addr show label lo:1 > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > $ ip/ip -4 addr show label lo:1 > inet 192.168.192.1/24 scope global lo:1 > valid_lft forever preferred_lft forever > > Note that we no longer show link information as we did previously: > we are filtering by "label" pattern, not showing by "dev". > > Fixes: commit 9516823051ce ("ipaddress: Improve print_linkinfo()") > Reported-by: Vincent Bernat > Signed-off-by: Serhey Popovych Makes sense applied. Thanks for following through on this.