diff mbox

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

Message ID 4C6328A8.4070703@candelatech.com
State Superseded, archived
Delegated to: stephen hemminger
Headers show

Commit Message

Ben Greear Aug. 11, 2010, 10:48 p.m. UTC 
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

Comments

Brian Haley Aug. 12, 2010, 7 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Aug. 12, 2010, 7:10 p.m. UTC | #2 
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
diff mbox

Patch

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);
 		}