Re: [iproute PATCH] ip-address: Fix filtering by negated address flags

[Stephen Hemminger]

On Wed, 14 Nov 2018 11:52:51 +0100
Phil Sutter <p...@nwl.cc> wrote:

> Hi Stephen,
> 
> On Tue, Nov 13, 2018 at 02:47:59PM -0800, Stephen Hemminger wrote:
> > On Tue, 13 Nov 2018 16:12:01 +0100
> > Phil Sutter <p...@nwl.cc> wrote:
> >   
> > > + if (arg[0] == '-') {
> > > +         inv = true;
> > > +         arg++;
> > > + }  
> > The inverse logic needs to be moved into the loop handling filter names.
> > 
> > Otherwise, you get weirdness like "-dynamic" being accepted and not
> > doing what was expected.  
> 
> I intentionally moved it there to allow for '-dynamic' and '-primary'
> as well. IMO this is consistent: 'dynamic' is the inverse of 'permanent'
> and 'primary' the inverse of 'secondary' but currently only '-permanent'
> and '-secondary' are allowed. With my patch applied, one may specify not
> only '-permanent' to get the same effect as 'dynamic' but also
> '-dynamic' to get the same effect as 'permanent'. Likewise for the other
> two. Did I miss something?
> 
> > Also, please make sure the man page matches the code.  
> 
> Oh, right. Given the above is fine with you, I will add the man page
> change in v2.
> 
> Thanks, Phil

I was thinking something like this which simplifies the logic.

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index cd8cc76a3473..3f1510383071 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1212,37 +1212,34 @@ static void print_ifa_flags(FILE *fp, const struct 
ifaddrmsg *ifa,
 static int get_filter(const char *arg)
 {
        unsigned int i;
+       bool inv = false;
 
        /* Special cases */
        if (strcmp(arg, "dynamic") == 0) {
-               filter.flags &= ~IFA_F_PERMANENT;
-               filter.flagmask |= IFA_F_PERMANENT;
+               arg = "-permanent";
        } else if (strcmp(arg, "primary") == 0) {
-               filter.flags &= ~IFA_F_SECONDARY;
-               filter.flagmask |= IFA_F_SECONDARY;
-       } else if (*arg == '-') {
-               for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
-                       if (strcmp(arg + 1, ifa_flag_names[i].name))
-                               continue;
+               arg = "-secondary";
+       }
 
-                       filter.flags &= ifa_flag_names[i].value;
-                       filter.flagmask |= ifa_flag_names[i].value;
-                       return 0;
-               }
+       if (*arg == '-') {
+               inv = true;
+               ++arg;
+       }
 
-               return -1;
-       } else {
-               for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
-                       if (strcmp(arg, ifa_flag_names[i].name))
-                               continue;
+       for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
+               if (strcmp(arg, ifa_flag_names[i].name))
+                       continue;
+
+               if (inv) {
+                       filter.flags &= ~ifa_flag_names[i].value;
+                       filter.flagmask |= ifa_flag_names[i].value;
+               } else {
                        filter.flags |= ifa_flag_names[i].value;
                        filter.flagmask |= ifa_flag_names[i].value;
-                       return 0;
                }
-               return -1;
+               return 0;
        }
-
-       return 0;
+       return -1;
 }
 
 static int ifa_label_match_rta(int ifindex, const struct rtattr *rta)