[PATCH iproute] arrange prefix parsing code after redundant patches

[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 <gr...@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))
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(, 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(, 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(, RTA_DATA(tb[RTA_PREFSRC]), 
host_len/8);
}
 
-   if (filter.rdst.family && inet_addr_match(, , 
filter.rdst.bitlen))
+   if ((filter.rdst.family || filter.rdst.flags & PREFIXLEN_SPECIFIED) &&
+   inet_addr_match(, , 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(, , r->rtm_dst_len))
return 0;
 
-   if (filter.rsrc.family && inet_addr_match(, , 
filter.rsrc.bitlen))
+   if ((filter.rsrc.family || filter.rsrc.flags & PREFIXLEN_SPECIFIED) &&
+   inet_addr_match(, , filter.rsrc.bitlen))
return 0;
-   if (filter.msrc.family && filter.m

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <step...@networkplumber.org>:
> On Tue, 27 Mar 2018 18:29:31 +0200
> Alexander Zubkov <gr...@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" <step...@networkplumber.org>:
>>  > On Wed, 14 Mar 2018 21:26:40 +0100
>>  > Alexander Zubkov <gr...@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.

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <step...@networkplumber.org>:
> On Wed, 14 Mar 2018 21:26:40 +0100
> Alexander Zubkov <gr...@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.

Re: [PATCH iproute2] treat "default" and "all"/"any" addresses differenty

[Alexander Zubkov]

This version of patch is for master now.

18.03.2018, 17:50, "Alexander Zubkov" <gr...@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 <bl...@debian.org>
> Signed-off-by: Alexander Zubkov <gr...@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

[PATCH iproute2] treat "default" and "all"/"any" addresses differenty

[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 <bl...@debian.org>
Signed-off-by: Alexander Zubkov <gr...@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

Re: [PATCH iproute2] treat "default" and "all"/"any" parameters differenty

[Alexander Zubkov]

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" <step...@networkplumber.org>:
> On Tue, 13 Mar 2018 21:19:45 +0100
> Alexander Zubkov <gr...@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 <bl...@debian.org>
>>  Signed-off-by: Alexander Zubkov <gr...@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?

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <gr...@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" <gr...@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" <bl...@debian.org>:

    On Mon, 2018-03-12 at 14:03 -0700, Stephen Hemminger wrote:

 This reverts commit 9135c4d6037ff9f1818507bac0049fc44db8c3d2.

 Debian maintainer found 

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <gr...@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" <gr...@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" <bl...@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 <bl...@debian.org>
>>>>>> Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
>>>>>> ---
>>>>>>  ip/iproute.c | 65 ++--
>>>>>>  
>>>>>> 
>>>>>>  lib/utils.c  | 13 
>>>>>>  2 files changed, 32 insertions(+), 46 deletions(-)
>>>>>
>>>>>    Tested-by: Luca Boccassi <bl...@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

[PATCH iproute2] treat "default" and "all"/"any" parameters differenty

[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 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 <bl...@debian.org>
Signed-off-by: Alexander Zubkov <gr...@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

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <gr...@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" <bl...@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 <bl...@debian.org>
   Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
   ---
    ip/iproute.c | 65 ++--

   
    lib/utils.c  | 13 
    2 files changed, 32 insertions(+), 46 deletions(-)


  Tested-by: Luca Boccassi <bl...@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

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" <gr...@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" <bl...@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 <bl...@debian.org>
>>>   Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
>>>   ---
>>>    ip/iproute.c | 65 ++--
>>>   
>>>    lib/utils.c  | 13 
>>>    2 files changed, 32 insertions(+), 46 deletions(-)
>>
>>  Tested-by: Luca Boccassi <bl...@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

Re: [PATCH iproute2] Revert "iproute: "list/flush/save default" selected all of the routes"

[Alexander Zubkov]

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" :
> 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 
>>  Signed-off-by: Stephen Hemminger 
>>  ---
>>   ip/iproute.c | 65 ++--
>>  
>>   lib/utils.c  | 13 
>>   2 files changed, 32 insertions(+), 46 deletions(-)
>
> Tested-by: Luca Boccassi 
>
> 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

Re: [PATCH iproute2] ip route: broken logic when using default word and family not specified

[Alexander Zubkov]

OK. I have found the recommendations for kernel developers and
resubmited both of my patches according to their guidelines (I hope).
I have used my other e-mail because I think gmail is not good with
text email format.

On Sat, Dec 16, 2017 at 10:21 PM, Stephen Hemminger
<step...@networkplumber.org> wrote:
> On Sat, 18 Nov 2017 14:12:48 +0100
> Alexander Zubkov <zubkov...@gmail.com> wrote:
>
>> Hello,
>>
>> I have found odd behaviour when using "ip route list" (and other bound
>> commands) with prefix "default".
>>
>> When family not specified, its value is completely ignored and "ip
>> route list default" shows all inet4 prefixes. Same do "ip route list
>> exact default" and "ip route list match default". Examples are at the
>> end of the message.
>>
>> When family is specified, the behaviour changes and default works as
>> expected (=0.0.0.0/0 for inet4 and =::/0 for inet6). The above
>> commands all shows only default prefix in the output and only "root
>> default" shows all prefixes.
>>
>> I tried to dig into the code and found that when default is using with
>> unspecified family - the resulting structures filter.[mr]dst will
>> actually become all-zeroes as in the case when nothing is specified.
>>
>> I propose to change this in such a way (see attached patch). When
>> default prefix is parsed, the flag PREFIXLEN_SPECIFIED is attached to
>> it too, like for prefixes with "/". It seems logical to me,
>> because "/0" is really implied by "default" and even directly set up
>> in the code:
>>
>> dst->bitlen = 0;
>>
>> Then during filtering there is additional logic for unspecified family
>> and specified prefix.
>>
>> With this patch ip route list commands shown above are working as
>> expected. And it also works with unspecified table when routes are
>> printed from different families.
>>
>> Examples after applying the patch:
>>
>> # ./ip route list
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip -6 route list
>> fe80::/64 dev eth0 proto kernel metric 256 pref medium
>> fe80:1::/64 via fe80::3 dev eth0 metric 1024 pref medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> # ./ip route list default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list exact default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list match default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list root default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip route list default table all
>> default via 192.168.0.2 dev eth0
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> # ./ip route list root default table all
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> ...
>> ff00::/8 dev eth0 table local metric 256 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>>
>> And before patch:
>>
>> # ip route list default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip route list match default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip route list exact default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ip -4 route list exact default
>> default via 192.168.0.2 dev eth0
>> # ip -6 route list exact default
>> default via fe80::2 dev eth0  metric 1024
>> # ip route list exact default table all
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
>> ...
>> local fe80::3c09:19ff:feee:9866 dev lo  table local  proto none  metric 0
>> ff00::/8 dev eth0  table local  metric 256
>> unreachable default dev lo  table unspec  proto kernel  metric
>> 4294967295  error -101
>
> I think this is fine, and see no problem.
> The patch is missing 'Signed-off-by' line which is required for iproute2;
> the overall rules for iproute2 are (mostly) the same as the kernel.
>
> Please resubmit with Signed-off-by

[PATCH v3] iproute: list/flush/save filter also by metric

[Alexander Zubkov]

Metric is one of the "unique key" fields of the route in Linux. But
still one can not use its value in filter while running ip list.
Because of this writing checks in scripts for example is incovenient.

Signed-off-by: Alexander Zubkov <gr...@msu.ru>
---
 ip/iproute.c | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 16eadab..32c93ed 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -126,6 +126,7 @@ static struct
int oif, oifmask;
int mark, markmask;
int realm, realmmask;
+   __u32 metric, metricmask;
inet_prefix rprefsrc;
inet_prefix rvia;
inet_prefix rdst;
@@ -288,6 +289,14 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr 
**tb, int host_len)
if ((mark ^ filter.mark) & filter.markmask)
return 0;
}
+   if (filter.metricmask) {
+   __u32 metric = 0;
+
+   if (tb[RTA_PRIORITY])
+   metric = rta_getattr_u32(tb[RTA_PRIORITY]);
+   if ((metric ^ filter.metric) & filter.metricmask)
+   return 0;
+   }
if (filter.flushb &&
r->rtm_family == AF_INET6 &&
r->rtm_dst_len == 0 &&
@@ -441,7 +450,7 @@ int print_route(const struct sockaddr_nl *who, struct 
nlmsghdr *n, void *arg)
fprintf(fp, "src %s ",
rt_addr_n2a_rta(r->rtm_family, tb[RTA_PREFSRC]));
}
-   if (tb[RTA_PRIORITY])
+   if (tb[RTA_PRIORITY] && filter.metricmask != -1)
fprintf(fp, "metric %u ", rta_getattr_u32(tb[RTA_PRIORITY]));
if (r->rtm_flags & RTNH_F_DEAD)
fprintf(fp, "dead ");
@@ -1518,6 +1527,16 @@ static int iproute_list_flush_or_save(int argc, char 
**argv, int action)
if (get_unsigned(, *argv, 0))
invarg("invalid mark value", *argv);
filter.markmask = -1;
+   } else if (matches(*argv, "metric") == 0 ||
+  matches(*argv, "priority") == 0 ||
+  strcmp(*argv, "preference") == 0) {
+   __u32 metric;
+
+   NEXT_ARG();
+   if (get_u32(, *argv, 0))
+   invarg("\"metric\" value is invalid\n", *argv);
+   filter.metric = metric;
+   filter.metricmask = -1;
} else if (strcmp(*argv, "via") == 0) {
int family;
 

[PATCH] iproute: "list/flush/save default" selected all of the routes

[Alexander Zubkov]

When running "ip route list default" and not specifying address family,
one will get all of the routes instead of just default only. The same
is for "exact default" and "match default".

It behaves in such a way because default route with unspecified family
has the same all-zeroes value like no prefix specified at all. Thus
following code blindly ignores the fact, that prefix was actually
specified.

This patch adds the flag PREFIXLEN_SPECIFIED to the default route too.
And then checks its value when filtering routes.

Signed-off-by: Alexander Zubkov <gr...@msu.ru>
---
 ip/iproute.c | 65 ++--
 lib/utils.c  |  1 +
 2 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 16eadab..5e96783 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -190,20 +190,42 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr 
**tb, int host_len)
return 0;
if ((filter.tos^r->rtm_tos))
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;
 
@@ -220,7 +242,9 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr 
**tb, int host_len)
 
if (tb[RTA_DST])
memcpy(, 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(, RTA_DATA(tb[RTA_SRC]), 
(r->rtm_src_len+7)/8);
}
@@ -240,15 +264,18 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr 
**tb, int host_len)
memcpy(, RTA_DATA(tb[RTA_PREFSRC]), 
host_len/8);
}
 
-   if (filter.rdst.family && inet_addr_match(, , 
filter.rdst.bitlen))
+   if ((filter.rdst.family || filter.rdst.flags & PREFIXLEN_SPECIFIED) &&
+   inet_addr_match(, , 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(, , r->rtm_dst_len))
return 0;
 
-   if (filter.rsrc.family && inet_addr_match(, , 
filter.rsrc.bitlen))
+   if ((filter.rsrc.family || f

Re: [PATCH iproute2] ip route: broken logic when using default word and family not specified

[Alexander Zubkov]

Hello everybody,

I will be glad to hear a piece of feedback on this proposal.

On Sat, Nov 18, 2017 at 5:56 PM, Alexander Zubkov <zubkov...@gmail.com> wrote:
> I also opened earlier a ticket in bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=197899
> And Stephen Hemminger had couple of comments there which I want to argue:
>
>> $ ip route list default
>> Means list all routes in any address family (ie same as any)
>> but
>>
>> $ ip route list 0/0
>> Means list all routes for IPv4 default route.
>
> This is not correct, because first command do not show routes in any
> address family. Now it do so only when table 0 is specified, otherwise
> only IPv4 routes are showed. Here is the code from iproute.c:
>
> if (do_ipv6 == AF_UNSPEC && filter.tb)
> do_ipv6 = AF_INET;
>
>> It probably is worth a man page warning, but changing semantics
>> that have existed for many years is more likely to break some existing user.
>
> Yes, backward compatibility is a reason. But as I remember, that
> sematics already have changed earlier. Probably it was showing IPv4
> and IPv6 routes together without family specified - I do not remember
> exactly. And I have doubts that such feature could be lied on
> reliably.
> I as a end user would prefer to make the behaviour more consistent and
> without such excetptions. But of course there may be other opinions.
>
> On Sat, Nov 18, 2017 at 2:12 PM, Alexander Zubkov <zubkov...@gmail.com> wrote:
>> Hello,
>>
>> I have found odd behaviour when using "ip route list" (and other bound
>> commands) with prefix "default".
>>
>> When family not specified, its value is completely ignored and "ip
>> route list default" shows all inet4 prefixes. Same do "ip route list
>> exact default" and "ip route list match default". Examples are at the
>> end of the message.
>>
>> When family is specified, the behaviour changes and default works as
>> expected (=0.0.0.0/0 for inet4 and =::/0 for inet6). The above
>> commands all shows only default prefix in the output and only "root
>> default" shows all prefixes.
>>
>> I tried to dig into the code and found that when default is using with
>> unspecified family - the resulting structures filter.[mr]dst will
>> actually become all-zeroes as in the case when nothing is specified.
>>
>> I propose to change this in such a way (see attached patch). When
>> default prefix is parsed, the flag PREFIXLEN_SPECIFIED is attached to
>> it too, like for prefixes with "/". It seems logical to me,
>> because "/0" is really implied by "default" and even directly set up
>> in the code:
>>
>> dst->bitlen = 0;
>>
>> Then during filtering there is additional logic for unspecified family
>> and specified prefix.
>>
>> With this patch ip route list commands shown above are working as
>> expected. And it also works with unspecified table when routes are
>> printed from different families.
>>
>> Examples after applying the patch:
>>
>> # ./ip route list
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip -6 route list
>> fe80::/64 dev eth0 proto kernel metric 256 pref medium
>> fe80:1::/64 via fe80::3 dev eth0 metric 1024 pref medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> # ./ip route list default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list exact default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list match default
>> default via 192.168.0.2 dev eth0
>> # ./ip route list root default
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> 192.168.1.0/24 via 192.168.0.3 dev eth0
>> # ./ip route list default table all
>> default via 192.168.0.2 dev eth0
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> default via fe80::2 dev eth0 metric 1024 pref medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
>> medium
>> # ./ip route list root default table all
>> default via 192.168.0.2 dev eth0
>> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
>> ...
>> ff00::/8 dev eth0 table local metric 256 pref medium
>> unreachable default dev lo proto kernel metric 4294967295

Re: iproute2: make ip route list to search by metric too

[Alexander Zubkov]

Hello everybody,

Excuse me for probably being impatient. But may I have some feedback
on my proposal?

On Sat, Nov 18, 2017 at 11:11 AM, Alexander Zubkov <zubkov...@gmail.com> wrote:
> I think this version will be better. It uses metric mask (like for
> some other options) instead of simple yes/no flag.
>
> On Sat, Nov 18, 2017 at 2:44 AM, Alexander Zubkov <zubkov...@gmail.com> wrote:
>> Hello again,
>>
>> Things turned out to be not so hard. Please take a look at the attached 
>> patch.
>> I'm only not sure if RTA_PRIORITY is enough. Because the print_route
>> function prints "metric" also for some situations with RTA_METRICS,
>> which I haven't managed to understand.
>>
>> On Fri, Nov 17, 2017 at 1:40 AM, Alexander Zubkov <zubkov...@gmail.com> 
>> wrote:
>>> Hello all,
>>>
>>> Currently routes in the Linux routing table have these "key" fields:
>>> prefix, tos, table, metric (as I know). I.e. we cannot have two
>>> different routes with the same set of this fields. And "ip route list"
>>> command can be provided with all but one of those fields. We cannot
>>> pass metric to it and this is inconvenient. I ask if this behaviour
>>> can be changed by someone. We can even use "secondary" fields, for
>>> example type, dev or via, but not metric unfortunately.
>>> Sorry, I can not provide patches. I have written code long time ago. I
>>> tried to trace it, but as I see it parses arguments and fills some
>>> structures. And then my tries to understand failed.
>>> I opened the bug: https://bugzilla.kernel.org/show_bug.cgi?id=197897,
>>> but I was pointed out that this mailing list is a better place for
>>> this question.
>>>
>>> --
>>> Alexander Zubkov

Re: [PATCH iproute2] ip route: broken logic when using default word and family not specified

[Alexander Zubkov]

I also opened earlier a ticket in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=197899
And Stephen Hemminger had couple of comments there which I want to argue:

> $ ip route list default
> Means list all routes in any address family (ie same as any)
> but
>
> $ ip route list 0/0
> Means list all routes for IPv4 default route.

This is not correct, because first command do not show routes in any
address family. Now it do so only when table 0 is specified, otherwise
only IPv4 routes are showed. Here is the code from iproute.c:

if (do_ipv6 == AF_UNSPEC && filter.tb)
do_ipv6 = AF_INET;

> It probably is worth a man page warning, but changing semantics
> that have existed for many years is more likely to break some existing user.

Yes, backward compatibility is a reason. But as I remember, that
sematics already have changed earlier. Probably it was showing IPv4
and IPv6 routes together without family specified - I do not remember
exactly. And I have doubts that such feature could be lied on
reliably.
I as a end user would prefer to make the behaviour more consistent and
without such excetptions. But of course there may be other opinions.

On Sat, Nov 18, 2017 at 2:12 PM, Alexander Zubkov <zubkov...@gmail.com> wrote:
> Hello,
>
> I have found odd behaviour when using "ip route list" (and other bound
> commands) with prefix "default".
>
> When family not specified, its value is completely ignored and "ip
> route list default" shows all inet4 prefixes. Same do "ip route list
> exact default" and "ip route list match default". Examples are at the
> end of the message.
>
> When family is specified, the behaviour changes and default works as
> expected (=0.0.0.0/0 for inet4 and =::/0 for inet6). The above
> commands all shows only default prefix in the output and only "root
> default" shows all prefixes.
>
> I tried to dig into the code and found that when default is using with
> unspecified family - the resulting structures filter.[mr]dst will
> actually become all-zeroes as in the case when nothing is specified.
>
> I propose to change this in such a way (see attached patch). When
> default prefix is parsed, the flag PREFIXLEN_SPECIFIED is attached to
> it too, like for prefixes with "/". It seems logical to me,
> because "/0" is really implied by "default" and even directly set up
> in the code:
>
> dst->bitlen = 0;
>
> Then during filtering there is additional logic for unspecified family
> and specified prefix.
>
> With this patch ip route list commands shown above are working as
> expected. And it also works with unspecified table when routes are
> printed from different families.
>
> Examples after applying the patch:
>
> # ./ip route list
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
> 192.168.1.0/24 via 192.168.0.3 dev eth0
> # ./ip -6 route list
> fe80::/64 dev eth0 proto kernel metric 256 pref medium
> fe80:1::/64 via fe80::3 dev eth0 metric 1024 pref medium
> default via fe80::2 dev eth0 metric 1024 pref medium
> # ./ip route list default
> default via 192.168.0.2 dev eth0
> # ./ip route list exact default
> default via 192.168.0.2 dev eth0
> # ./ip route list match default
> default via 192.168.0.2 dev eth0
> # ./ip route list root default
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
> 192.168.1.0/24 via 192.168.0.3 dev eth0
> # ./ip route list default table all
> default via 192.168.0.2 dev eth0
> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
> medium
> default via fe80::2 dev eth0 metric 1024 pref medium
> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
> medium
> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
> medium
> # ./ip route list root default table all
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
> ...
> ff00::/8 dev eth0 table local metric 256 pref medium
> unreachable default dev lo proto kernel metric 4294967295 error -101 pref 
> medium
>
> And before patch:
>
> # ip route list default
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
> 192.168.1.0/24 via 192.168.0.3 dev eth0
> # ip route list match default
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
> 192.168.1.0/24 via 192.168.0.3 dev eth0
> # ip route list exact default
> default via 192.168.0.2 dev eth0
> 192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.1

[PATCH iproute2] ip route: broken logic when using default word and family not specified

[Alexander Zubkov]

Hello,

I have found odd behaviour when using "ip route list" (and other bound
commands) with prefix "default".

When family not specified, its value is completely ignored and "ip
route list default" shows all inet4 prefixes. Same do "ip route list
exact default" and "ip route list match default". Examples are at the
end of the message.

When family is specified, the behaviour changes and default works as
expected (=0.0.0.0/0 for inet4 and =::/0 for inet6). The above
commands all shows only default prefix in the output and only "root
default" shows all prefixes.

I tried to dig into the code and found that when default is using with
unspecified family - the resulting structures filter.[mr]dst will
actually become all-zeroes as in the case when nothing is specified.

I propose to change this in such a way (see attached patch). When
default prefix is parsed, the flag PREFIXLEN_SPECIFIED is attached to
it too, like for prefixes with "/". It seems logical to me,
because "/0" is really implied by "default" and even directly set up
in the code:

dst->bitlen = 0;

Then during filtering there is additional logic for unspecified family
and specified prefix.

With this patch ip route list commands shown above are working as
expected. And it also works with unspecified table when routes are
printed from different families.

Examples after applying the patch:

# ./ip route list
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
192.168.1.0/24 via 192.168.0.3 dev eth0
# ./ip -6 route list
fe80::/64 dev eth0 proto kernel metric 256 pref medium
fe80:1::/64 via fe80::3 dev eth0 metric 1024 pref medium
default via fe80::2 dev eth0 metric 1024 pref medium
# ./ip route list default
default via 192.168.0.2 dev eth0
# ./ip route list exact default
default via 192.168.0.2 dev eth0
# ./ip route list match default
default via 192.168.0.2 dev eth0
# ./ip route list root default
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
192.168.1.0/24 via 192.168.0.3 dev eth0
# ./ip route list default table all
default via 192.168.0.2 dev eth0
unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
default via fe80::2 dev eth0 metric 1024 pref medium
unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium
# ./ip route list root default table all
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.1
...
ff00::/8 dev eth0 table local metric 256 pref medium
unreachable default dev lo proto kernel metric 4294967295 error -101 pref medium

And before patch:

# ip route list default
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
192.168.1.0/24 via 192.168.0.3 dev eth0
# ip route list match default
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
192.168.1.0/24 via 192.168.0.3 dev eth0
# ip route list exact default
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
192.168.1.0/24 via 192.168.0.3 dev eth0
# ip -4 route list exact default
default via 192.168.0.2 dev eth0
# ip -6 route list exact default
default via fe80::2 dev eth0  metric 1024
# ip route list exact default table all
default via 192.168.0.2 dev eth0
192.168.0.0/24 dev eth0  proto kernel  scope link  src 192.168.0.1
...
local fe80::3c09:19ff:feee:9866 dev lo  table local  proto none  metric 0
ff00::/8 dev eth0  table local  metric 256
unreachable default dev lo  table unspec  proto kernel  metric
4294967295  error -101
--- 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))
 		return 0;
-	if (filter.rdst.family &&
-	(r->rtm_family != filter.rdst.family || filter.rdst.bitlen > r->rtm_dst_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 &&
-	(r->rtm_family != filter.mdst.family ||
-	 (filter.mdst.bitlen >= 0 && filter.mdst.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 &&
-	(r->rtm_family != filter.rsrc.family || filter.rsrc.bitlen > r->rtm_src_len))
-		return 0;
+	if (filter.rsrc.family) {
+		if (r->rtm_family != filter.rsrc.family ||
+		filter.rsrc.bitlen 

Re: iproute2: make ip route list to search by metric too

[Alexander Zubkov]

I think this version will be better. It uses metric mask (like for
some other options) instead of simple yes/no flag.

On Sat, Nov 18, 2017 at 2:44 AM, Alexander Zubkov <zubkov...@gmail.com> wrote:
> Hello again,
>
> Things turned out to be not so hard. Please take a look at the attached patch.
> I'm only not sure if RTA_PRIORITY is enough. Because the print_route
> function prints "metric" also for some situations with RTA_METRICS,
> which I haven't managed to understand.
>
> On Fri, Nov 17, 2017 at 1:40 AM, Alexander Zubkov <zubkov...@gmail.com> wrote:
>> Hello all,
>>
>> Currently routes in the Linux routing table have these "key" fields:
>> prefix, tos, table, metric (as I know). I.e. we cannot have two
>> different routes with the same set of this fields. And "ip route list"
>> command can be provided with all but one of those fields. We cannot
>> pass metric to it and this is inconvenient. I ask if this behaviour
>> can be changed by someone. We can even use "secondary" fields, for
>> example type, dev or via, but not metric unfortunately.
>> Sorry, I can not provide patches. I have written code long time ago. I
>> tried to trace it, but as I see it parses arguments and fills some
>> structures. And then my tries to understand failed.
>> I opened the bug: https://bugzilla.kernel.org/show_bug.cgi?id=197897,
>> but I was pointed out that this mailing list is a better place for
>> this question.
>>
>> --
>> Alexander Zubkov


list-metric.patch.2
Description: Binary data

Re: iproute2: make ip route list to search by metric too

[Alexander Zubkov]

Hello again,

Things turned out to be not so hard. Please take a look at the attached patch.
I'm only not sure if RTA_PRIORITY is enough. Because the print_route
function prints "metric" also for some situations with RTA_METRICS,
which I haven't managed to understand.

On Fri, Nov 17, 2017 at 1:40 AM, Alexander Zubkov <zubkov...@gmail.com> wrote:
> Hello all,
>
> Currently routes in the Linux routing table have these "key" fields:
> prefix, tos, table, metric (as I know). I.e. we cannot have two
> different routes with the same set of this fields. And "ip route list"
> command can be provided with all but one of those fields. We cannot
> pass metric to it and this is inconvenient. I ask if this behaviour
> can be changed by someone. We can even use "secondary" fields, for
> example type, dev or via, but not metric unfortunately.
> Sorry, I can not provide patches. I have written code long time ago. I
> tried to trace it, but as I see it parses arguments and fills some
> structures. And then my tries to understand failed.
> I opened the bug: https://bugzilla.kernel.org/show_bug.cgi?id=197897,
> but I was pointed out that this mailing list is a better place for
> this question.
>
> --
> Alexander Zubkov
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -126,6 +126,8 @@ static struct
 	int oif, oifmask;
 	int mark, markmask;
 	int realm, realmmask;
+	int have_metric;
+	__u32 metric;
 	inet_prefix rprefsrc;
 	inet_prefix rvia;
 	inet_prefix rdst;
@@ -288,6 +290,14 @@ static int filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len)
 		if ((mark ^ filter.mark) & filter.markmask)
 			return 0;
 	}
+	if (filter.have_metric) {
+		__u32 metric = 0;
+
+		if (tb[RTA_PRIORITY])
+			metric = rta_getattr_u32(tb[RTA_PRIORITY]);
+		if (filter.metric != metric)
+			return 0;
+	}
 	if (filter.flushb &&
 	r->rtm_family == AF_INET6 &&
 	r->rtm_dst_len == 0 &&
@@ -1518,6 +1528,16 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action)
 			if (get_unsigned(, *argv, 0))
 invarg("invalid mark value", *argv);
 			filter.markmask = -1;
+		} else if (matches(*argv, "metric") == 0 ||
+		   matches(*argv, "priority") == 0 ||
+		   strcmp(*argv, "preference") == 0) {
+			__u32 metric;
+
+			NEXT_ARG();
+			if (get_u32(, *argv, 0))
+invarg("\"metric\" value is invalid\n", *argv);
+			filter.metric = metric;
+			filter.have_metric = 1;
 		} else if (strcmp(*argv, "via") == 0) {
 			int family;
 

iproute2: make ip route list to search by metric too

[Alexander Zubkov]

Hello all,

Currently routes in the Linux routing table have these "key" fields:
prefix, tos, table, metric (as I know). I.e. we cannot have two
different routes with the same set of this fields. And "ip route list"
command can be provided with all but one of those fields. We cannot
pass metric to it and this is inconvenient. I ask if this behaviour
can be changed by someone. We can even use "secondary" fields, for
example type, dev or via, but not metric unfortunately.
Sorry, I can not provide patches. I have written code long time ago. I
tried to trace it, but as I see it parses arguments and fills some
structures. And then my tries to understand failed.
I opened the bug: https://bugzilla.kernel.org/show_bug.cgi?id=197897,
but I was pointed out that this mailing list is a better place for
this question.

--
Alexander Zubkov