Message ID | 20181113151201.19297-1-phil@nwl.cc |
---|---|
State | Changes Requested, archived |
Delegated to: | stephen hemminger |
Headers | show |
Series | [iproute] ip-address: Fix filtering by negated address flags | expand |
On Tue, 13 Nov 2018 16:12:01 +0100 Phil Sutter <phil@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. Also, please make sure the man page matches the code.
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 <phil@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
On Wed, 14 Nov 2018 11:52:51 +0100 Phil Sutter <phil@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 <phil@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)
Hi Stephen, On Wed, Nov 14, 2018 at 11:29:03AM -0800, Stephen Hemminger wrote: [...] > 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"; I like this idea, also because it's much easier to get how 'dynamic' and 'primary' are special. I'll respin then. Thanks, Phil
diff --git a/ip/ipaddress.c b/ip/ipaddress.c index cd8cc76a3473f..7b98877085878 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -1211,31 +1211,35 @@ static void print_ifa_flags(FILE *fp, const struct ifaddrmsg *ifa, static int get_filter(const char *arg) { + bool inv = false; unsigned int i; + if (arg[0] == '-') { + inv = true; + arg++; + } /* Special cases */ if (strcmp(arg, "dynamic") == 0) { - filter.flags &= ~IFA_F_PERMANENT; + if (inv) + filter.flags |= IFA_F_PERMANENT; + else + filter.flags &= ~IFA_F_PERMANENT; filter.flagmask |= IFA_F_PERMANENT; } else if (strcmp(arg, "primary") == 0) { - filter.flags &= ~IFA_F_SECONDARY; + if (inv) + filter.flags |= IFA_F_SECONDARY; + else + 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; - - filter.flags &= ifa_flag_names[i].value; - filter.flagmask |= ifa_flag_names[i].value; - return 0; - } - - return -1; } else { for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) { if (strcmp(arg, ifa_flag_names[i].name)) continue; - filter.flags |= ifa_flag_names[i].value; + + if (inv) + filter.flags &= ~ifa_flag_names[i].value; + else + filter.flags |= ifa_flag_names[i].value; filter.flagmask |= ifa_flag_names[i].value; return 0; }
When disabling a flag, one needs to AND with the inverse not the flag itself. Otherwise specifying for instance 'home -nodad' will effectively clear the flags variable. While being at it, simplify the code a bit by merging common parts of negated and non-negated case branches. Also allow for the "special cases" to be inverted, too. Fixes: f73ac674d0abf ("ip: change flag names to an array") Signed-off-by: Phil Sutter <phil@nwl.cc> --- ip/ipaddress.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)