[PATCH iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[PATCH iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Hangbin Liu]

After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
entry now. But `ip neigh flush` still skip the FAILED entry.

Let's remove this filter so we can also flush FAILED entry.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 ip/ipneigh.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ip/ipneigh.c b/ip/ipneigh.c
index 4d8fc85..8082fa8 100644
--- a/ip/ipneigh.c
+++ b/ip/ipneigh.c
@@ -445,7 +445,6 @@ static int do_show_or_flush(int argc, char **argv, int flush)
 		filter.flushb = flushb;
 		filter.flushp = 0;
 		filter.flushe = sizeof(flushb);
-		filter.state &= ~NUD_FAILED;
 
 		while (round < MAX_ROUNDS) {
 			if (rtnl_dump_request_n(&rth, &req.n) < 0) {
-- 
2.5.5

Re: [PATCH iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Stephen Hemminger]

On Mon,  5 Jun 2017 16:31:29 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
> on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
> entry now. But `ip neigh flush` still skip the FAILED entry.
> 
> Let's remove this filter so we can also flush FAILED entry.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

This might create a problem. iproute2 has to be forward/backwards compatiable
with multiple kernel versions. Users must be able to run newer versions of ip
command on older kernels.

What happens if you run the ip command with your patch on older kernels?

Re: [PATCH iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Hangbin Liu]

Hi Stephen,
On Wed, Jun 14, 2017 at 09:54:50AM -0700, Stephen Hemminger wrote:
> On Mon,  5 Jun 2017 16:31:29 +0800
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> > After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
> > on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
> > entry now. But `ip neigh flush` still skip the FAILED entry.
> > 
> > Let's remove this filter so we can also flush FAILED entry.
> > 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> This might create a problem. iproute2 has to be forward/backwards compatiable
> with multiple kernel versions. Users must be able to run newer versions of ip
> command on older kernels.
> 
> What happens if you run the ip command with your patch on older kernels?
> 
On older kernel with tihs patch. The result is same, we could not delete
FAILED entry. Since we could not delete it, `ip neigh` will try MAX_ROUNDS
and fail at the end. But IMHO, this should be the correct behavoir. Because
this is just what kernel did and what kernel fixed now.

Here is the result on old kernel:

With unpatched ip cmd
# ip -4 neigh show dev eth1
192.168.1.5  FAILED
192.168.1.4  FAILED
192.168.1.3  FAILED
192.168.1.6  FAILED
192.168.1.2  FAILED
# ip -4 neigh flush dev eth1
# ip -4 -s neigh flush dev eth1
Nothing to flush.
# echo $?
0

With patched ip cmd
# ./ip -4 neigh flush dev eth1
*** Flush not complete bailing out after 10 rounds
# ./ip -4 -s neigh flush dev eth1

*** Round 1, deleting 5 entries ***

*** Round 2, deleting 5 entries ***

*** Round 3, deleting 5 entries ***

*** Round 4, deleting 5 entries ***

*** Round 5, deleting 5 entries ***

*** Round 6, deleting 5 entries ***

*** Round 7, deleting 5 entries ***

*** Round 8, deleting 5 entries ***

*** Round 9, deleting 5 entries ***

*** Round 10, deleting 5 entries ***
*** Flush not complete bailing out after 10 rounds
# echo $?
255

But if you thought the message reallly annoying. Then how about this patch,
which is a little tricky.

diff --git a/ip/ipneigh.c b/ip/ipneigh.c
index 4d8fc85..9088a05 100644
--- a/ip/ipneigh.c
+++ b/ip/ipneigh.c
@@ -445,7 +445,7 @@ static int do_show_or_flush(int argc, char **argv, int flush)
                filter.flushb = flushb;
                filter.flushp = 0;
                filter.flushe = sizeof(flushb);
-               filter.state &= ~NUD_FAILED;

                while (round < MAX_ROUNDS) {
                        if (rtnl_dump_request_n(&rth, &req.n) < 0) {
@@ -474,6 +474,7 @@ static int do_show_or_flush(int argc, char **argv, int flush)
                                printf("\n*** Round %d, deleting %d entries ***\n", round, filter.flushed);
                                fflush(stdout);
                        }
+                       filter.state &= ~NUD_FAILED;
                }
                printf("*** Flush not complete bailing out after %d rounds\n",
                        MAX_ROUNDS);


With this patch, the result will looks almost the same with old behavior,
except it will filter out the FAILED entry the first time.

# ./ip -4 neigh flush dev eth1
# ./ip -4 -s neigh flush dev eth1

*** Round 1, deleting 5 entries ***
*** Flush is complete after 1 round ***
# echo $?
0


Thanks
Hangbin

Re: [PATCH iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Stephen Hemminger]

On Thu, 15 Jun 2017 10:30:16 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> Hi Stephen,
> On Wed, Jun 14, 2017 at 09:54:50AM -0700, Stephen Hemminger wrote:
> > On Mon,  5 Jun 2017 16:31:29 +0800
> > Hangbin Liu <liuhangbin@gmail.com> wrote:
> >   
> > > After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
> > > on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
> > > entry now. But `ip neigh flush` still skip the FAILED entry.
> > > 
> > > Let's remove this filter so we can also flush FAILED entry.
> > > 
> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>  
> > 
> > This might create a problem. iproute2 has to be forward/backwards compatiable
> > with multiple kernel versions. Users must be able to run newer versions of ip
> > command on older kernels.
> > 
> > What happens if you run the ip command with your patch on older kernels?
> >   
> On older kernel with tihs patch. The result is same, we could not delete
> FAILED entry. Since we could not delete it, `ip neigh` will try MAX_ROUNDS
> and fail at the end. But IMHO, this should be the correct behavoir. Because
> this is just what kernel did and what kernel fixed now.
> 
> Here is the result on old kernel:
> 
> With unpatched ip cmd
> # ip -4 neigh show dev eth1
> 192.168.1.5  FAILED
> 192.168.1.4  FAILED
> 192.168.1.3  FAILED
> 192.168.1.6  FAILED
> 192.168.1.2  FAILED
> # ip -4 neigh flush dev eth1
> # ip -4 -s neigh flush dev eth1
> Nothing to flush.
> # echo $?
> 0
> 
> With patched ip cmd
> # ./ip -4 neigh flush dev eth1
> *** Flush not complete bailing out after 10 rounds
> # ./ip -4 -s neigh flush dev eth1
> 
> *** Round 1, deleting 5 entries ***
> 
> *** Round 2, deleting 5 entries ***
> 
> *** Round 3, deleting 5 entries ***
> 
> *** Round 4, deleting 5 entries ***
> 
> *** Round 5, deleting 5 entries ***
> 
> *** Round 6, deleting 5 entries ***
> 
> *** Round 7, deleting 5 entries ***
> 
> *** Round 8, deleting 5 entries ***
> 
> *** Round 9, deleting 5 entries ***
> 
> *** Round 10, deleting 5 entries ***
> *** Flush not complete bailing out after 10 rounds
> # echo $?
> 255
> 
> But if you thought the message reallly annoying. Then how about this patch,
> which is a little tricky.
> 
> diff --git a/ip/ipneigh.c b/ip/ipneigh.c
> index 4d8fc85..9088a05 100644
> --- a/ip/ipneigh.c
> +++ b/ip/ipneigh.c
> @@ -445,7 +445,7 @@ static int do_show_or_flush(int argc, char **argv, int flush)
>                 filter.flushb = flushb;
>                 filter.flushp = 0;
>                 filter.flushe = sizeof(flushb);
> -               filter.state &= ~NUD_FAILED;
> 
>                 while (round < MAX_ROUNDS) {
>                         if (rtnl_dump_request_n(&rth, &req.n) < 0) {
> @@ -474,6 +474,7 @@ static int do_show_or_flush(int argc, char **argv, int flush)
>                                 printf("\n*** Round %d, deleting %d entries ***\n", round, filter.flushed);
>                                 fflush(stdout);
>                         }
> +                       filter.state &= ~NUD_FAILED;
>                 }
>                 printf("*** Flush not complete bailing out after %d rounds\n",
>                         MAX_ROUNDS);
> 
> 
> With this patch, the result will looks almost the same with old behavior,
> except it will filter out the FAILED entry the first time.
> 
> # ./ip -4 neigh flush dev eth1
> # ./ip -4 -s neigh flush dev eth1
> 
> *** Round 1, deleting 5 entries ***
> *** Flush is complete after 1 round ***
> # echo $?
> 0
> 
> 
> Thanks
> Hangbin

That looks better, could you send an updated patch. I will apply that

[PATCHv2 iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Hangbin Liu]

After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
entry now. But `ip neigh flush` still skip the FAILED entry.

Move the filter after first round flush so we can flush FAILED entry on fixed
kernel and also do not keep retrying on old kernel.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 ip/ipneigh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/ipneigh.c b/ip/ipneigh.c
index 4d8fc85..9c38a60 100644
--- a/ip/ipneigh.c
+++ b/ip/ipneigh.c
@@ -445,7 +445,6 @@ static int do_show_or_flush(int argc, char **argv, int flush)
 		filter.flushb = flushb;
 		filter.flushp = 0;
 		filter.flushe = sizeof(flushb);
-		filter.state &= ~NUD_FAILED;
 
 		while (round < MAX_ROUNDS) {
 			if (rtnl_dump_request_n(&rth, &req.n) < 0) {
@@ -474,6 +473,7 @@ static int do_show_or_flush(int argc, char **argv, int flush)
 				printf("\n*** Round %d, deleting %d entries ***\n", round, filter.flushed);
 				fflush(stdout);
 			}
+			filter.state &= ~NUD_FAILED;
 		}
 		printf("*** Flush not complete bailing out after %d rounds\n",
 			MAX_ROUNDS);
-- 
2.5.5

Re: [PATCHv2 iproute2 net-next] ip neigh: allow flush FAILED neighbour entry

[Stephen Hemminger]

On Fri, 16 Jun 2017 11:31:52 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:

> After upstream commit 5071034e4af7 ('neigh: Really delete an arp/neigh entry
> on "ip neigh delete" or "arp -d"'), we could delete a single FAILED neighbour
> entry now. But `ip neigh flush` still skip the FAILED entry.
> 
> Move the filter after first round flush so we can flush FAILED entry on fixed
> kernel and also do not keep retrying on old kernel.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied, thanks.