* [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" @ 2018-03-12 21:03 Stephen Hemminger 2018-03-12 21:37 ` Luca Boccassi 0 siblings, 1 reply; 22+ messages in thread From: Stephen Hemminger @ 2018-03-12 21:03 UTC (permalink / raw) To: bluca, green; +Cc: netdev, Stephen Hemminger This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. Debian maintainer found that basic command: # ip route flush all No longer worked as expected which breaks user scripts and expectations. It no longer flushed all IPv4 routes. Reported-by: Luca Boccassi <bluca@debian.org> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- ip/iproute.c | 65 ++++++++++++++++++------------------------------------------ lib/utils.c | 13 ++++++++++++ 2 files changed, 32 insertions(+), 46 deletions(-) diff --git a/ip/iproute.c b/ip/iproute.c index bf886fda9d76..32c93ed5abd9 100644 --- a/ip/iproute.c +++ b/ip/iproute.c @@ -191,42 +191,20 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) return 0; if ((filter.tos^r->rtm_tos)&filter.tosmask) return 0; - if (filter.rdst.family) { - if (r->rtm_family != filter.rdst.family || - filter.rdst.bitlen > r->rtm_dst_len) - return 0; - } else if (filter.rdst.flags & PREFIXLEN_SPECIFIED) { - if (filter.rdst.bitlen > r->rtm_dst_len) - return 0; - } - if (filter.mdst.family) { - if (r->rtm_family != filter.mdst.family || - (filter.mdst.bitlen >= 0 && - filter.mdst.bitlen < r->rtm_dst_len)) - return 0; - } else if (filter.mdst.flags & PREFIXLEN_SPECIFIED) { - if (filter.mdst.bitlen >= 0 && - filter.mdst.bitlen < r->rtm_dst_len) - return 0; - } - if (filter.rsrc.family) { - if (r->rtm_family != filter.rsrc.family || - filter.rsrc.bitlen > r->rtm_src_len) - return 0; - } else if (filter.rsrc.flags & PREFIXLEN_SPECIFIED) { - if (filter.rsrc.bitlen > r->rtm_src_len) - return 0; - } - if (filter.msrc.family) { - if (r->rtm_family != filter.msrc.family || - (filter.msrc.bitlen >= 0 && - filter.msrc.bitlen < r->rtm_src_len)) - return 0; - } else if (filter.msrc.flags & PREFIXLEN_SPECIFIED) { - if (filter.msrc.bitlen >= 0 && - filter.msrc.bitlen < r->rtm_src_len) - return 0; - } + if (filter.rdst.family && + (r->rtm_family != filter.rdst.family || filter.rdst.bitlen > r->rtm_dst_len)) + return 0; + if (filter.mdst.family && + (r->rtm_family != filter.mdst.family || + (filter.mdst.bitlen >= 0 && filter.mdst.bitlen < r->rtm_dst_len))) + return 0; + if (filter.rsrc.family && + (r->rtm_family != filter.rsrc.family || filter.rsrc.bitlen > r->rtm_src_len)) + return 0; + if (filter.msrc.family && + (r->rtm_family != filter.msrc.family || + (filter.msrc.bitlen >= 0 && filter.msrc.bitlen < r->rtm_src_len))) + return 0; if (filter.rvia.family) { int family = r->rtm_family; @@ -243,9 +221,7 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) if (tb[RTA_DST]) memcpy(&dst.data, RTA_DATA(tb[RTA_DST]), (r->rtm_dst_len+7)/8); - if (filter.rsrc.family || filter.msrc.family || - filter.rsrc.flags & PREFIXLEN_SPECIFIED || - filter.msrc.flags & PREFIXLEN_SPECIFIED) { + if (filter.rsrc.family || filter.msrc.family) { if (tb[RTA_SRC]) memcpy(&src.data, RTA_DATA(tb[RTA_SRC]), (r->rtm_src_len+7)/8); } @@ -265,18 +241,15 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) memcpy(&prefsrc.data, RTA_DATA(tb[RTA_PREFSRC]), host_len/8); } - if ((filter.rdst.family || filter.rdst.flags & PREFIXLEN_SPECIFIED) && - inet_addr_match(&dst, &filter.rdst, filter.rdst.bitlen)) + if (filter.rdst.family && inet_addr_match(&dst, &filter.rdst, filter.rdst.bitlen)) return 0; - if ((filter.mdst.family || filter.mdst.flags & PREFIXLEN_SPECIFIED) && + if (filter.mdst.family && filter.mdst.bitlen >= 0 && inet_addr_match(&dst, &filter.mdst, r->rtm_dst_len)) return 0; - if ((filter.rsrc.family || filter.rsrc.flags & PREFIXLEN_SPECIFIED) && - inet_addr_match(&src, &filter.rsrc, filter.rsrc.bitlen)) + if (filter.rsrc.family && inet_addr_match(&src, &filter.rsrc, filter.rsrc.bitlen)) return 0; - if ((filter.msrc.family || filter.msrc.flags & PREFIXLEN_SPECIFIED) && - filter.msrc.bitlen >= 0 && + if (filter.msrc.family && filter.msrc.bitlen >= 0 && inet_addr_match(&src, &filter.msrc, r->rtm_src_len)) return 0; diff --git a/lib/utils.c b/lib/utils.c index 379739d61246..87b609f2a6bc 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -681,6 +681,19 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) char *slash; int err, bitlen, flags; + memset(dst, 0, sizeof(*dst)); + + if (strcmp(arg, "default") == 0 || + strcmp(arg, "any") == 0 || + strcmp(arg, "all") == 0) { + if ((family == AF_DECnet) || (family == AF_MPLS)) + return -1; + dst->family = family; + dst->bytelen = 0; + dst->bitlen = 0; + return 0; + } + slash = strchr(arg, '/'); if (slash) *slash = 0; -- 2.16.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-12 21:03 [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger @ 2018-03-12 21:37 ` Luca Boccassi 2018-03-13 8:46 ` Alexander Zubkov 0 siblings, 1 reply; 22+ messages in thread From: Luca Boccassi @ 2018-03-12 21:37 UTC (permalink / raw) To: Stephen Hemminger, green; +Cc: netdev [-- Attachment #1: Type: text/plain, Size: 966 bytes --] On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: > This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. > > Debian maintainer found that basic command: > # ip route flush all > No longer worked as expected which breaks user scripts and > expectations. It no longer flushed all IPv4 routes. > > Reported-by: Luca Boccassi <bluca@debian.org> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > ip/iproute.c | 65 ++++++++++++++++++------------------------------ > ------------ > lib/utils.c | 13 ++++++++++++ > 2 files changed, 32 insertions(+), 46 deletions(-) Tested-by: Luca Boccassi <bluca@debian.org> Thanks, solves the problem. I'll backport it to Debian. Alexander, reproducing the issue is quite simple - before that commit, ip route ls all showed all routes, but with the change it started showing only the default table. Same for ip route flush. -- Kind regards, Luca Boccassi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-12 21:37 ` Luca Boccassi @ 2018-03-13 8:46 ` Alexander Zubkov 2018-03-13 11:05 ` Alexander Zubkov 0 siblings, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-13 8:46 UTC (permalink / raw) To: Luca Boccassi, Stephen Hemminger; +Cc: netdev Hello. May be the better way would be to change how "all"/"any" argument behaves? My original concern was about "default" only. I agree too, that "all" or "any" should work for all routes. But not for the default. 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: > On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >> >> Debian maintainer found that basic command: >> # ip route flush all >> No longer worked as expected which breaks user scripts and >> expectations. It no longer flushed all IPv4 routes. >> >> Reported-by: Luca Boccassi <bluca@debian.org> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >> --- >> ip/iproute.c | 65 ++++++++++++++++++------------------------------ >> ------------ >> lib/utils.c | 13 ++++++++++++ >> 2 files changed, 32 insertions(+), 46 deletions(-) > > Tested-by: Luca Boccassi <bluca@debian.org> > > Thanks, solves the problem. I'll backport it to Debian. > > Alexander, reproducing the issue is quite simple - before that commit, > ip route ls all showed all routes, but with the change it started > showing only the default table. Same for ip route flush. > > -- > Kind regards, > Luca Boccassi ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-13 8:46 ` Alexander Zubkov @ 2018-03-13 11:05 ` Alexander Zubkov 2018-03-13 12:02 ` Luca Boccassi 0 siblings, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-13 11:05 UTC (permalink / raw) To: Luca Boccassi, Stephen Hemminger; +Cc: netdev Hello again, The fun thing is that before the commit "ip route ls all" showed all routes, but "ip -[4|6] route ls all" showed only default. So it was broken too, but in other way. I see parsing of prefix was changed since my patch. So I need several days to propose fix. I think if "ip route ls [all|any]" shows all routes and "ip route ls default" shows only default, everybody will be happy with that? 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: > Hello. > > May be the better way would be to change how "all"/"any" argument behaves? My original concern was about "default" only. I agree too, that "all" or "any" should work for all routes. But not for the default. > > 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: >> On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >>> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >>> >>> Debian maintainer found that basic command: >>> # ip route flush all >>> No longer worked as expected which breaks user scripts and >>> expectations. It no longer flushed all IPv4 routes. >>> >>> Reported-by: Luca Boccassi <bluca@debian.org> >>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >>> --- >>> ip/iproute.c | 65 ++++++++++++++++++------------------------------ >>> ------------ >>> lib/utils.c | 13 ++++++++++++ >>> 2 files changed, 32 insertions(+), 46 deletions(-) >> >> Tested-by: Luca Boccassi <bluca@debian.org> >> >> Thanks, solves the problem. I'll backport it to Debian. >> >> Alexander, reproducing the issue is quite simple - before that commit, >> ip route ls all showed all routes, but with the change it started >> showing only the default table. Same for ip route flush. >> >> -- >> Kind regards, >> Luca Boccassi ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-13 11:05 ` Alexander Zubkov @ 2018-03-13 12:02 ` Luca Boccassi 2018-03-13 20:12 ` Alexander Zubkov 2018-03-13 20:19 ` [PATCH iproute2] treat "default" and "all"/"any" parameters differenty Alexander Zubkov 0 siblings, 2 replies; 22+ messages in thread From: Luca Boccassi @ 2018-03-13 12:02 UTC (permalink / raw) To: Alexander Zubkov, Stephen Hemminger; +Cc: netdev [-- Attachment #1: Type: text/plain, Size: 2300 bytes --] On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: > Hello again, > > The fun thing is that before the commit "ip route ls all" showed all > routes, but "ip -[4|6] route ls all" showed only default. So it was > broken too, but in other way. > I see parsing of prefix was changed since my patch. So I need several > days to propose fix. I think if "ip route ls [all|any]" shows all > routes and "ip route ls default" shows only default, everybody will > be happy with that? Hi, My only concern is that behaviour of existing commands that have been in releases is not changed, otherwise I get bugs raised :-) Thank you for your work! > 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: > > Hello. > > > > May be the better way would be to change how "all"/"any" argument > > behaves? My original concern was about "default" only. I agree too, > > that "all" or "any" should work for all routes. But not for the > > default. > > > > 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: > > > On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: > > > > This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. > > > > > > > > Debian maintainer found that basic command: > > > > # ip route flush all > > > > No longer worked as expected which breaks user scripts and > > > > expectations. It no longer flushed all IPv4 routes. > > > > > > > > Reported-by: Luca Boccassi <bluca@debian.org> > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > > > --- > > > > ip/iproute.c | 65 ++++++++++++++++++---------------------- > > > > -------- > > > > ------------ > > > > lib/utils.c | 13 ++++++++++++ > > > > 2 files changed, 32 insertions(+), 46 deletions(-) > > > > > > Tested-by: Luca Boccassi <bluca@debian.org> > > > > > > Thanks, solves the problem. I'll backport it to Debian. > > > > > > Alexander, reproducing the issue is quite simple - before that > > > commit, > > > ip route ls all showed all routes, but with the change it > > > started > > > showing only the default table. Same for ip route flush. > > > > > > -- > > > Kind regards, > > > Luca Boccassi -- Kind regards, Luca Boccassi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-13 12:02 ` Luca Boccassi @ 2018-03-13 20:12 ` Alexander Zubkov 2018-03-13 20:15 ` Luca Boccassi 2018-03-14 8:59 ` Alexander Zubkov 2018-03-13 20:19 ` [PATCH iproute2] treat "default" and "all"/"any" parameters differenty Alexander Zubkov 1 sibling, 2 replies; 22+ messages in thread From: Alexander Zubkov @ 2018-03-13 20:12 UTC (permalink / raw) To: Luca Boccassi, Stephen Hemminger; +Cc: netdev Hi, I just realized that you need patch for v4.15.0, which is easier to do. I'll send it as separate message now. I will make patch for the master branch, but later. On 13.03.2018 13:02, Luca Boccassi wrote: > On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: >> Hello again, >> >> The fun thing is that before the commit "ip route ls all" showed all >> routes, but "ip -[4|6] route ls all" showed only default. So it was >> broken too, but in other way. >> I see parsing of prefix was changed since my patch. So I need several >> days to propose fix. I think if "ip route ls [all|any]" shows all >> routes and "ip route ls default" shows only default, everybody will >> be happy with that? > > Hi, > > My only concern is that behaviour of existing commands that have been > in releases is not changed, otherwise I get bugs raised :-) > > Thank you for your work! > >> 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: >>> Hello. >>> >>> May be the better way would be to change how "all"/"any" argument >>> behaves? My original concern was about "default" only. I agree too, >>> that "all" or "any" should work for all routes. But not for the >>> default. >>> >>> 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: >>>> On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >>>>> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >>>>> >>>>> Debian maintainer found that basic command: >>>>> # ip route flush all >>>>> No longer worked as expected which breaks user scripts and >>>>> expectations. It no longer flushed all IPv4 routes. >>>>> >>>>> Reported-by: Luca Boccassi <bluca@debian.org> >>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >>>>> --- >>>>> ip/iproute.c | 65 ++++++++++++++++++---------------------- >>>>> -------- >>>>> ------------ >>>>> lib/utils.c | 13 ++++++++++++ >>>>> 2 files changed, 32 insertions(+), 46 deletions(-) >>>> >>>> Tested-by: Luca Boccassi <bluca@debian.org> >>>> >>>> Thanks, solves the problem. I'll backport it to Debian. >>>> >>>> Alexander, reproducing the issue is quite simple - before that >>>> commit, >>>> ip route ls all showed all routes, but with the change it >>>> started >>>> showing only the default table. Same for ip route flush. >>>> >>>> -- >>>> Kind regards, >>>> Luca Boccassi > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-13 20:12 ` Alexander Zubkov @ 2018-03-13 20:15 ` Luca Boccassi 2018-03-14 8:59 ` Alexander Zubkov 1 sibling, 0 replies; 22+ messages in thread From: Luca Boccassi @ 2018-03-13 20:15 UTC (permalink / raw) To: Alexander Zubkov, Stephen Hemminger; +Cc: netdev [-- Attachment #1: Type: text/plain, Size: 3179 bytes --] On Tue, 2018-03-13 at 21:12 +0100, Alexander Zubkov wrote: > Hi, > > I just realized that you need patch for v4.15.0, which is easier to > do. > I'll send it as separate message now. I will make patch for the > master > branch, but later. Thanks but don't worry about 4.15 - Stephen's revert will be enough for now. I'm going to push 4.16 as soon as it's out anyway, so you can just do the changes for master if you wish. > On 13.03.2018 13:02, Luca Boccassi wrote: > > On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: > > > Hello again, > > > > > > The fun thing is that before the commit "ip route ls all" showed > > > all > > > routes, but "ip -[4|6] route ls all" showed only default. So it > > > was > > > broken too, but in other way. > > > I see parsing of prefix was changed since my patch. So I need > > > several > > > days to propose fix. I think if "ip route ls [all|any]" shows all > > > routes and "ip route ls default" shows only default, everybody > > > will > > > be happy with that? > > > > Hi, > > > > My only concern is that behaviour of existing commands that have > > been > > in releases is not changed, otherwise I get bugs raised :-) > > > > Thank you for your work! > > > > > 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: > > > > Hello. > > > > > > > > May be the better way would be to change how "all"/"any" > > > > argument > > > > behaves? My original concern was about "default" only. I agree > > > > too, > > > > that "all" or "any" should work for all routes. But not for the > > > > default. > > > > > > > > 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: > > > > > On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: > > > > > > This reverts commit > > > > > > 9135c4d6037ff9f1818507bac0049fc44db8c3d2. > > > > > > > > > > > > Debian maintainer found that basic command: > > > > > > # ip route flush all > > > > > > No longer worked as expected which breaks user scripts > > > > > > and > > > > > > expectations. It no longer flushed all IPv4 routes. > > > > > > > > > > > > Reported-by: Luca Boccassi <bluca@debian.org> > > > > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber > > > > > > .org> > > > > > > --- > > > > > > ip/iproute.c | 65 ++++++++++++++++++------------------- > > > > > > --- > > > > > > -------- > > > > > > ------------ > > > > > > lib/utils.c | 13 ++++++++++++ > > > > > > 2 files changed, 32 insertions(+), 46 deletions(-) > > > > > > > > > > Tested-by: Luca Boccassi <bluca@debian.org> > > > > > > > > > > Thanks, solves the problem. I'll backport it to Debian. > > > > > > > > > > Alexander, reproducing the issue is quite simple - before > > > > > that > > > > > commit, > > > > > ip route ls all showed all routes, but with the change it > > > > > started > > > > > showing only the default table. Same for ip route flush. > > > > > > > > > > -- > > > > > Kind regards, > > > > > Luca Boccassi > > -- Kind regards, Luca Boccassi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-13 20:12 ` Alexander Zubkov 2018-03-13 20:15 ` Luca Boccassi @ 2018-03-14 8:59 ` Alexander Zubkov 2018-03-14 20:26 ` Alexander Zubkov 1 sibling, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-14 8:59 UTC (permalink / raw) To: Serhey Popovych, Luca Boccassi, Stephen Hemminger; +Cc: netdev Hello, There was a series of patches by Serhey and specifically this one: https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=93fa12418dc6f5943692250244be303bb162175b It drops handling of special prefix names in get_prefix_1(), and in get_addr_1() they always receive family and bytelen. But as I unerstand for any case it was important to keep it with unspecified family for further filtering. As I do not know what is the global idea, I want to discuss it. Because there are options depending on how and where we want to handle those special names. Like keep unspecified family or change filtering logic. I have added Serhey Popovych in the recepients, so he can give some ideas on what his aim is and help choose better solution. 13.03.2018, 21:12, "Alexander Zubkov" <green@msu.ru>: > Hi, > > I just realized that you need patch for v4.15.0, which is easier to do. > I'll send it as separate message now. I will make patch for the master > branch, but later. > > On 13.03.2018 13:02, Luca Boccassi wrote: >> On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: >>> Hello again, >>> >>> The fun thing is that before the commit "ip route ls all" showed all >>> routes, but "ip -[4|6] route ls all" showed only default. So it was >>> broken too, but in other way. >>> I see parsing of prefix was changed since my patch. So I need several >>> days to propose fix. I think if "ip route ls [all|any]" shows all >>> routes and "ip route ls default" shows only default, everybody will >>> be happy with that? >> >> Hi, >> >> My only concern is that behaviour of existing commands that have been >> in releases is not changed, otherwise I get bugs raised :-) >> >> Thank you for your work! >> >>> 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: >>>> Hello. >>>> >>>> May be the better way would be to change how "all"/"any" argument >>>> behaves? My original concern was about "default" only. I agree too, >>>> that "all" or "any" should work for all routes. But not for the >>>> default. >>>> >>>> 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: >>>>> On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >>>>>> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >>>>>> >>>>>> Debian maintainer found that basic command: >>>>>> # ip route flush all >>>>>> No longer worked as expected which breaks user scripts and >>>>>> expectations. It no longer flushed all IPv4 routes. >>>>>> >>>>>> Reported-by: Luca Boccassi <bluca@debian.org> >>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >>>>>> --- >>>>>> ip/iproute.c | 65 ++++++++++++++++++---------------------- >>>>>> -------- >>>>>> ------------ >>>>>> lib/utils.c | 13 ++++++++++++ >>>>>> 2 files changed, 32 insertions(+), 46 deletions(-) >>>>> >>>>> Tested-by: Luca Boccassi <bluca@debian.org> >>>>> >>>>> Thanks, solves the problem. I'll backport it to Debian. >>>>> >>>>> Alexander, reproducing the issue is quite simple - before that >>>>> commit, >>>>> ip route ls all showed all routes, but with the change it >>>>> started >>>>> showing only the default table. Same for ip route flush. >>>>> >>>>> -- >>>>> Kind regards, >>>>> Luca Boccassi ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-14 8:59 ` Alexander Zubkov @ 2018-03-14 20:26 ` Alexander Zubkov 2018-03-18 16:50 ` [PATCH iproute2] treat "default" and "all"/"any" addresses differenty Alexander Zubkov 2018-03-27 16:01 ` [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger 0 siblings, 2 replies; 22+ messages in thread From: Alexander Zubkov @ 2018-03-14 20:26 UTC (permalink / raw) To: Serhey Popovych, Luca Boccassi, Stephen Hemminger; +Cc: netdev Hello, For example, it can be fixed in such way (patch is below): - split handling of default and all/any - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default - and AF_UNSPEC for all/any In this case "ip route show default" shows only default route and "ip route show all" shows all routes. And both also work when family (-4 or -6) is specified. Serhey, does it goes in line with what you wanted to achieve? Because I do not know - may be there are reasons why all/any should be provided with specific family. If you think this solution is suitable, I'll do some additional tests and package the patch in a proper way for this mailing list. And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both branches. May be someone have some additional thoughts on that? --- a/lib/utils.c +++ b/lib/utils.c @@ -560,14 +560,23 @@ static int __get_addr_1(inet_prefix *addr, const char *name, int family) { memset(addr, 0, sizeof(*addr)); - if (strcmp(name, "default") == 0 || - strcmp(name, "all") == 0 || - strcmp(name, "any") == 0) { + if (strcmp(name, "default") == 0) { if ((family == AF_DECnet) || (family == AF_MPLS)) return -1; addr->family = (family != AF_UNSPEC) ? family : AF_INET; addr->bytelen = af_byte_len(addr->family); addr->bitlen = -2; + addr->flags |= PREFIXLEN_SPECIFIED; + return 0; + } + + if (strcmp(name, "all") == 0 || + strcmp(name, "any") == 0) { + if ((family == AF_DECnet) || (family == AF_MPLS)) + return -1; + addr->family = AF_UNSPEC; + addr->bytelen = 0; + addr->bitlen = -2; return 0; } @@ -695,7 +704,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) bitlen = af_bit_len(dst->family); - flags = PREFIXLEN_SPECIFIED; + flags = 0; if (slash) { unsigned int plen; @@ -706,12 +715,11 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) if (plen > bitlen) return -1; + flags |= PREFIXLEN_SPECIFIED; bitlen = plen; } else { if (dst->bitlen == -2) bitlen = 0; - else - flags = 0; } dst->flags |= flags; On 14.03.2018 09:59, Alexander Zubkov wrote: > Hello, > > There was a series of patches by Serhey and specifically this one: > https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/?id=93fa12418dc6f5943692250244be303bb162175b > > It drops handling of special prefix names in get_prefix_1(), and in get_addr_1() they always receive family and bytelen. But as I unerstand for any case it was important to keep it with unspecified family for further filtering. As I do not know what is the global idea, I want to discuss it. Because there are options depending on how and where we want to handle those special names. Like keep unspecified family or change filtering logic. > > I have added Serhey Popovych in the recepients, so he can give some ideas on what his aim is and help choose better solution. > > 13.03.2018, 21:12, "Alexander Zubkov" <green@msu.ru>: >> Hi, >> >> I just realized that you need patch for v4.15.0, which is easier to do. >> I'll send it as separate message now. I will make patch for the master >> branch, but later. >> >> On 13.03.2018 13:02, Luca Boccassi wrote: >>> On Tue, 2018-03-13 at 12:05 +0100, Alexander Zubkov wrote: >>>> Hello again, >>>> >>>> The fun thing is that before the commit "ip route ls all" showed all >>>> routes, but "ip -[4|6] route ls all" showed only default. So it was >>>> broken too, but in other way. >>>> I see parsing of prefix was changed since my patch. So I need several >>>> days to propose fix. I think if "ip route ls [all|any]" shows all >>>> routes and "ip route ls default" shows only default, everybody will >>>> be happy with that? >>> >>> Hi, >>> >>> My only concern is that behaviour of existing commands that have been >>> in releases is not changed, otherwise I get bugs raised :-) >>> >>> Thank you for your work! >>> >>>> 13.03.2018, 09:46, "Alexander Zubkov" <green@msu.ru>: >>>>> Hello. >>>>> >>>>> May be the better way would be to change how "all"/"any" argument >>>>> behaves? My original concern was about "default" only. I agree too, >>>>> that "all" or "any" should work for all routes. But not for the >>>>> default. >>>>> >>>>> 12.03.2018, 22:37, "Luca Boccassi" <bluca@debian.org>: >>>>>> On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote: >>>>>>> This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2. >>>>>>> >>>>>>> Debian maintainer found that basic command: >>>>>>> # ip route flush all >>>>>>> No longer worked as expected which breaks user scripts and >>>>>>> expectations. It no longer flushed all IPv4 routes. >>>>>>> >>>>>>> Reported-by: Luca Boccassi <bluca@debian.org> >>>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> >>>>>>> --- >>>>>>> ip/iproute.c | 65 ++++++++++++++++++---------------------- >>>>>>> -------- >>>>>>> ------------ >>>>>>> lib/utils.c | 13 ++++++++++++ >>>>>>> 2 files changed, 32 insertions(+), 46 deletions(-) >>>>>> >>>>>> Tested-by: Luca Boccassi <bluca@debian.org> >>>>>> >>>>>> Thanks, solves the problem. I'll backport it to Debian. >>>>>> >>>>>> Alexander, reproducing the issue is quite simple - before that >>>>>> commit, >>>>>> ip route ls all showed all routes, but with the change it >>>>>> started >>>>>> showing only the default table. Same for ip route flush. >>>>>> >>>>>> -- >>>>>> Kind regards, >>>>>> Luca Boccassi ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH iproute2] treat "default" and "all"/"any" addresses differenty 2018-03-14 20:26 ` Alexander Zubkov @ 2018-03-18 16:50 ` Alexander Zubkov 2018-03-18 17:02 ` Alexander Zubkov 2018-03-27 16:01 ` [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger 1 sibling, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-18 16:50 UTC (permalink / raw) To: Stephen Hemminger Cc: Luca Boccassi, netdev, Serhey Popovych, Alexander Zubkov Debian maintainer found that basic command: # ip route flush all No longer worked as expected which breaks user scripts and expectations. It no longer flushed all IPv4 routes. Recently behavior of "default" prefix parameter was corrected. But at the same time behavior of "all"/"any" was altered too, because they were the same branch of the code. As those parameters mean different, they need to be treated differently in code too. This patch reflects the difference. Also after mentioned change, address parsing code was changed more and address family was set explicitly even for "all"/"any" addresses. And that broke matching conditions further. This patch fixes that too and returns AF_UNSPEC to "all"/"any" address. Now "default" is treated as top-level prefix (for example 0.0.0.0/0 in IPv4) and "all"/"any" always matches anything in exact, root and match modes. Reported-by: Luca Boccassi <bluca@debian.org> Signed-off-by: Alexander Zubkov <green@msu.ru> --- lib/utils.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/utils.c b/lib/utils.c index 379739d..eba4fa7 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -560,14 +560,23 @@ static int __get_addr_1(inet_prefix *addr, const char *name, int family) { memset(addr, 0, sizeof(*addr)); - if (strcmp(name, "default") == 0 || - strcmp(name, "all") == 0 || - strcmp(name, "any") == 0) { + if (strcmp(name, "default") == 0) { if ((family == AF_DECnet) || (family == AF_MPLS)) return -1; addr->family = (family != AF_UNSPEC) ? family : AF_INET; addr->bytelen = af_byte_len(addr->family); addr->bitlen = -2; + addr->flags |= PREFIXLEN_SPECIFIED; + return 0; + } + + if (strcmp(name, "all") == 0 || + strcmp(name, "any") == 0) { + if ((family == AF_DECnet) || (family == AF_MPLS)) + return -1; + addr->family = AF_UNSPEC; + addr->bytelen = 0; + addr->bitlen = -2; return 0; } @@ -695,7 +704,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) bitlen = af_bit_len(dst->family); - flags = PREFIXLEN_SPECIFIED; + flags = 0; if (slash) { unsigned int plen; @@ -706,12 +715,11 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) if (plen > bitlen) return -1; + flags |= PREFIXLEN_SPECIFIED; bitlen = plen; } else { if (dst->bitlen == -2) bitlen = 0; - else - flags = 0; } dst->flags |= flags; -- 1.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] treat "default" and "all"/"any" addresses differenty 2018-03-18 16:50 ` [PATCH iproute2] treat "default" and "all"/"any" addresses differenty Alexander Zubkov @ 2018-03-18 17:02 ` Alexander Zubkov 0 siblings, 0 replies; 22+ messages in thread From: Alexander Zubkov @ 2018-03-18 17:02 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Luca Boccassi, netdev, Serhey Popovych This version of patch is for master now. 18.03.2018, 17:50, "Alexander Zubkov" <green@msu.ru>: > Debian maintainer found that basic command: > # ip route flush all > No longer worked as expected which breaks user scripts and > expectations. It no longer flushed all IPv4 routes. > > Recently behavior of "default" prefix parameter was corrected. But at > the same time behavior of "all"/"any" was altered too, because they > were the same branch of the code. As those parameters mean different, > they need to be treated differently in code too. This patch reflects > the difference. > > Also after mentioned change, address parsing code was changed more > and address family was set explicitly even for "all"/"any" addresses. > And that broke matching conditions further. This patch fixes that too > and returns AF_UNSPEC to "all"/"any" address. > > Now "default" is treated as top-level prefix (for example 0.0.0.0/0 in > IPv4) and "all"/"any" always matches anything in exact, root and match > modes. > > Reported-by: Luca Boccassi <bluca@debian.org> > Signed-off-by: Alexander Zubkov <green@msu.ru> > --- > lib/utils.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/lib/utils.c b/lib/utils.c > index 379739d..eba4fa7 100644 > --- a/lib/utils.c > +++ b/lib/utils.c > @@ -560,14 +560,23 @@ static int __get_addr_1(inet_prefix *addr, const char *name, int family) > { > memset(addr, 0, sizeof(*addr)); > > - if (strcmp(name, "default") == 0 || > - strcmp(name, "all") == 0 || > - strcmp(name, "any") == 0) { > + if (strcmp(name, "default") == 0) { > if ((family == AF_DECnet) || (family == AF_MPLS)) > return -1; > addr->family = (family != AF_UNSPEC) ? family : AF_INET; > addr->bytelen = af_byte_len(addr->family); > addr->bitlen = -2; > + addr->flags |= PREFIXLEN_SPECIFIED; > + return 0; > + } > + > + if (strcmp(name, "all") == 0 || > + strcmp(name, "any") == 0) { > + if ((family == AF_DECnet) || (family == AF_MPLS)) > + return -1; > + addr->family = AF_UNSPEC; > + addr->bytelen = 0; > + addr->bitlen = -2; > return 0; > } > > @@ -695,7 +704,7 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) > > bitlen = af_bit_len(dst->family); > > - flags = PREFIXLEN_SPECIFIED; > + flags = 0; > if (slash) { > unsigned int plen; > > @@ -706,12 +715,11 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) > if (plen > bitlen) > return -1; > > + flags |= PREFIXLEN_SPECIFIED; > bitlen = plen; > } else { > if (dst->bitlen == -2) > bitlen = 0; > - else > - flags = 0; > } > > dst->flags |= flags; > -- > 1.9.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-14 20:26 ` Alexander Zubkov 2018-03-18 16:50 ` [PATCH iproute2] treat "default" and "all"/"any" addresses differenty Alexander Zubkov @ 2018-03-27 16:01 ` Stephen Hemminger 2018-03-27 16:29 ` Alexander Zubkov 1 sibling, 1 reply; 22+ messages in thread From: Stephen Hemminger @ 2018-03-27 16:01 UTC (permalink / raw) To: Alexander Zubkov; +Cc: Serhey Popovych, Luca Boccassi, netdev On Wed, 14 Mar 2018 21:26:40 +0100 Alexander Zubkov <green@msu.ru> wrote: > Hello, > > For example, it can be fixed in such way (patch is below): > - split handling of default and all/any > - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default > - and AF_UNSPEC for all/any > In this case "ip route show default" shows only default route and "ip > route show all" shows all routes. And both also work when family (-4 or > -6) is specified. > Serhey, does it goes in line with what you wanted to achieve? Because I > do not know - may be there are reasons why all/any should be provided > with specific family. If you think this solution is suitable, I'll do > some additional tests and package the patch in a proper way for this > mailing list. > And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both > branches. May be someone have some additional thoughts on that? > I applied this to master. We can work on the other cases after that. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-27 16:01 ` [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger @ 2018-03-27 16:29 ` Alexander Zubkov 2018-03-27 17:00 ` Stephen Hemminger 0 siblings, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-27 16:29 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Serhey Popovych, Luca Boccassi, netdev Hi Stephen, Looks like the new patch was applied after the revert of original patch and fix patch for 4.15 branch. Which is not correct and I did not test it. This is how patches were designed: 1) your revert patch - rolls back 4.15 branch to old behaviour by reverting the original patch 2) my patch for 4.15 - fixes problem is 4.15 branch, it does not require revert patch, it is an alternative solution for the problem, it is designed solely for version 4.15 3) my patch for master - fixes problem, it requires neither revert patch nor my patch for 4.15, it is standalone patch designed to do things right in master branch 27.03.2018, 18:01, "Stephen Hemminger" <stephen@networkplumber.org>: > On Wed, 14 Mar 2018 21:26:40 +0100 > Alexander Zubkov <green@msu.ru> wrote: > >> Hello, >> >> For example, it can be fixed in such way (patch is below): >> - split handling of default and all/any >> - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default >> - and AF_UNSPEC for all/any >> In this case "ip route show default" shows only default route and "ip >> route show all" shows all routes. And both also work when family (-4 or >> -6) is specified. >> Serhey, does it goes in line with what you wanted to achieve? Because I >> do not know - may be there are reasons why all/any should be provided >> with specific family. If you think this solution is suitable, I'll do >> some additional tests and package the patch in a proper way for this >> mailing list. >> And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both >> branches. May be someone have some additional thoughts on that? > > I applied this to master. > > We can work on the other cases after that. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-27 16:29 ` Alexander Zubkov @ 2018-03-27 17:00 ` Stephen Hemminger 2018-03-27 17:33 ` Alexander Zubkov 0 siblings, 1 reply; 22+ messages in thread From: Stephen Hemminger @ 2018-03-27 17:00 UTC (permalink / raw) To: Alexander Zubkov; +Cc: Serhey Popovych, Luca Boccassi, netdev On Tue, 27 Mar 2018 18:29:31 +0200 Alexander Zubkov <green@msu.ru> wrote: > Hi Stephen, > > Looks like the new patch was applied after the revert of original patch and fix patch for 4.15 branch. Which is not correct and I did not test it. This is how patches were designed: > 1) your revert patch - rolls back 4.15 branch to old behaviour by reverting the original patch > 2) my patch for 4.15 - fixes problem is 4.15 branch, it does not require revert patch, it is an alternative solution for the problem, it is designed solely for version 4.15 > 3) my patch for master - fixes problem, it requires neither revert patch nor my patch for 4.15, it is standalone patch designed to do things right in master branch > > 27.03.2018, 18:01, "Stephen Hemminger" <stephen@networkplumber.org>: > > On Wed, 14 Mar 2018 21:26:40 +0100 > > Alexander Zubkov <green@msu.ru> wrote: > > > >> Hello, > >> > >> For example, it can be fixed in such way (patch is below): > >> - split handling of default and all/any > >> - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default > >> - and AF_UNSPEC for all/any > >> In this case "ip route show default" shows only default route and "ip > >> route show all" shows all routes. And both also work when family (-4 or > >> -6) is specified. > >> Serhey, does it goes in line with what you wanted to achieve? Because I > >> do not know - may be there are reasons why all/any should be provided > >> with specific family. If you think this solution is suitable, I'll do > >> some additional tests and package the patch in a proper way for this > >> mailing list. > >> And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both > >> branches. May be someone have some additional thoughts on that? > > > > I applied this to master. > > > > We can work on the other cases after that. Please send the update back to what works. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-27 17:00 ` Stephen Hemminger @ 2018-03-27 17:33 ` Alexander Zubkov 2018-03-27 17:58 ` Stephen Hemminger 0 siblings, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-27 17:33 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Serhey Popovych, Luca Boccassi, netdev master before merging revert + my recent patch (1) should work. Or you mean to prepare patch to change new master to desired state? I can do it. 1) https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/patch/?id=7696f1097f79be2ce5984a8a16103fd17391cac2 27.03.2018, 19:00, "Stephen Hemminger" <stephen@networkplumber.org>: > On Tue, 27 Mar 2018 18:29:31 +0200 > Alexander Zubkov <green@msu.ru> wrote: > >> Hi Stephen, >> >> Looks like the new patch was applied after the revert of original patch and fix patch for 4.15 branch. Which is not correct and I did not test it. This is how patches were designed: >> 1) your revert patch - rolls back 4.15 branch to old behaviour by reverting the original patch >> 2) my patch for 4.15 - fixes problem is 4.15 branch, it does not require revert patch, it is an alternative solution for the problem, it is designed solely for version 4.15 >> 3) my patch for master - fixes problem, it requires neither revert patch nor my patch for 4.15, it is standalone patch designed to do things right in master branch >> >> 27.03.2018, 18:01, "Stephen Hemminger" <stephen@networkplumber.org>: >> > On Wed, 14 Mar 2018 21:26:40 +0100 >> > Alexander Zubkov <green@msu.ru> wrote: >> > >> >> Hello, >> >> >> >> For example, it can be fixed in such way (patch is below): >> >> - split handling of default and all/any >> >> - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default >> >> - and AF_UNSPEC for all/any >> >> In this case "ip route show default" shows only default route and "ip >> >> route show all" shows all routes. And both also work when family (-4 or >> >> -6) is specified. >> >> Serhey, does it goes in line with what you wanted to achieve? Because I >> >> do not know - may be there are reasons why all/any should be provided >> >> with specific family. If you think this solution is suitable, I'll do >> >> some additional tests and package the patch in a proper way for this >> >> mailing list. >> >> And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both >> >> branches. May be someone have some additional thoughts on that? >> > >> > I applied this to master. >> > >> > We can work on the other cases after that. > > Please send the update back to what works. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" 2018-03-27 17:33 ` Alexander Zubkov @ 2018-03-27 17:58 ` Stephen Hemminger 2018-03-27 23:57 ` [PATCH iproute] arrange prefix parsing code after redundant patches Alexander Zubkov 0 siblings, 1 reply; 22+ messages in thread From: Stephen Hemminger @ 2018-03-27 17:58 UTC (permalink / raw) To: Alexander Zubkov; +Cc: Serhey Popovych, Luca Boccassi, netdev On Tue, 27 Mar 2018 19:33:28 +0200 Alexander Zubkov <green@msu.ru> wrote: > master before merging revert + my recent patch (1) should work. Or you mean to prepare patch to change new master to desired state? I can do it. > > 1) https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/patch/?id=7696f1097f79be2ce5984a8a16103fd17391cac2 > > 27.03.2018, 19:00, "Stephen Hemminger" <stephen@networkplumber.org>: > > On Tue, 27 Mar 2018 18:29:31 +0200 > > Alexander Zubkov <green@msu.ru> wrote: > > > >> Hi Stephen, > >> > >> Looks like the new patch was applied after the revert of original patch and fix patch for 4.15 branch. Which is not correct and I did not test it. This is how patches were designed: > >> 1) your revert patch - rolls back 4.15 branch to old behaviour by reverting the original patch > >> 2) my patch for 4.15 - fixes problem is 4.15 branch, it does not require revert patch, it is an alternative solution for the problem, it is designed solely for version 4.15 > >> 3) my patch for master - fixes problem, it requires neither revert patch nor my patch for 4.15, it is standalone patch designed to do things right in master branch > >> > >> 27.03.2018, 18:01, "Stephen Hemminger" <stephen@networkplumber.org>: > >> > On Wed, 14 Mar 2018 21:26:40 +0100 > >> > Alexander Zubkov <green@msu.ru> wrote: > >> > > >> >> Hello, > >> >> > >> >> For example, it can be fixed in such way (patch is below): > >> >> - split handling of default and all/any > >> >> - set needed attributes in get_addr: PREFIXLEN_SPECIFIED flag for default > >> >> - and AF_UNSPEC for all/any > >> >> In this case "ip route show default" shows only default route and "ip > >> >> route show all" shows all routes. And both also work when family (-4 or > >> >> -6) is specified. > >> >> Serhey, does it goes in line with what you wanted to achieve? Because I > >> >> do not know - may be there are reasons why all/any should be provided > >> >> with specific family. If you think this solution is suitable, I'll do > >> >> some additional tests and package the patch in a proper way for this > >> >> mailing list. > >> >> And I'm unsure if check for AF_DECnet and AF_MPLS should be kept in both > >> >> branches. May be someone have some additional thoughts on that? > >> > > >> > I applied this to master. > >> > > >> > We can work on the other cases after that. > > > > Please send the update back to what works. Make patches against current master. For visible repositories, I prefer to only move forward and not rollback. So you can send a revert patch than new code if that is easier. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH iproute] arrange prefix parsing code after redundant patches 2018-03-27 17:58 ` Stephen Hemminger @ 2018-03-27 23:57 ` Alexander Zubkov 2018-03-29 15:44 ` Stephen Hemminger 0 siblings, 1 reply; 22+ messages in thread From: Alexander Zubkov @ 2018-03-27 23:57 UTC (permalink / raw) To: Stephen Hemminger Cc: Luca Boccassi, netdev, Serhey Popovych, Alexander Zubkov A problem was reported with parsing of prefixes all/any/default. Commit 7696f1097f79be2ce5984a8a16103fd17391cac2 fixes the problem, but there were also other pathces applied: 00b31a6b2ecf73ee477f701098164600a2bfe227, which were intended to fix the same problem. And they became redundant now. This patch reverts changes introduced by those redundant patches. Signed-off-by: Alexander Zubkov <green@msu.ru> --- ip/iproute.c | 65 ++++++++++++++++++++++++++++++++++++++++++------------------ lib/utils.c | 13 ------------ 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/ip/iproute.c b/ip/iproute.c index 32c93ed..bf886fd 100644 --- a/ip/iproute.c +++ b/ip/iproute.c @@ -191,20 +191,42 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) return 0; if ((filter.tos^r->rtm_tos)&filter.tosmask) return 0; - if (filter.rdst.family && - (r->rtm_family != filter.rdst.family || filter.rdst.bitlen > r->rtm_dst_len)) - return 0; - if (filter.mdst.family && - (r->rtm_family != filter.mdst.family || - (filter.mdst.bitlen >= 0 && filter.mdst.bitlen < r->rtm_dst_len))) - return 0; - if (filter.rsrc.family && - (r->rtm_family != filter.rsrc.family || filter.rsrc.bitlen > r->rtm_src_len)) - return 0; - if (filter.msrc.family && - (r->rtm_family != filter.msrc.family || - (filter.msrc.bitlen >= 0 && filter.msrc.bitlen < r->rtm_src_len))) - return 0; + if (filter.rdst.family) { + if (r->rtm_family != filter.rdst.family || + filter.rdst.bitlen > r->rtm_dst_len) + return 0; + } else if (filter.rdst.flags & PREFIXLEN_SPECIFIED) { + if (filter.rdst.bitlen > r->rtm_dst_len) + return 0; + } + if (filter.mdst.family) { + if (r->rtm_family != filter.mdst.family || + (filter.mdst.bitlen >= 0 && + filter.mdst.bitlen < r->rtm_dst_len)) + return 0; + } else if (filter.mdst.flags & PREFIXLEN_SPECIFIED) { + if (filter.mdst.bitlen >= 0 && + filter.mdst.bitlen < r->rtm_dst_len) + return 0; + } + if (filter.rsrc.family) { + if (r->rtm_family != filter.rsrc.family || + filter.rsrc.bitlen > r->rtm_src_len) + return 0; + } else if (filter.rsrc.flags & PREFIXLEN_SPECIFIED) { + if (filter.rsrc.bitlen > r->rtm_src_len) + return 0; + } + if (filter.msrc.family) { + if (r->rtm_family != filter.msrc.family || + (filter.msrc.bitlen >= 0 && + filter.msrc.bitlen < r->rtm_src_len)) + return 0; + } else if (filter.msrc.flags & PREFIXLEN_SPECIFIED) { + if (filter.msrc.bitlen >= 0 && + filter.msrc.bitlen < r->rtm_src_len) + return 0; + } if (filter.rvia.family) { int family = r->rtm_family; @@ -221,7 +243,9 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) if (tb[RTA_DST]) memcpy(&dst.data, RTA_DATA(tb[RTA_DST]), (r->rtm_dst_len+7)/8); - if (filter.rsrc.family || filter.msrc.family) { + if (filter.rsrc.family || filter.msrc.family || + filter.rsrc.flags & PREFIXLEN_SPECIFIED || + filter.msrc.flags & PREFIXLEN_SPECIFIED) { if (tb[RTA_SRC]) memcpy(&src.data, RTA_DATA(tb[RTA_SRC]), (r->rtm_src_len+7)/8); } @@ -241,15 +265,18 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len) memcpy(&prefsrc.data, RTA_DATA(tb[RTA_PREFSRC]), host_len/8); } - if (filter.rdst.family && inet_addr_match(&dst, &filter.rdst, filter.rdst.bitlen)) + if ((filter.rdst.family || filter.rdst.flags & PREFIXLEN_SPECIFIED) && + inet_addr_match(&dst, &filter.rdst, filter.rdst.bitlen)) return 0; - if (filter.mdst.family && filter.mdst.bitlen >= 0 && + if ((filter.mdst.family || filter.mdst.flags & PREFIXLEN_SPECIFIED) && inet_addr_match(&dst, &filter.mdst, r->rtm_dst_len)) return 0; - if (filter.rsrc.family && inet_addr_match(&src, &filter.rsrc, filter.rsrc.bitlen)) + if ((filter.rsrc.family || filter.rsrc.flags & PREFIXLEN_SPECIFIED) && + inet_addr_match(&src, &filter.rsrc, filter.rsrc.bitlen)) return 0; - if (filter.msrc.family && filter.msrc.bitlen >= 0 && + if ((filter.msrc.family || filter.msrc.flags & PREFIXLEN_SPECIFIED) && + filter.msrc.bitlen >= 0 && inet_addr_match(&src, &filter.msrc, r->rtm_src_len)) return 0; diff --git a/lib/utils.c b/lib/utils.c index dadefb5..b9e9a6c 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -693,19 +693,6 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) char *slash; int err, bitlen, flags; - memset(dst, 0, sizeof(*dst)); - - if (strcmp(arg, "default") == 0 || - strcmp(arg, "any") == 0 || - strcmp(arg, "all") == 0) { - if ((family == AF_DECnet) || (family == AF_MPLS)) - return -1; - dst->family = family; - dst->bytelen = 0; - dst->bitlen = 0; - return 0; - } - slash = strchr(arg, '/'); if (slash) *slash = 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH iproute] arrange prefix parsing code after redundant patches 2018-03-27 23:57 ` [PATCH iproute] arrange prefix parsing code after redundant patches Alexander Zubkov @ 2018-03-29 15:44 ` Stephen Hemminger 0 siblings, 0 replies; 22+ messages in thread From: Stephen Hemminger @ 2018-03-29 15:44 UTC (permalink / raw) To: Alexander Zubkov; +Cc: Luca Boccassi, netdev, Serhey Popovych On Wed, 28 Mar 2018 01:57:13 +0200 Alexander Zubkov <green@msu.ru> wrote: > A problem was reported with parsing of prefixes all/any/default. > Commit 7696f1097f79be2ce5984a8a16103fd17391cac2 fixes the problem, > but there were also other pathces applied: > 00b31a6b2ecf73ee477f701098164600a2bfe227, which were intended to > fix the same problem. And they became redundant now. This patch > reverts changes introduced by those redundant patches. > > Signed-off-by: Alexander Zubkov <green@msu.ru> Thanks applied to current master branch ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH iproute2] treat "default" and "all"/"any" parameters differenty 2018-03-13 12:02 ` Luca Boccassi 2018-03-13 20:12 ` Alexander Zubkov @ 2018-03-13 20:19 ` Alexander Zubkov 2018-03-13 21:45 ` Luca Boccassi 2018-03-16 20:37 ` Stephen Hemminger 1 sibling, 2 replies; 22+ messages in thread From: Alexander Zubkov @ 2018-03-13 20:19 UTC (permalink / raw) To: Luca Boccassi, Stephen Hemminger; +Cc: netdev Debian maintainer found that basic command: # ip route flush all No longer worked as expected which breaks user scripts and expectations. It no longer flushed all IPv4 routes. Recently behaviour of "default" prefix parameter was corrected. But at the same time behaviour of "all"/"any" was altered too, because they were the same branch of the code. As those parameters mean different, they need to be treated differently in code too. This patch reflects the difference. Reported-by: Luca Boccassi <bluca@debian.org> Signed-off-by: Alexander Zubkov <green@msu.ru> --- lib/utils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/utils.c b/lib/utils.c index 9fa5220..b289d9c 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -658,7 +658,8 @@ int get_prefix_1(inet_prefix *dst, char *arg, int family) dst->family = family; dst->bytelen = 0; dst->bitlen = 0; - dst->flags |= PREFIXLEN_SPECIFIED; + if (strcmp(arg, "default") == 0) + dst->flags |= PREFIXLEN_SPECIFIED; return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] treat "default" and "all"/"any" parameters differenty 2018-03-13 20:19 ` [PATCH iproute2] treat "default" and "all"/"any" parameters differenty Alexander Zubkov @ 2018-03-13 21:45 ` Luca Boccassi 2018-03-16 20:37 ` Stephen Hemminger 1 sibling, 0 replies; 22+ messages in thread From: Luca Boccassi @ 2018-03-13 21:45 UTC (permalink / raw) To: Alexander Zubkov, Stephen Hemminger; +Cc: netdev [-- Attachment #1: Type: text/plain, Size: 1367 bytes --] On Tue, 2018-03-13 at 21:19 +0100, Alexander Zubkov wrote: > Debian maintainer found that basic command: > # ip route flush all > No longer worked as expected which breaks user scripts and > expectations. It no longer flushed all IPv4 routes. > > Recently behaviour of "default" prefix parameter was corrected. But > at > the same time behaviour of "all"/"any" was altered too, because they > were the same branch of the code. As those parameters mean different, > they need to be treated differently in code too. This patch reflects > the difference. > > Reported-by: Luca Boccassi <bluca@debian.org> > Signed-off-by: Alexander Zubkov <green@msu.ru> > --- > lib/utils.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/utils.c b/lib/utils.c > index 9fa5220..b289d9c 100644 > --- a/lib/utils.c > +++ b/lib/utils.c > @@ -658,7 +658,8 @@ int get_prefix_1(inet_prefix *dst, char *arg, > int > family) > dst->family = family; > dst->bytelen = 0; > dst->bitlen = 0; > - dst->flags |= PREFIXLEN_SPECIFIED; > + if (strcmp(arg, "default") == 0) > + dst->flags |= PREFIXLEN_SPECIFIED; > return 0; > } > > -- > 1.9.1 Tested-by: Luca Boccassi <bluca@debian.org> Yep solves the problem, ls all/any now work as before. Thanks! -- Kind regards, Luca Boccassi [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] treat "default" and "all"/"any" parameters differenty 2018-03-13 20:19 ` [PATCH iproute2] treat "default" and "all"/"any" parameters differenty Alexander Zubkov 2018-03-13 21:45 ` Luca Boccassi @ 2018-03-16 20:37 ` Stephen Hemminger 2018-03-16 20:59 ` Alexander Zubkov 1 sibling, 1 reply; 22+ messages in thread From: Stephen Hemminger @ 2018-03-16 20:37 UTC (permalink / raw) To: Alexander Zubkov; +Cc: Luca Boccassi, netdev On Tue, 13 Mar 2018 21:19:45 +0100 Alexander Zubkov <green@msu.ru> wrote: > Debian maintainer found that basic command: > # ip route flush all > No longer worked as expected which breaks user scripts and > expectations. It no longer flushed all IPv4 routes. > > Recently behaviour of "default" prefix parameter was corrected. But at > the same time behaviour of "all"/"any" was altered too, because they > were the same branch of the code. As those parameters mean different, > they need to be treated differently in code too. This patch reflects > the difference. > > Reported-by: Luca Boccassi <bluca@debian.org> > Signed-off-by: Alexander Zubkov <green@msu.ru> > --- > lib/utils.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/utils.c b/lib/utils.c > index 9fa5220..b289d9c 100644 > --- a/lib/utils.c > +++ b/lib/utils.c > @@ -658,7 +658,8 @@ int get_prefix_1(inet_prefix *dst, char *arg, int > family) > dst->family = family; > dst->bytelen = 0; > dst->bitlen = 0; > - dst->flags |= PREFIXLEN_SPECIFIED; > + if (strcmp(arg, "default") == 0) > + dst->flags |= PREFIXLEN_SPECIFIED; > return 0; > } > > -- > 1.9.1 > Which code is this a patch against? the current master or followup to my revert of the original patch? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH iproute2] treat "default" and "all"/"any" parameters differenty 2018-03-16 20:37 ` Stephen Hemminger @ 2018-03-16 20:59 ` Alexander Zubkov 0 siblings, 0 replies; 22+ messages in thread From: Alexander Zubkov @ 2018-03-16 20:59 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Luca Boccassi, netdev Hi Stephen, This patch is against v4.15.0 only. It will not fit the current master, because there were changes it that areas of code since then. I have made some patch for master too and trying to discuss it with Serhey, who made other changes there. He has not answered yet. If there are no answer for some more time, I'll consider that fix as suitable and send it as a proper patch to the list. That will not require reverse patch too. 16.03.2018, 21:37, "Stephen Hemminger" <stephen@networkplumber.org>: > On Tue, 13 Mar 2018 21:19:45 +0100 > Alexander Zubkov <green@msu.ru> wrote: > >> Debian maintainer found that basic command: >> # ip route flush all >> No longer worked as expected which breaks user scripts and >> expectations. It no longer flushed all IPv4 routes. >> >> Recently behaviour of "default" prefix parameter was corrected. But at >> the same time behaviour of "all"/"any" was altered too, because they >> were the same branch of the code. As those parameters mean different, >> they need to be treated differently in code too. This patch reflects >> the difference. >> >> Reported-by: Luca Boccassi <bluca@debian.org> >> Signed-off-by: Alexander Zubkov <green@msu.ru> >> --- >> lib/utils.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/utils.c b/lib/utils.c >> index 9fa5220..b289d9c 100644 >> --- a/lib/utils.c >> +++ b/lib/utils.c >> @@ -658,7 +658,8 @@ int get_prefix_1(inet_prefix *dst, char *arg, int >> family) >> dst->family = family; >> dst->bytelen = 0; >> dst->bitlen = 0; >> - dst->flags |= PREFIXLEN_SPECIFIED; >> + if (strcmp(arg, "default") == 0) >> + dst->flags |= PREFIXLEN_SPECIFIED; >> return 0; >> } >> >> -- >> 1.9.1 > > Which code is this a patch against? the current master or followup to my revert > of the original patch? ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-03-29 15:44 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-12 21:03 [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger 2018-03-12 21:37 ` Luca Boccassi 2018-03-13 8:46 ` Alexander Zubkov 2018-03-13 11:05 ` Alexander Zubkov 2018-03-13 12:02 ` Luca Boccassi 2018-03-13 20:12 ` Alexander Zubkov 2018-03-13 20:15 ` Luca Boccassi 2018-03-14 8:59 ` Alexander Zubkov 2018-03-14 20:26 ` Alexander Zubkov 2018-03-18 16:50 ` [PATCH iproute2] treat "default" and "all"/"any" addresses differenty Alexander Zubkov 2018-03-18 17:02 ` Alexander Zubkov 2018-03-27 16:01 ` [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes" Stephen Hemminger 2018-03-27 16:29 ` Alexander Zubkov 2018-03-27 17:00 ` Stephen Hemminger 2018-03-27 17:33 ` Alexander Zubkov 2018-03-27 17:58 ` Stephen Hemminger 2018-03-27 23:57 ` [PATCH iproute] arrange prefix parsing code after redundant patches Alexander Zubkov 2018-03-29 15:44 ` Stephen Hemminger 2018-03-13 20:19 ` [PATCH iproute2] treat "default" and "all"/"any" parameters differenty Alexander Zubkov 2018-03-13 21:45 ` Luca Boccassi 2018-03-16 20:37 ` Stephen Hemminger 2018-03-16 20:59 ` Alexander Zubkov
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.