diff mbox

[iproute2] ip: change flag names to an array

Message ID 20170707153941.5427-1-stephen@networkplumber.org
State Superseded, archived
Delegated to: stephen hemminger
Headers show

Commit Message

Stephen Hemminger July 7, 2017, 3:39 p.m. UTC 
For the most of the address flags, use a table of bit values rather
than open coding every value.  This allows for easier inevitable
expansion of flags.

This also fixes the missing stable-privacy flag.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 ip/ipaddress.c | 152 ++++++++++++++++++++++++---------------------------------
 1 file changed, 65 insertions(+), 87 deletions(-)

Comments

Lorenzo Colitti July 8, 2017, 6:08 a.m. UTC | #1 
On Sat, Jul 8, 2017 at 12:39 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> For the most of the address flags, use a table of bit values rather
> than open coding every value.  This allows for easier inevitable
> expansion of flags.

Thanks for doing this.

> +static unsigned int get_ifa_flag_mask(const char *name)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
> +               if (!strcmp(name, ifa_flag_names[i]))
> +                       return 1u << i;
> +       }
> +       return 0;
> +}

It looks like the user can specify things such as "-deprecated".
Perhaps that sort of thing can be handled by this function too?
Instead of returning an unsigned int, this function could be void and
take a flag value and a mask as output parameters. If the flag name
starts with "-" it would set the value to 0, and if it doesn't it
would set the value to 1.

Otherwise, some of the -<flag> parameters will either need to be
special cased or cease to be supported. Specifically, I see this one:

> -               } else if (strcmp(*argv, "-tentative") == 0) {
> -                       filter.flags &= ~IFA_F_TENTATIVE;
> -                       filter.flagmask |= IFA_F_TENTATIVE;

this one:

> -               } else if (strcmp(*argv, "-deprecated") == 0) {
> -                       filter.flags &= ~IFA_F_DEPRECATED;
> -                       filter.flagmask |= IFA_F_DEPRECATED;

and this one:

> -               } else if (strcmp(*argv, "-dadfailed") == 0) {
> -                       filter.flags &= ~IFA_F_DADFAILED;
> -                       filter.flagmask |= IFA_F_DADFAILED;
David Laight July 10, 2017, 10:10 a.m. UTC | #2 
From:  Stephen Hemminger
> Sent: 07 July 2017 16:40
> For the most of the address flags, use a table of bit values rather
> than open coding every value.  This allows for easier inevitable
> expansion of flags.
> 
> This also fixes the missing stable-privacy flag.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  ip/ipaddress.c | 152 ++++++++++++++++++++++++---------------------------------
>  1 file changed, 65 insertions(+), 87 deletions(-)
> 
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index f06f5829fb61..b4efb9fedcd2 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -1013,14 +1013,67 @@ static unsigned int get_ifa_flags(struct ifaddrmsg *ifa,
>  				ifa->ifa_flags;
>  }
> 
> +
> +static const char *ifa_flag_names[] = {
> +	"secondary",
> +	"nodad",
> +	"optimistic",
> +	"dadfailed",
> +	"home",
> +	"deprecated",
> +	"tentative",
> +	"permanent",
> +	"mngtmpaddr",
> +	"noprefixroute",
> +	"autojoin",
> +	"stable-privacy",
> +};

It would be safer to set up a table of the constant - string pairs
instead of relying on the table being in the right order.

	David
diff mbox

Patch

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index f06f5829fb61..b4efb9fedcd2 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1013,14 +1013,67 @@  static unsigned int get_ifa_flags(struct ifaddrmsg *ifa,
 				ifa->ifa_flags;
 }
 
+
+static const char *ifa_flag_names[] = {
+	"secondary",
+	"nodad",
+	"optimistic",
+	"dadfailed",
+	"home",
+	"deprecated",
+	"tentative",
+	"permanent",
+	"mngtmpaddr",
+	"noprefixroute",
+	"autojoin",
+	"stable-privacy",
+};
+
+static void print_ifa_flags(FILE *fp, const struct ifaddrmsg *ifa, unsigned int flags)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
+		unsigned int mask = 1u << i;
+
+		if (mask == IFA_F_PERMANENT) {
+			if (!(flags & mask))
+				fprintf(fp, "dynamic ");
+		} else if (flags & mask) {
+			if (mask == IFA_F_SECONDARY) {
+				if (ifa->ifa_family == AF_INET6)
+					fprintf(fp, "temporary ");
+				else
+					fprintf(fp, "secondary ");
+			} else
+				fprintf(fp, "%s ", ifa_flag_names[i]);
+		}
+
+		flags &= ~mask;
+	}
+
+	if (flags)
+		fprintf(fp, "flags %02x ", flags);
+
+}
+
+static unsigned int get_ifa_flag_mask(const char *name)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ifa_flag_names); i++) {
+		if (!strcmp(name, ifa_flag_names[i]))
+			return 1u << i;
+	}
+	return 0;
+}
+
 int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		   void *arg)
 {
 	FILE *fp = arg;
 	struct ifaddrmsg *ifa = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
-	int deprecated = 0;
-	/* Use local copy of ifa_flags to not interfere with filtering code */
 	unsigned int ifa_flags;
 	struct rtattr *rta_tb[IFA_MAX+1];
 
@@ -1040,6 +1093,7 @@  int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 	parse_rtattr(rta_tb, IFA_MAX, IFA_RTA(ifa),
 		     n->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa)));
 
+
 	ifa_flags = get_ifa_flags(ifa, rta_tb[IFA_FLAGS]);
 
 	if (!rta_tb[IFA_LOCAL])
@@ -1145,52 +1199,9 @@  int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 					      rta_tb[IFA_ANYCAST]));
 	}
 	fprintf(fp, "scope %s ", rtnl_rtscope_n2a(ifa->ifa_scope, b1, sizeof(b1)));
-	if (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_flags & IFA_F_TENTATIVE) {
-		ifa_flags &= ~IFA_F_TENTATIVE;
-		fprintf(fp, "tentative ");
-	}
-	if (ifa_flags & IFA_F_DEPRECATED) {
-		ifa_flags &= ~IFA_F_DEPRECATED;
-		deprecated = 1;
-		fprintf(fp, "deprecated ");
-	}
-	if (ifa_flags & IFA_F_HOMEADDRESS) {
-		ifa_flags &= ~IFA_F_HOMEADDRESS;
-		fprintf(fp, "home ");
-	}
-	if (ifa_flags & IFA_F_NODAD) {
-		ifa_flags &= ~IFA_F_NODAD;
-		fprintf(fp, "nodad ");
-	}
-	if (ifa_flags & IFA_F_MANAGETEMPADDR) {
-		ifa_flags &= ~IFA_F_MANAGETEMPADDR;
-		fprintf(fp, "mngtmpaddr ");
-	}
-	if (ifa_flags & IFA_F_NOPREFIXROUTE) {
-		ifa_flags &= ~IFA_F_NOPREFIXROUTE;
-		fprintf(fp, "noprefixroute ");
-	}
-	if (ifa_flags & IFA_F_MCAUTOJOIN) {
-		ifa_flags &= ~IFA_F_MCAUTOJOIN;
-		fprintf(fp, "autojoin ");
-	}
-	if (!(ifa_flags & IFA_F_PERMANENT))
-		fprintf(fp, "dynamic ");
-	else
-		ifa_flags &= ~IFA_F_PERMANENT;
-	if (ifa_flags & IFA_F_DADFAILED) {
-		ifa_flags &= ~IFA_F_DADFAILED;
-		fprintf(fp, "dadfailed ");
-	}
-	if (ifa_flags)
-		fprintf(fp, "flags %02x ", ifa_flags);
+
+	print_ifa_flags(fp, ifa, ifa_flags);
+
 	if (rta_tb[IFA_LABEL])
 		fprintf(fp, "%s", rta_getattr_str(rta_tb[IFA_LABEL]));
 	if (rta_tb[IFA_CACHEINFO]) {
@@ -1206,7 +1217,7 @@  int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		if (ci->ifa_prefered == INFINITY_LIFE_TIME)
 			fprintf(fp, "forever");
 		else {
-			if (deprecated)
+			if (ifa_flags & IFA_F_DEPRECATED)
 				fprintf(fp, "%dsec", ci->ifa_prefered);
 			else
 				fprintf(fp, "%usec", ci->ifa_prefered);
@@ -1570,6 +1581,7 @@  static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 	struct nlmsg_chain _ainfo = { NULL, NULL}, *ainfo = NULL;
 	struct nlmsg_list *l;
 	char *filter_dev = NULL;
+	unsigned int mask;
 	int no_link = 0;
 
 	ipaddr_reset_filter(oneline, 0);
@@ -1612,49 +1624,15 @@  static int ipaddr_list_flush_or_save(int argc, char **argv, int action)
 		} else if (strcmp(*argv, "dynamic") == 0) {
 			filter.flags &= ~IFA_F_PERMANENT;
 			filter.flagmask |= IFA_F_PERMANENT;
-		} else if (strcmp(*argv, "permanent") == 0) {
-			filter.flags |= IFA_F_PERMANENT;
-			filter.flagmask |= IFA_F_PERMANENT;
-		} else if (strcmp(*argv, "secondary") == 0 ||
-			   strcmp(*argv, "temporary") == 0) {
+		} else if (strcmp(*argv, "temporary") == 0) {
 			filter.flags |= IFA_F_SECONDARY;
 			filter.flagmask |= IFA_F_SECONDARY;
 		} else if (strcmp(*argv, "primary") == 0) {
 			filter.flags &= ~IFA_F_SECONDARY;
 			filter.flagmask |= IFA_F_SECONDARY;
-		} else if (strcmp(*argv, "tentative") == 0) {
-			filter.flags |= IFA_F_TENTATIVE;
-			filter.flagmask |= IFA_F_TENTATIVE;
-		} else if (strcmp(*argv, "-tentative") == 0) {
-			filter.flags &= ~IFA_F_TENTATIVE;
-			filter.flagmask |= IFA_F_TENTATIVE;
-		} else if (strcmp(*argv, "deprecated") == 0) {
-			filter.flags |= IFA_F_DEPRECATED;
-			filter.flagmask |= IFA_F_DEPRECATED;
-		} else if (strcmp(*argv, "-deprecated") == 0) {
-			filter.flags &= ~IFA_F_DEPRECATED;
-			filter.flagmask |= IFA_F_DEPRECATED;
-		} else if (strcmp(*argv, "home") == 0) {
-			filter.flags |= IFA_F_HOMEADDRESS;
-			filter.flagmask |= IFA_F_HOMEADDRESS;
-		} else if (strcmp(*argv, "nodad") == 0) {
-			filter.flags |= IFA_F_NODAD;
-			filter.flagmask |= IFA_F_NODAD;
-		} else if (strcmp(*argv, "mngtmpaddr") == 0) {
-			filter.flags |= IFA_F_MANAGETEMPADDR;
-			filter.flagmask |= IFA_F_MANAGETEMPADDR;
-		} else if (strcmp(*argv, "noprefixroute") == 0) {
-			filter.flags |= IFA_F_NOPREFIXROUTE;
-			filter.flagmask |= IFA_F_NOPREFIXROUTE;
-		} else if (strcmp(*argv, "autojoin") == 0) {
-			filter.flags |= IFA_F_MCAUTOJOIN;
-			filter.flagmask |= IFA_F_MCAUTOJOIN;
-		} else if (strcmp(*argv, "dadfailed") == 0) {
-			filter.flags |= IFA_F_DADFAILED;
-			filter.flagmask |= IFA_F_DADFAILED;
-		} else if (strcmp(*argv, "-dadfailed") == 0) {
-			filter.flags &= ~IFA_F_DADFAILED;
-			filter.flagmask |= IFA_F_DADFAILED;
+		} else if ((mask = get_ifa_flag_mask(*argv)) != 0) {
+			filter.flags |= mask;
+			filter.flagmask |= mask;
 		} else if (strcmp(*argv, "label") == 0) {
 			NEXT_ARG();
 			filter.label = *argv;