PERFORCE change 166206 for review

Tatsiana Elavaya tsel at FreeBSD.org
Fri Jul 17 18:36:07 UTC 2009


http://perforce.freebsd.org/chv.cgi?CH=166206

Change 166206 by tsel at tsel_mz on 2009/07/17 18:35:33

	Style fixes
	Separate ipfw_optimize into 2 functions

Affected files ...

.. //depot/projects/soc2009/tsel_ipfw/sbin/ipfw/ipfw2.c#11 edit
.. //depot/projects/soc2009/tsel_ipfw/sys/netinet/ip_fw2.c#7 edit

Differences ...

==== //depot/projects/soc2009/tsel_ipfw/sbin/ipfw/ipfw2.c#11 (text+ko) ====

@@ -452,13 +452,15 @@
 	return 0;
 }
 
-static void*
+/*
+ * Get rules or pipes from kernel, resize data buffer as necessary.
+ */
+static void *
 ipfw_get_all(int ocmd, int *nbytes)
 {
 	void *data = NULL;
 	int nalloc = 1024;	/* start somewhere... */
 
-	/* get rules or pipes from kernel, resizing array as necessary */
 	*nbytes = nalloc;
 
 	while (*nbytes >= nalloc) {
@@ -472,22 +474,24 @@
 	return data;
 }
 
-static struct ip_fw**
-get_rules_cached(int *len)
+/*
+ * Get cached copy of rules
+ */
+static struct ip_fw **
+ipfw_get_rules_cached(int *len)
 {
 	static struct ip_fw *raw_rules = NULL;
 	static struct ip_fw **rules = NULL;
 	static int rules_len = 0;
+	char *r, *end;
 	int i, sz;
 
 	if (raw_rules == NULL || rules == NULL) {
-		char *r, *end;
-
-		raw_rules = (struct ip_fw*) ipfw_get_all(IP_FW_GET, &sz);
-		rules = safe_calloc(sz / sizeof(struct ip_fw) + 1, sizeof(void*));
-		r = (char*)raw_rules;
+		raw_rules = (struct ip_fw *)ipfw_get_all(IP_FW_GET, &sz);
+		rules = safe_calloc(sz / sizeof(struct ip_fw) + 1, sizeof(void *));
+		r = (char *)raw_rules;
 		end = r + sz;
-		for (i = 0; ; i++) {
+		for (i = 0;; i++) {
 			if (r + RULESIZE(r) > end) {
 				rules[i] = NULL;
 				break;
@@ -503,8 +507,11 @@
 	return rules;
 }
 
-static char*
-get_rule_alias(struct ip_fw *rule)
+/*
+ * Search rule instructions for alias
+ */
+static char *
+ipfw_rule_alias(struct ip_fw *rule)
 {
 	ipfw_insn *cmd;
 	int l;
@@ -512,11 +519,14 @@
 	for (l = rule->cmd_len - rule->act_ofs, cmd = ACTION_PTR(rule);
 			l > 0 ; l -= F_LEN(cmd), cmd += F_LEN(cmd)) {
 		if (cmd->opcode == O_ALIAS)
-			return ((ipfw_insn_alias*) cmd)->alias;
+			return ((ipfw_insn_alias *)cmd)->alias;
 	}
 	return NULL;
 }
 
+/*
+ * Find rule number for given alias 
+ */
 static int
 alias_lookup_rulenum(const char *alias)
 {
@@ -524,25 +534,28 @@
 	char *rule_alias;
 	int i;
 
-	rules = get_rules_cached(NULL);
+	rules = ipfw_get_rules_cached(NULL);
 	for (i = 0; rules[i]; i++) {
-		rule_alias = get_rule_alias(rules[i]);
+		rule_alias = ipfw_rule_alias(rules[i]);
 		if (rule_alias && !strcmp(rule_alias, alias))
 			return rules[i]->rulenum;
 	}
 	return -1;
 }
 
-static char* 
+/*
+ * Find alias for given rule number 
+ */
+static char *
 alias_lookup(int rulenum)
 {
 	struct ip_fw **rules;
 	int i;
 
-	rules = get_rules_cached(NULL);
+	rules = ipfw_get_rules_cached(NULL);
 	for (i = 0; rules[i]; i++) {
 		if (rules[i]->rulenum == rulenum)
-			return get_rule_alias(rules[i]);
+			return ipfw_rule_alias(rules[i]);
 	}
 	return NULL;
 }
@@ -1072,7 +1085,7 @@
 		}
 	}
 
-	alias = get_rule_alias(rule);
+	alias = ipfw_rule_alias(rule);
 	if (alias)
 		printf("alias %s ", alias);
 
@@ -1132,14 +1145,13 @@
 				print_unreach6_code(cmd->arg1);
 			break;
 
-		case O_SKIPTO: {
-			char *alias = alias_lookup(cmd->arg1);
+		case O_SKIPTO:
+			alias = alias_lookup(cmd->arg1);
 			if (alias == NULL)
 				PRINT_UINT_ARG("skipto ", cmd->arg1);
 			else
 				printf("skipto %s", alias);
 			break;
-	       }
 
 		case O_PIPE:
 			PRINT_UINT_ARG("pipe ", cmd->arg1);
@@ -1882,7 +1894,7 @@
 		char *alias;
 
 		for (r = data; r->rulenum < IPFW_DEFAULT_RULE; r = NEXT(r)) {
-			alias = get_rule_alias(r);
+			alias = ipfw_rule_alias(r);
 			if (alias)
 				printf("%-5d %s\n", r->rulenum, alias);
 		}
@@ -2073,6 +2085,10 @@
 
 static LIST_MERGESORT_GENERATE(insn_match_rule_cmd_sort, insn_match_rule_head, insn_match, rule_entries);
 
+/*
+ * Compare instructions. Ignore instructions with F_OR flag, actions and
+ * instructions that depend on processing state (like O_UID, O_TAGGED).
+ */
 int
 insn_eq(ipfw_insn *a, ipfw_insn *b)
 {
@@ -2146,6 +2162,10 @@
 	return 0;
 }
 
+/*
+ * Create a match for instruction and add it to match group list creating a
+ * match group if necessary.
+ */
 int
 insn_match_insert(struct insn_match_group_head *group_head, ipfw_insn *cmd, 
 		struct insn_match_rule *rule)
@@ -2184,20 +2204,21 @@
 	free(m);
 }
 
+/*
+ * Compare groups by rank, update rank if necessary.
+ */
 static int
 insn_match_group_cmp(struct insn_match_group *_a, struct insn_match_group *_b)
 {
 	struct insn_match_group *a[2] = { _a, _b };
-	int i;
+	struct insn_match *m;
+	int i, min_r, max_r;
 
 	if (a[0] == NULL)
 		return a[1] == NULL ? 0 : 1;
 	if (a[1] == NULL)
 		return -1;
 	for (i = 0; i < 2; i++) {
-		struct insn_match *m;
-		int min_r, max_r;
-
 		if (a[i]->rank || a[i]->match_count == 0)
 			continue;
 		min_r = max_r = LIST_FIRST(&a[i]->match_head)->match_rule->rule->rulenum;
@@ -2220,6 +2241,10 @@
 	return b->group->rank - a->group->rank;
 }
 
+/*
+ * Remove groups with single match from the list.
+ * Trim list if it exceeds max size
+ */
 static int
 optimization_filter_groups(struct insn_match_group_head *head)
 {
@@ -2227,11 +2252,11 @@
 	int labels_max, group_count;
 
 	group_count = sizeof(labels_max);
-	if (sysctlbyname("net.inet.ip.fw.optimization_buf_max",
-				&labels_max, &group_count, NULL, 0) == -1) {
+	if (sysctlbyname("net.inet.ip.fw.optimization_buf_max", &labels_max,
+	    &group_count, NULL, 0) == -1) {
 		errx(EX_DATAERR, "optimization not supported");
 	}
-	labels_max *= 8 / 2; /* one label is 2 bits long */
+	labels_max *= 8 / 2; /* 2 bits long per label. */
 
 	group_count = 0;
 	LIST_FOREACH_SAFE(g, head, group_entries, g_tmp) {
@@ -2247,33 +2272,120 @@
 	return group_count;
 }
 
+/*
+ * Enable/disable kernel level optimization support to prevent races while
+ * updating rules. Also set number of labels needed.
+ */
 static void
 optimization_setup(int enable, int labels)
 {
+	int r;
+
 	if (enable) {
 		labels = (labels + 3)/4;
-		if (sysctlbyname("net.inet.ip.fw.optimization_buf",
-					NULL, 0, &labels, sizeof(labels)) == -1) {
+		r = sysctlbyname("net.inet.ip.fw.optimization_buf", NULL, 0,
+		    &labels, sizeof(labels));
+		if (r == -1) {
 			errx(EX_DATAERR, "optimization not supported");
 		}
 	}
 
-	if (sysctlbyname("net.inet.ip.fw.optimization_enable",
-				NULL, 0, &enable, sizeof(enable)) == -1) {
+	r = sysctlbyname("net.inet.ip.fw.optimization_enable", NULL, 0, &enable,
+	    sizeof(enable));
+	if (r == -1) {
 		errx(EX_DATAERR, "optimization not supported");
 	}
 
 }
 
+/*
+ * Remove original rule and add rule with optimization instructions
+ */
+static void
+optimization_update_rule(struct insn_match_rule *match_rule, struct ip_fw *rule, struct ip_fw *rule_buf)
+{
+	struct insn_match *m;
+	ipfw_insn *cmd, *rcmd;
+	ipfw_insn_u16 *optimize_cmd;
+	int l, optimize_cnt, skip;
+
+	memcpy(rule_buf, rule, sizeof(struct ip_fw) - 4);
+	cmd = rule_buf->cmd;
+	rcmd = rule->cmd;
+	l = rule->cmd_len;
+
+	while ((rcmd->opcode == O_PROB || rcmd->opcode == O_PROBE_STATE) && l > 0) {
+		memcpy(cmd, rcmd, F_LEN(rcmd) * 4);
+		cmd += F_LEN(rcmd);
+		l -= F_LEN(rcmd);
+	}
+
+	optimize_cmd = (ipfw_insn_u16 *) cmd;
+	optimize_cmd->o.opcode = O_OPTIMIZE;
+	optimize_cmd->o.arg1 = 0;
+
+	insn_match_rule_cmd_sort(&match_rule->rule_head, insn_match_rule_cmd_cmp);
+	optimize_cnt = 0;
+	LIST_FOREACH(m, &match_rule->rule_head, rule_entries) {
+		optimize_cmd->ports[optimize_cnt] = m->group->label;
+		if (optimize_cnt % 2 == 0)
+			optimize_cmd->ports[optimize_cnt + 1] = 0;
+		optimize_cnt++;
+	}
+
+	optimize_cmd->o.len = F_INSN_SIZE(ipfw_insn) + (optimize_cnt + 1) / 2;
+	cmd += optimize_cmd->o.len;
+	rule_buf->cmd_len += optimize_cmd->o.len;
+	rule_buf->act_ofs += optimize_cmd->o.len;
+
+	LIST_FOREACH(m, &match_rule->rule_head, rule_entries) {
+		memcpy(cmd, m->cmd, F_LEN(m->cmd) * 4);
+		cmd += F_LEN(m->cmd);
+		l -= F_LEN(m->cmd);
+	}
+
+	while (l > 0) {
+		skip = 0;
+		LIST_FOREACH(m, &match_rule->rule_head, rule_entries) {
+			if (rcmd == m->cmd) {
+				skip = 1;
+				break;
+			}
+		}
+		if (!skip) {
+			memcpy(cmd, rcmd, F_LEN(rcmd) * 4);
+			cmd += F_LEN(rcmd);
+			l -= F_LEN(rcmd);
+		}
+		rcmd += F_LEN(rcmd);
+	}
+
+	l = rule_buf->rulenum & 0xffff;
+	if (do_cmd(IP_FW_DEL, &l, sizeof(l)))
+		errx(EX_DATAERR, "rule %u: setsockopt(IP_FW_DEL)", rule_buf->rulenum);
+
+	l = RULESIZE(rule_buf);
+	if (do_cmd(IP_FW_ADD, rule_buf, (uintptr_t)&l))
+		errx(EX_DATAERR, "rule %u: setsockopt(IP_FW_ADD)", rule_buf->rulenum);
+
+	if (co.verbose)
+		show_ipfw(rule_buf, 0, 0);
+}
+
+/*
+ * Optimize current ruleset.
+ */
 void
 ipfw_optimize(int argc, char **argv)
 {
 	struct insn_match_group_head groups_opcode[O_LAST_OPCODE];
 	struct insn_match_group_head groups;
+	struct insn_match_group *g;
 	struct insn_match_rule *match_rules;
 	struct ip_fw **rules;
-	struct ip_fw *orule;
-	int i, group_count, rules_count;
+	struct ip_fw *rule_buf;
+	ipfw_insn *cmd;
+	int i, l, group_count, rules_count;
 
 	if (co.test_only) {
 		fprintf(stderr, "Testing only, optimization disabled\n");
@@ -2285,14 +2397,10 @@
 		LIST_INIT(&groups_opcode[i]);
 	}
 
-	rules = get_rules_cached(&rules_count);
+	rules = ipfw_get_rules_cached(&rules_count);
 	match_rules = safe_calloc(rules_count, sizeof(struct insn_match_rule));
 
 	for (i = 0; i < rules_count; i++) {
-		ipfw_insn *cmd;
-		int l;
-
-
 		if (i > 0 && rules[i]->rulenum == rules[i - 1]->rulenum)
 			errx(EX_DATAERR, "rule number is not unique: %d", rules[i]->rulenum);
 
@@ -2302,7 +2410,7 @@
 
 		for (l = rules[i]->cmd_len, cmd = rules[i]->cmd; l > 0;) {
 			l -= F_LEN(cmd);
-			/* remove old optimization labels */
+			/* Remove old optimization labels. */
 			if (cmd->opcode == O_OPTIMIZE) {
 				rules[i]->cmd_len -= F_LEN(cmd);
 				rules[i]->act_ofs -= F_LEN(cmd);
@@ -2314,10 +2422,10 @@
 		}
 	}
 
+	/* Move all groups into single list. */
 	for (i = 0; i < O_LAST_OPCODE; i++) {
 		while(!LIST_EMPTY(&groups_opcode[i])) {
-			struct insn_match_group *g = LIST_FIRST(&groups_opcode[i]);
-
+			g = LIST_FIRST(&groups_opcode[i]);
 			LIST_REMOVE(g, group_entries);
 			LIST_INSERT_HEAD(&groups, g, group_entries);
 		}
@@ -2332,78 +2440,11 @@
 	
 	optimization_setup(0, 0);
 
-	orule = (struct ip_fw*)safe_calloc(1, sizeof(struct ip_fw) + 255*4);
+	rule_buf = (struct ip_fw*)safe_calloc(1, sizeof(struct ip_fw) + 255*4);
 	for (i = 0; rules[i]; i++) {
-		ipfw_insn *cmd, *rcmd;
-		struct insn_match *m;
-		ipfw_insn_u16 *optimize_cmd;
-		int optimize_cnt = 0;
-		int l;
-
 		if (LIST_EMPTY(&match_rules[i].rule_head))
 			continue;
-
-		memcpy(orule, rules[i], sizeof(struct ip_fw) - 4);
-		cmd = orule->cmd;
-		rcmd = rules[i]->cmd;
-		l = rules[i]->cmd_len;
-
-		while ((rcmd->opcode == O_PROB || rcmd->opcode == O_PROBE_STATE) && l > 0) {
-			memcpy(cmd, rcmd, F_LEN(rcmd) * 4);
-			cmd += F_LEN(rcmd);
-			l -= F_LEN(rcmd);
-		}
-
-		optimize_cmd = (ipfw_insn_u16 *) cmd;
-		optimize_cmd->o.opcode = O_OPTIMIZE;
-		optimize_cmd->o.arg1 = 0;
-
-		insn_match_rule_cmd_sort(&match_rules[i].rule_head, insn_match_rule_cmd_cmp);
-		LIST_FOREACH(m, &match_rules[i].rule_head, rule_entries) {
-			optimize_cmd->ports[optimize_cnt] = m->group->label;
-			if (optimize_cnt % 2 == 0)
-				optimize_cmd->ports[optimize_cnt + 1] = 0;
-			optimize_cnt++;
-		}
-
-		optimize_cmd->o.len = F_INSN_SIZE(ipfw_insn) + (optimize_cnt + 1) / 2;
-		cmd += optimize_cmd->o.len;
-		orule->cmd_len += optimize_cmd->o.len;
-		orule->act_ofs += optimize_cmd->o.len;
-
-		LIST_FOREACH(m, &match_rules[i].rule_head, rule_entries) {
-			memcpy(cmd, m->cmd, F_LEN(m->cmd) * 4);
-			cmd += F_LEN(m->cmd);
-			l -= F_LEN(m->cmd);
-		}
-
-		while (l > 0) {
-			int skip = 0;
-
-			LIST_FOREACH(m, &match_rules[i].rule_head, rule_entries) {
-				if (rcmd == m->cmd) {
-					skip = 1;
-					break;
-				}
-			}
-			if (!skip) {
-				memcpy(cmd, rcmd, F_LEN(rcmd) * 4);
-				cmd += F_LEN(rcmd);
-				l -= F_LEN(rcmd);
-			}
-			rcmd += F_LEN(rcmd);
-		}
-
-		l = orule->rulenum & 0xffff;
-		if (do_cmd(IP_FW_DEL, &l, sizeof(l)))
-			errx(EX_DATAERR, "rule %u: setsockopt(IP_FW_DEL)", orule->rulenum);
-
-		l = RULESIZE(orule);
-		if (do_cmd(IP_FW_ADD, orule, (uintptr_t)&l))
-			errx(EX_DATAERR, "rule %u: setsockopt(IP_FW_ADD)", orule->rulenum);
-
-		if (co.verbose)
-			show_ipfw(orule, 0, 0);
+		optimization_update_rule(&match_rules[i], rules[i], rule_buf);
 	}
 
 	optimization_setup(1, group_count);
@@ -3079,7 +3120,8 @@
 	 */
 	ipfw_insn_alias alias_cmd;
 	ipfw_insn *have_state = NULL;	/* check-state or keep-state */
-	ipfw_insn *have_log = NULL, *have_altq = NULL, *have_tag = NULL, *have_alias = NULL;
+	ipfw_insn *have_log = NULL, *have_altq = NULL, *have_tag = NULL;
+	ipfw_insn *have_alias = NULL;
 	size_t len;
 
 	int i;
@@ -3110,15 +3152,16 @@
 
 	/* [alias ALIAS] */
 	if (ac > 1 && _substrcmp(*av, "alias") == 0) {
-		int alias_rule;
-
 		NEED1("missing alias name");
-		for (i = 0; isdigit(av[1][i]) || av[1][i] == '+' || av[1][i] == '-'; i++) { ; }
-		if (av[1][i] == '\0' || strpbrk(av[1], " \t\n\r") != NULL || strcmp(av[1], "alias") == 0)
+		for (i = 0; isdigit(av[1][i]) || av[1][i] == '+' ||
+		    av[1][i] == '-'; i++)
+			;	/* nothing */
+		if (av[1][i] == '\0' || strpbrk(av[1], " \t\n\r") != NULL ||
+		    strcmp(av[1], "alias") == 0)
 			errx(EX_DATAERR, "invalid alias '%s'", av[1]);
-		alias_rule = alias_lookup_rulenum(av[1]);
-		if (alias_rule > 0)
-			errx(EX_DATAERR, "rule %d already has alias %s", alias_rule, av[1]);
+		if (alias_lookup_rulenum(av[1]) > 0)
+			errx(EX_DATAERR, "rule %d already has alias %s",
+			    alias_lookup_rulenum(av[1]), av[1]);
 		alias_cmd.o.opcode = O_ALIAS;
 		alias_cmd.o.len = F_INSN_SIZE(ipfw_insn_alias);
 		strlcpy(alias_cmd.alias, av[1], IPFW_ALIAS_NAME_SIZE);

==== //depot/projects/soc2009/tsel_ipfw/sys/netinet/ip_fw2.c#7 (text+ko) ====

@@ -182,7 +182,8 @@
 
 #define OPTIMIZATION_POOLS		4
 #define OPTIMIZATION_BUF_MAX		PAGE_SIZE
-#define OPTIMIZATION_BUF_INITIAL	32
+#define OPTIMIZATION_BUF_ROUND		(sizeof(uint32_t) * 8)
+#define OPTIMIZATION_BUF_INITIAL	OPTIMIZATION_BUF_ROUND
 
 #ifdef SYSCTL_NODE
 SYSCTL_NODE(_net_inet_ip, OID_AUTO, fw, CTLFLAG_RW, 0, "Firewall");
@@ -357,8 +358,8 @@
 	if (sz <= 0 || sz > OPTIMIZATION_BUF_MAX)
 		return EINVAL;
 
-	if (sz % 32)
-		sz += 32 - (sz % 32);
+	if (sz % OPTIMIZATION_BUF_ROUND)
+		sz += OPTIMIZATION_BUF_ROUND - (sz % OPTIMIZATION_BUF_ROUND);
 	if (sz > OPTIMIZATION_BUF_MAX)
 		sz = OPTIMIZATION_BUF_MAX;
 


More information about the p4-projects mailing list