[iproute2] iproute2: Fix 'addr flush secondary' logic.

[iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

It appears that the check for secondary v/s primary was reversed
when flushing addresses.  This caused:
ip addr flush dev eth0 secondary
to not actually flush anything.

This patch makes the check for IFA_F_SECONDARY match the
function names.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 5f0789c... 77b400d... M	ip/ipaddress.c
 ip/ipaddress.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5f0789c..77b400d 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -637,7 +637,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (!ifa->ifa_flags & IFA_F_SECONDARY)
+	if (ifa->ifa_flags & IFA_F_SECONDARY)
 		return 0;
 
 	return print_addrinfo(who, n, arg);
@@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
+	if (!ifa->ifa_flags & IFA_F_SECONDARY)
 		return 0;
 
 	return print_addrinfo(who, n, arg);
-- 
1.6.2.5

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Brian Haley]

On 08/11/2010 01:19 PM, Ben Greear wrote:
> @@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>  {
>  	struct ifaddrmsg *ifa = NLMSG_DATA(n);
>  
> -	if (ifa->ifa_flags & IFA_F_SECONDARY)
> +	if (!ifa->ifa_flags & IFA_F_SECONDARY)
>  		return 0;

Shouldn't this be:

	if (!(ifa->ifa_flags & IFA_F_SECONDARY))

-Brian

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

On 08/11/2010 01:41 PM, Brian Haley wrote:
> On 08/11/2010 01:19 PM, Ben Greear wrote:
>> @@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>>   {
>>   	struct ifaddrmsg *ifa = NLMSG_DATA(n);
>>
>> -	if (ifa->ifa_flags&  IFA_F_SECONDARY)
>> +	if (!ifa->ifa_flags&  IFA_F_SECONDARY)
>>   		return 0;
>
> Shouldn't this be:
>
> 	if (!(ifa->ifa_flags&  IFA_F_SECONDARY))

Yes, that looks like a bug.  Let me re-test to make sure
it wasn't hiding other bugs and will re-submit.

Thanks,
Ben

>
> -Brian


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On 08/11/2010 01:41 PM, Brian Haley wrote:
> On 08/11/2010 01:19 PM, Ben Greear wrote:
>> @@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>>   {
>>   	struct ifaddrmsg *ifa = NLMSG_DATA(n);
>>
>> -	if (ifa->ifa_flags&  IFA_F_SECONDARY)
>> +	if (!ifa->ifa_flags&  IFA_F_SECONDARY)
>>   		return 0;
>
> Shouldn't this be:
>
> 	if (!(ifa->ifa_flags&  IFA_F_SECONDARY))
>
> -Brian

Looks like the code was broken in several different places.

* It ran only a single filter if there were multiple.
* Don't want to flush in a loop if you are doing primary
   because otherwise promoted seconaries will get deleted
   for each additional loop (10 in upstream code).
* No idea what a while (0); statement at the end of a for
   loop does, but I don't think it needed to be there!

The attached patch makes it work for me, supporting
flushing primary or secondary addresses.

It could certainly use some review, however.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


[-- Attachment #2: iproute_flush.patch --]
[-- Type: text/plain, Size: 2813 bytes --]

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5f0789c..803df17 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -637,7 +637,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (!ifa->ifa_flags & IFA_F_SECONDARY)
+	if (ifa->ifa_flags & IFA_F_SECONDARY)
 		return 0;
 
 	return print_addrinfo(who, n, arg);
@@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
+	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
 		return 0;
 
 	return print_addrinfo(who, n, arg);
@@ -865,6 +865,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
 				printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
 				fflush(stdout);
 			}
+
+			/* If we are flushing, and specifying primary, then we want to flush only a single round.
+			 * Otherwise, we'll start flushing secondaries that were promoted to primaries
+			 */
+			if (!(filter.flags & IFA_F_SECONDARY) && (filter.flagmask & IFA_F_SECONDARY)) {
+				return 0;
+			}
 		}
 		fprintf(stderr, "*** Flush remains incomplete after %d rounds. ***\n", max_flush_loops);
 		fflush(stderr);
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index cfeb894..d18e8a0 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -189,6 +189,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 	while (1) {
 		int status;
 		const struct rtnl_dump_filter_arg *a;
+		int found_done = 0;
+		int msglen = 0;
 
 		iov.iov_len = sizeof(buf);
 		status = recvmsg(rth->fd, &msg, 0);
@@ -208,8 +210,9 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 
 		for (a = arg; a->filter; a++) {
 			struct nlmsghdr *h = (struct nlmsghdr*)buf;
+			msglen = status;
 
-			while (NLMSG_OK(h, status)) {
+			while (NLMSG_OK(h, msglen)) {
 				int err;
 
 				if (nladdr.nl_pid != 0 ||
@@ -224,8 +227,10 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					goto skip_it;
 				}
 
-				if (h->nlmsg_type == NLMSG_DONE)
-					return 0;
+				if (h->nlmsg_type == NLMSG_DONE) {
+					found_done = 1;
+					break; /* process next filter */
+				}
 				if (h->nlmsg_type == NLMSG_ERROR) {
 					struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
 					if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
@@ -242,14 +247,18 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					return err;
 
 skip_it:
-				h = NLMSG_NEXT(h, status);
+				h = NLMSG_NEXT(h, msglen);
 			}
-		} while (0);
+		}
+
+		if (found_done)
+			return 0;
+
 		if (msg.msg_flags & MSG_TRUNC) {
 			fprintf(stderr, "Message truncated\n");
 			continue;
 		}
-		if (status) {
+		if (msglen) {
 			fprintf(stderr, "!!!Remnant of size %d\n", status);
 			exit(1);
 		}

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Brian Haley]

On 08/11/2010 06:48 PM, Ben Greear wrote:
> Looks like the code was broken in several different places.
> 
> * It ran only a single filter if there were multiple.
> * Don't want to flush in a loop if you are doing primary
>   because otherwise promoted seconaries will get deleted
>   for each additional loop (10 in upstream code).
> * No idea what a while (0); statement at the end of a for
>   loop does, but I don't think it needed to be there!
> 
> The attached patch makes it work for me, supporting
> flushing primary or secondary addresses.

> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 5f0789c..803df17 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -637,7 +637,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>  {
>  	struct ifaddrmsg *ifa = NLMSG_DATA(n);
> 
> -	if (!ifa->ifa_flags & IFA_F_SECONDARY)
> +	if (ifa->ifa_flags & IFA_F_SECONDARY)
 		return 0;

This should be:

	if (!(ifa->ifa_flags & IFA_F_SECONDARY))

as this function does the opposite of what it seems.
 

> @@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>  {
> 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
> 
> -	if (ifa->ifa_flags & IFA_F_SECONDARY)
> +	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
 		return 0;

>From testing, the original code here was correct.

> @@ -865,6 +865,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
> 				printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
> 				fflush(stdout);
> 			}
> +
> +			/* If we are flushing, and specifying primary, then we want to flush only a single round.
> +			 * Otherwise, we'll start flushing secondaries that were promoted to primaries
> +			 */
> +			if (!(filter.flags & IFA_F_SECONDARY) && (filter.flagmask & IFA_F_SECONDARY)) {
> +				return 0;
> +			}

This doesn't seem to do anything, I see all my IPv4 addresses flushed if I
run 'ip -4 -s a flush primary dev eth2'.  And it says it only deleted one
when it deleted two addresses :-/  Also, if it did work, it should really goto
a few lines above so it prints the summary stats:

                        if (filter.flushed == 0) {
flush_done:
                                if (show_stats) {

Even when I corrected the lines above, it didn't work:

# ip -4 a s dev eth2
2: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
    inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2

# ip a a 192.168.6.100/24 brd + dev eth2
# ip -4 a s dev eth2
2: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
    inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2
    inet 192.168.6.100/24 brd 192.168.6.255 scope global secondary eth2

# ./ip -4 -s -s -o a flush primary dev eth2
2: eth2    inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2

*** Round 1, deleting 1 addresses ***
[missing *** Flush is complete after 1 round ***]

# ip -4 a s dev eth2
[nothing]

Where did my .100 secondary address go?  Now this will bug me until I figure
out why.  Maybe it's because I'm booted to 2.6.32.


> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index cfeb894..d18e8a0 100644

Can you give an example of how you tested this double filter change?  My
distro /sbin/ip seemed to work just fine.

> -		if (status) {
> +		if (msglen) {
> 			fprintf(stderr, "!!!Remnant of size %d\n", status);
> 			exit(1);
> 		}

Should the arg to the fprintf() be msglen too?

-Brian

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

On 08/12/2010 12:00 PM, Brian Haley wrote:
> On 08/11/2010 06:48 PM, Ben Greear wrote:
>> Looks like the code was broken in several different places.
>>
>> * It ran only a single filter if there were multiple.
>> * Don't want to flush in a loop if you are doing primary
>>    because otherwise promoted seconaries will get deleted
>>    for each additional loop (10 in upstream code).
>> * No idea what a while (0); statement at the end of a for
>>    loop does, but I don't think it needed to be there!
>>
>> The attached patch makes it work for me, supporting
>> flushing primary or secondary addresses.
>
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 5f0789c..803df17 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -637,7 +637,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>>   {
>>   	struct ifaddrmsg *ifa = NLMSG_DATA(n);
>>
>> -	if (!ifa->ifa_flags&  IFA_F_SECONDARY)
>> +	if (ifa->ifa_flags&  IFA_F_SECONDARY)
>   		return 0;
>
> This should be:
>
> 	if (!(ifa->ifa_flags&  IFA_F_SECONDARY))
>
> as this function does the opposite of what it seems.

I see no reason to let it do the opposite of what it seems.

 From what I can tell, the original code never even called this
method anyway since it was the second filter and only the first
filter was used.


>> @@ -648,7 +648,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
>>   {
>> 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
>>
>> -	if (ifa->ifa_flags&  IFA_F_SECONDARY)
>> +	if (!(ifa->ifa_flags&  IFA_F_SECONDARY))
>   		return 0;
>
>> From testing, the original code here was correct.

It deleted addresses, but not how it was intended to work,
I think.

>> @@ -865,6 +865,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
>> 				printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
>> 				fflush(stdout);
>> 			}
>> +
>> +			/* If we are flushing, and specifying primary, then we want to flush only a single round.
>> +			 * Otherwise, we'll start flushing secondaries that were promoted to primaries
>> +			 */
>> +			if (!(filter.flags&  IFA_F_SECONDARY)&&  (filter.flagmask&  IFA_F_SECONDARY)) {
>> +				return 0;
>> +			}
>
> This doesn't seem to do anything, I see all my IPv4 addresses flushed if I
> run 'ip -4 -s a flush primary dev eth2'.  And it says it only deleted one
> when it deleted two addresses :-/  Also, if it did work, it should really goto
> a few lines above so it prints the summary stats:
>
>                          if (filter.flushed == 0) {
> flush_done:
>                                  if (show_stats) {
>
> Even when I corrected the lines above, it didn't work:
>
> # ip -4 a s dev eth2
> 2: eth2:<BROADCAST,MULTICAST,UP,LOWER_UP>  mtu 1500 qdisc mq state UP qlen 1000
>      inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2
>
> # ip a a 192.168.6.100/24 brd + dev eth2
> # ip -4 a s dev eth2
> 2: eth2:<BROADCAST,MULTICAST,UP,LOWER_UP>  mtu 1500 qdisc mq state UP qlen 1000
>      inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2
>      inet 192.168.6.100/24 brd 192.168.6.255 scope global secondary eth2
>
> # ./ip -4 -s -s -o a flush primary dev eth2
> 2: eth2    inet 192.168.6.4/24 brd 192.168.6.255 scope global eth2
>
> *** Round 1, deleting 1 addresses ***
> [missing *** Flush is complete after 1 round ***]
>
> # ip -4 a s dev eth2
> [nothing]
>
> Where did my .100 secondary address go?  Now this will bug me until I figure
> out why.  Maybe it's because I'm booted to 2.6.32.

Do you have the 'promote secondaries' sysctl enabled?  I think you
need that to have the secondaries not just dissappear upon removal
of the primary.

>> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
>> index cfeb894..d18e8a0 100644
>
> Can you give an example of how you tested this double filter change?  My
> distro /sbin/ip seemed to work just fine.
>
>> -		if (status) {
>> +		if (msglen) {
>> 			fprintf(stderr, "!!!Remnant of size %d\n", status);
>> 			exit(1);
>> 		}
>
> Should the arg to the fprintf() be msglen too?

Yes, that should be fixed.

Thanks for the review..I'll make this change and show you the commands
I used for testing.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

[-- Attachment #1: Type: text/plain, Size: 4988 bytes --]

On 08/12/2010 12:00 PM, Brian Haley wrote:
> On 08/11/2010 06:48 PM, Ben Greear wrote:
>> Looks like the code was broken in several different places.
>>
>> * It ran only a single filter if there were multiple.
>> * Don't want to flush in a loop if you are doing primary
>>    because otherwise promoted seconaries will get deleted
>>    for each additional loop (10 in upstream code).
>> * No idea what a while (0); statement at the end of a for
>>    loop does, but I don't think it needed to be there!

Attached is an updated patch to fix some of the errors you
mentioned.  It also fixes a bug when '-s -s' is used:  The old
code modified the ifa flags, which made it not handle the check
for primary v/s secondary correctly in the next filter.


Here are my testing commands:

Another thing you might find interesting is to run 'ip monitor'
in another window.  It should show the secondaries getting deleted
before the primaries if you flush all (with my modification).  Without
my patch, you should see only the primary being deleted in each
round.

[root@ct503-10G-09 lanforge]# cat /proc/sys/net/ipv4/conf/all/promote_secondaries
1

[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.101/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.102/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.103/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip -s -s addr flush dev eth3
5: eth3    inet 172.16.223.102/16 scope global secondary eth3
5: eth3    inet 172.16.223.103/16 scope global secondary eth3
5: eth3    inet 172.16.223.101/16 scope global eth3

*** Round 1, deleting 3 addresses ***
*** Flush is complete after 1 round ***

[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.101/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.102/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.103/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip -s  addr flush dev eth3

*** Round 1, deleting 3 addresses ***
*** Flush is complete after 1 round ***

[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.101/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.102/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.103/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff
     inet 172.16.223.101/16 scope global eth3
     inet 172.16.223.102/16 scope global secondary eth3
     inet 172.16.223.103/16 scope global secondary eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr flush dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff


[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.101/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.102/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr add 172.16.223.103/16 dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff
     inet 172.16.223.101/16 scope global eth3
     inet 172.16.223.102/16 scope global secondary eth3
     inet 172.16.223.103/16 scope global secondary eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr flush dev eth3 primary
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff
     inet 172.16.223.102/16 scope global eth3
     inet 172.16.223.103/16 scope global secondary eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr flush dev eth3 secondary
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff
     inet 172.16.223.102/16 scope global eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr flush dev eth3 secondary
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff
     inet 172.16.223.102/16 scope global eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr flush dev eth3
[root@ct503-10G-09 lanforge]# ./local/sbin/ip addr show dev eth3
5: eth3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 1000
     link/ether 00:0c:bd:00:9b:1b brd ff:ff:ff:ff:ff:ff


Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


[-- Attachment #2: iproute_flush.patch --]
[-- Type: text/plain, Size: 4990 bytes --]

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5f0789c..51b59b4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -452,6 +452,7 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
 	int deprecated = 0;
+	unsigned int ifa_flags;
 	struct rtattr * rta_tb[IFA_MAX+1];
 	char abuf[256];
 	SPRINT_BUF(b1);
@@ -571,40 +572,41 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 				    abuf, sizeof(abuf)));
 	}
 	fprintf(fp, "scope %s ", rtnl_rtscope_n2a(ifa->ifa_scope, b1, sizeof(b1)));
+	ifa_flags = ifa->ifa_flags;
 	if (ifa->ifa_flags&IFA_F_SECONDARY) {
-		ifa->ifa_flags &= ~IFA_F_SECONDARY;
+		ifa_flags &= ~IFA_F_SECONDARY;
 		if (ifa->ifa_family == AF_INET6)
 			fprintf(fp, "temporary ");
 		else
 			fprintf(fp, "secondary ");
 	}
 	if (ifa->ifa_flags&IFA_F_TENTATIVE) {
-		ifa->ifa_flags &= ~IFA_F_TENTATIVE;
+		ifa_flags &= ~IFA_F_TENTATIVE;
 		fprintf(fp, "tentative ");
 	}
 	if (ifa->ifa_flags&IFA_F_DEPRECATED) {
-		ifa->ifa_flags &= ~IFA_F_DEPRECATED;
+		ifa_flags &= ~IFA_F_DEPRECATED;
 		deprecated = 1;
 		fprintf(fp, "deprecated ");
 	}
 	if (ifa->ifa_flags&IFA_F_HOMEADDRESS) {
-		ifa->ifa_flags &= ~IFA_F_HOMEADDRESS;
+		ifa_flags &= ~IFA_F_HOMEADDRESS;
 		fprintf(fp, "home ");
 	}
 	if (ifa->ifa_flags&IFA_F_NODAD) {
-		ifa->ifa_flags &= ~IFA_F_NODAD;
+		ifa_flags &= ~IFA_F_NODAD;
 		fprintf(fp, "nodad ");
 	}
 	if (!(ifa->ifa_flags&IFA_F_PERMANENT)) {
 		fprintf(fp, "dynamic ");
 	} else
-		ifa->ifa_flags &= ~IFA_F_PERMANENT;
+		ifa_flags &= ~IFA_F_PERMANENT;
 	if (ifa->ifa_flags&IFA_F_DADFAILED) {
-		ifa->ifa_flags &= ~IFA_F_DADFAILED;
+		ifa_flags &= ~IFA_F_DADFAILED;
 		fprintf(fp, "dadfailed ");
 	}
-	if (ifa->ifa_flags)
-		fprintf(fp, "flags %02x ", ifa->ifa_flags);
+	if (ifa_flags)
+		fprintf(fp, "flags %02x ", ifa_flags);
 	if (rta_tb[IFA_LABEL])
 		fprintf(fp, "%s", (char*)RTA_DATA(rta_tb[IFA_LABEL]));
 	if (rta_tb[IFA_CACHEINFO]) {
@@ -637,7 +639,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (!ifa->ifa_flags & IFA_F_SECONDARY)
+	if (ifa->ifa_flags & IFA_F_SECONDARY)
 		return 0;
 
 	return print_addrinfo(who, n, arg);
@@ -648,7 +650,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
 {
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 
-	if (ifa->ifa_flags & IFA_F_SECONDARY)
+	if (!(ifa->ifa_flags & IFA_F_SECONDARY))
 		return 0;
 
 	return print_addrinfo(who, n, arg);
@@ -848,6 +850,7 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
 				exit(1);
 			}
 			if (filter.flushed == 0) {
+			flush_done:
 				if (show_stats) {
 					if (round == 0)
 						printf("Nothing to flush.\n");
@@ -865,6 +868,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
 				printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
 				fflush(stdout);
 			}
+
+			/* If we are flushing, and specifying primary, then we want to flush only a single round.
+			 * Otherwise, we'll start flushing secondaries that were promoted to primaries
+			 */
+			if (!(filter.flags & IFA_F_SECONDARY) && (filter.flagmask & IFA_F_SECONDARY)) {
+				goto flush_done;
+			}
 		}
 		fprintf(stderr, "*** Flush remains incomplete after %d rounds. ***\n", max_flush_loops);
 		fflush(stderr);
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index cfeb894..ee4f045 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -189,6 +189,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 	while (1) {
 		int status;
 		const struct rtnl_dump_filter_arg *a;
+		int found_done = 0;
+		int msglen = 0;
 
 		iov.iov_len = sizeof(buf);
 		status = recvmsg(rth->fd, &msg, 0);
@@ -208,8 +210,9 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 
 		for (a = arg; a->filter; a++) {
 			struct nlmsghdr *h = (struct nlmsghdr*)buf;
+			msglen = status;
 
-			while (NLMSG_OK(h, status)) {
+			while (NLMSG_OK(h, msglen)) {
 				int err;
 
 				if (nladdr.nl_pid != 0 ||
@@ -224,8 +227,10 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					goto skip_it;
 				}
 
-				if (h->nlmsg_type == NLMSG_DONE)
-					return 0;
+				if (h->nlmsg_type == NLMSG_DONE) {
+					found_done = 1;
+					break; /* process next filter */
+				}
 				if (h->nlmsg_type == NLMSG_ERROR) {
 					struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
 					if (h->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
@@ -242,15 +247,19 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
 					return err;
 
 skip_it:
-				h = NLMSG_NEXT(h, status);
+				h = NLMSG_NEXT(h, msglen);
 			}
-		} while (0);
+		}
+
+		if (found_done)
+			return 0;
+
 		if (msg.msg_flags & MSG_TRUNC) {
 			fprintf(stderr, "Message truncated\n");
 			continue;
 		}
-		if (status) {
-			fprintf(stderr, "!!!Remnant of size %d\n", status);
+		if (msglen) {
+			fprintf(stderr, "!!!Remnant of size %d\n", msglen);
 			exit(1);
 		}
 	}

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Brian Haley]

On 08/13/2010 03:49 PM, Ben Greear wrote:
> Attached is an updated patch to fix some of the errors you
> mentioned.  It also fixes a bug when '-s -s' is used:  The old
> code modified the ifa flags, which made it not handle the check
> for primary v/s secondary correctly in the next filter.

Thanks for the update, nitpicking comments below.

BTW, promote_secondaries was the reason for some of my testing
issues as I don't usually run with that enabled.

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 5f0789c..51b59b4 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c

> @@ -571,40 +572,41 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
> 				    abuf, sizeof(abuf)));
> 	}
> 	fprintf(fp, "scope %s ", rtnl_rtscope_n2a(ifa->ifa_scope, b1, sizeof(b1)));
> +	ifa_flags = ifa->ifa_flags;
> 	if (ifa->ifa_flags&IFA_F_SECONDARY) {

Maybe put a comment as to the reason, so someone doesn't change it
back in the future:

/* Use local copy of flags to not interfere with filtering code */

> @@ -848,6 +850,7 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
> 				exit(1);
> 			}
> 			if (filter.flushed == 0) {
> +			flush_done:
>  				if (show_stats) {
> 					if (round == 0)
> 						printf("Nothing to flush.\n");

Can you put "flush_done" at the beginning of the line?

> @@ -865,6 +868,13 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
> 				printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
> 				fflush(stdout);
> 			}
> +
> +			/* If we are flushing, and specifying primary, then we want to flush only a single round.
> +			 * Otherwise, we'll start flushing secondaries that were promoted to primaries
> +			 */
> +			if (!(filter.flags & IFA_F_SECONDARY) && (filter.flagmask & IFA_F_SECONDARY)) {
> +				goto flush_done;
> +			}

You don't need the {} here.  Also, the comment line-wraps since it's
> 80 chars.

Everything seems to work correctly in my limited testing otherwise, thanks.

Tested-by: Brian Haley <brian.haley@hp.com>

-Brian

Re: [iproute2] iproute2: Fix 'addr flush secondary' logic.

[Ben Greear]

On 08/16/2010 08:58 AM, Brian Haley wrote:
> On 08/13/2010 03:49 PM, Ben Greear wrote:
>> Attached is an updated patch to fix some of the errors you
>> mentioned.  It also fixes a bug when '-s -s' is used:  The old
>> code modified the ifa flags, which made it not handle the check
>> for primary v/s secondary correctly in the next filter.
>
> Thanks for the update, nitpicking comments below.
>
> BTW, promote_secondaries was the reason for some of my testing
> issues as I don't usually run with that enabled.

Thanks for all the help and suggestions.  I just sent an
'official' patch with your final suggestions included.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com