Better parsing and -v support for tables:

  - remove the tableaddrs and tableaddr yacc production and reuse
    host_list instead.
  - produce better error messages.
  - do not load addresses from external file when it is not
    required (like with -R option).
  - store initializers in a new node_tinit linked list before
    putting them into the address buffer (see next point).
  - add a new print_tabledef() function, which makes "pfctl -nvf"
    print something useful for table definitions, which in turn
    makes it possible to write better regress tests (see first chunk
    of the diff) and bring table definition consistant with other
    parsed rules.

ok dhartmei@

5 files changed, 178 insertions, 100 deletions

diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index 4d4a4fc7918..8b932319e39 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.398 2003/07/10 05:25:27 cedric Exp $	*/
+/*	$OpenBSD: parse.y,v 1.399 2003/07/11 08:29:34 cedric Exp $	*/
 
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
@@ -208,6 +208,7 @@ struct queue_opts {
 struct table_opts {
 	int			flags;
 	int			init_addr;
+	struct node_tinithead	init_nodes;
 } table_opts;
 
 struct node_hfsc_opts	hfsc_opts;
@@ -218,6 +219,7 @@ int	rule_consistent(struct pf_rule *);
 int	filter_consistent(struct pf_rule *);
 int	nat_consistent(struct pf_rule *);
 int	rdr_consistent(struct pf_rule *);
+int	process_tabledef(char *, struct table_opts *);
 int	yyparse(void);
 void	expand_label_str(char *, const char *, const char *);
 void	expand_label_if(const char *, char *, const char *);
@@ -855,26 +857,22 @@ tabledef	: TABLE '<' STRING '>' table_opts {
 				    PF_TABLE_NAME_SIZE - 1);
 				YYERROR;
 			}
-			if (pfctl_define_table($3, $5.flags, $5.init_addr,
-			    (pf->opts & PF_OPT_NOACTION) || !(pf->loadopt &
-				(PFCTL_FLAG_TABLE | PFCTL_FLAG_ALL)),
-			    pf->anchor, pf->ruleset, pf->ab, pf->tticket)) {
-				yyerror("cannot define table %s: %s", $3,
-				    pfr_strerror(errno));
-				YYERROR;
-			}
-			pf->tdirty = 1;
+			if (pf->loadopt & (PFCTL_FLAG_TABLE | PFCTL_FLAG_ALL))
+				if (process_tabledef($3, &$5))
+					YYERROR;
 		}
 		;
 
 table_opts	:	{
 			bzero(&table_opts, sizeof table_opts);
+			SIMPLEQ_INIT(&table_opts.init_nodes);
 		}
 		   table_opts_l
 			{ $$ = table_opts; }
 		| /* empty */
 			{
 			bzero(&table_opts, sizeof table_opts);
+			SIMPLEQ_INIT(&table_opts.init_nodes);
 			$$ = table_opts;
 		}
 		;
@@ -883,8 +881,7 @@ table_opts_l	: table_opts_l table_opt
 		| table_opt
 		;
 
-table_opt	: STRING
-			{
+table_opt	: STRING		{
 			if (!strcmp($1, "const"))
 				table_opts.flags |= PFR_TFLAG_CONST;
 			else if (!strcmp($1, "persist"))
@@ -892,45 +889,48 @@ table_opt	: STRING
 			else
 				YYERROR;
 		}
-		| '{' tableaddrs '}'	{ table_opts.init_addr = 1; }
-		| FILENAME STRING	{
-			if (pfr_buf_load(pf->ab, $2, 0, append_addr)) {
-				if (errno)
-					yyerror("cannot load %s: %s", $2,
-					    pfr_strerror(errno));
-					YYERROR;
-			}
-			table_opts.init_addr = 1;
-		}
-		;
-
-tableaddrs	: /* empty */
-		| tableaddrs tableaddr comma
-
-tableaddr	: not STRING {
-			if (append_addr_not(pf->ab, $2, 0, $1)) {
-				if (errno)
-					yyerror("cannot add %s: %s", $2,
-					    pfr_strerror(errno));
+		| '{' '}'		{ table_opts.init_addr = 1; }
+		| '{' host_list '}'	{
+			struct node_host	*n;
+			struct node_tinit	*ti;
+
+			for (n = $2; n != NULL; n = n->next) {
+				switch(n->addr.type) {
+				case PF_ADDR_ADDRMASK:
+					continue; /* ok */
+				case PF_ADDR_DYNIFTL:
+					yyerror("dynamic addresses are not "
+					    "permitted inside tables");
+					break;
+				case PF_ADDR_TABLE:
+					yyerror("tables cannot contain tables");
+					break;
+				case PF_ADDR_NOROUTE:
+					yyerror("\"no-route\" is not permitted "
+					    "inside tables");
+					break;
+				default:
+					yyerror("unknown address type %d",
+					    n->addr.type);
+				}
 				YYERROR;
 			}
+			if (!(ti = calloc(1, sizeof(*ti))))
+				err(1, "table_opt: calloc");
+			ti->host = $2;
+			SIMPLEQ_INSERT_TAIL(&table_opts.init_nodes, ti,
+			    entries);
+			table_opts.init_addr = 1;
 		}
-		| not STRING '/' number {
-			char *buf = NULL;
+		| FILENAME STRING	{
+			struct node_tinit	*ti;
 
-			if (asprintf(&buf, "%s/%d", $2, $4) < 0) {
-				if (errno)
-					yyerror("cannot add %s/%d: %s", $2, $4,
-					    strerror(errno));
-                                YYERROR;
-			} else if (append_addr_not(pf->ab, buf, 0, $1)) {
-				if (errno)
-					yyerror("cannot add %s: %s", buf,
-					    pfr_strerror(errno));
-				free(buf);
-				YYERROR;
-			}
-			free(buf);
+			if (!(ti = calloc(1, sizeof(*ti))))
+				err(1, "table_opt: calloc");
+			ti->file = $2;
+			SIMPLEQ_INSERT_TAIL(&table_opts.init_nodes, ti,
+			    entries);
+			table_opts.init_addr = 1;
 		}
 		;
 
@@ -1762,7 +1762,7 @@ xhost		: not host			{
 			$$ = $2;
 		}
 		| NOROUTE			{
-			$$ = calloc(1, sizeof(struct node_host));
+		$$ = calloc(1, sizeof(struct node_host));
 			if ($$ == NULL)
 				err(1, "xhost: calloc");
 			$$->addr.type = PF_ADDR_NOROUTE;
@@ -3154,6 +3154,50 @@ rdr_consistent(struct pf_rule *r)
 	return (-problems);
 }
 
+int
+process_tabledef(char *name, struct table_opts *opts)
+{
+	struct pfr_buffer	 ab;
+	struct node_tinit	*ti;
+
+	bzero(&ab, sizeof(ab));
+	ab.pfrb_type = PFRB_ADDRS;
+	SIMPLEQ_FOREACH(ti, &opts->init_nodes, entries) {
+		if (ti->file)
+			if (pfr_buf_load(&ab, ti->file, 0, append_addr)) {
+				if (errno)
+					yyerror("cannot load \"%s\": %s",
+					    ti->file, strerror(errno));
+				else
+					yyerror("file \"%s\" contains bad data",
+					    ti->file);
+				goto _error;
+			}
+		if (ti->host)
+			if (append_addr_host(&ab, ti->host, 0, 0)) {
+				yyerror("cannot create address buffer: %s",
+				    strerror(errno));
+				goto _error;
+			}
+	}
+	if (pf->opts & PF_OPT_VERBOSE)
+		print_tabledef(name, opts->flags, opts->init_addr,
+		    &opts->init_nodes);
+	if (!(pf->opts & PF_OPT_NOACTION) &&
+	    pfctl_define_table(name, opts->flags, opts->init_addr,
+	    pf->anchor, pf->ruleset, &ab, pf->tticket)) {
+		yyerror("cannot define table %s: %s", name,
+		    pfr_strerror(errno));
+		goto _error;
+	}
+	pf->tdirty = 1;
+	pfr_buf_clear(&ab);
+	return (0);
+_error:
+	pfr_buf_clear(&ab);
+	return (-1);
+}
+
 struct keywords {
 	const char	*k_name;
 	int		 k_val;
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 6769b1a2be0..131123fa382 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl.c,v 1.180 2003/07/03 21:09:13 cedric Exp $ */
+/*	$OpenBSD: pfctl.c,v 1.181 2003/07/11 08:29:34 cedric Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -921,13 +921,10 @@ pfctl_rules(int dev, char *filename, int opts, char *anchorname,
 	struct pfioc_rule	pr[PF_RULESET_MAX];
 	struct pfioc_altq	pa;
 	struct pfctl		pf;
-	struct pfr_buffer	ab;
 	int			i;
 
 	memset(&pa, 0, sizeof(pa));
 	memset(&pf, 0, sizeof(pf));
-	memset(&ab, 0, sizeof(ab));
-	ab.pfrb_type = PFRB_ADDRS;
 	for (i = 0; i < PF_RULESET_MAX; i++) {
 		memset(&pr[i], 0, sizeof(pr[i]));
 		memcpy(pr[i].anchor, anchorname, sizeof(pr[i].anchor));
@@ -969,7 +966,7 @@ pfctl_rules(int dev, char *filename, int opts, char *anchorname,
 				ERR("DIOCBEGINRULES");
 		}
 		if (loadopt & (PFCTL_FLAG_TABLE | PFCTL_FLAG_ALL)) {
-		        if (pfr_ina_begin(&pf.tticket, NULL, 0) != 0)
+			if (pfr_ina_begin(&pf.tticket, NULL, 0) != 0)
 				ERR("begin table");
 		}
 	}
@@ -978,7 +975,6 @@ pfctl_rules(int dev, char *filename, int opts, char *anchorname,
 	pf.opts = opts;
 	pf.loadopt = loadopt;
 	pf.paltq = &pa;
-	pf.ab = &ab;
 	for (i = 0; i < PF_RULESET_MAX; i++) {
 		pf.prule[i] = &pr[i];
 	}
diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index 368e24dcfec..55fa1d203bd 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_parser.c,v 1.167 2003/07/04 11:05:44 henning Exp $ */
+/*	$OpenBSD: pfctl_parser.c,v 1.168 2003/07/11 08:29:34 cedric Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -764,6 +764,42 @@ print_rule(struct pf_rule *r, int verbose)
 	printf("\n");
 }
 
+void
+print_tabledef(const char *name, int flags, int addrs,
+    struct node_tinithead *nodes)
+{
+	struct node_tinit	*ti, *nti;
+	struct node_host	*h;
+
+	printf("table <%s>", name);
+	if (flags & PFR_TFLAG_CONST)
+		printf(" const");
+	if (flags & PFR_TFLAG_PERSIST)
+		printf(" persist");
+	SIMPLEQ_FOREACH(ti, nodes, entries) {
+		if (ti->file) {
+			printf(" file \"%s\"", ti->file);
+			continue;
+		}
+		printf(" {");
+		for(;;) {
+			for (h = ti->host; h != NULL; h = h->next) {
+				printf(h->not ? " !" : " ");
+				print_addr(&h->addr, h->af, 0);
+			}
+			nti = SIMPLEQ_NEXT(ti, entries);
+			if (nti != NULL && nti->file == NULL)
+				ti = nti;	/* merge lists */
+			else
+				break;
+		}
+		printf(" }");
+	}
+	if (addrs && SIMPLEQ_EMPTY(nodes))
+		printf(" { }");
+	printf("\n");
+}
+
 int
 parse_flags(char *s)
 {
@@ -1182,35 +1218,35 @@ host_dns(const char *s, int v4mask, int v6mask)
 int
 append_addr(struct pfr_buffer *b, char *s, int test)
 {
-	return append_addr_not(b, s, test, 0);
+	char			 *r;
+	struct node_host	*n;
+	int			 not = 0;
+
+	for (r = s; *r == '!'; r++)
+		not = !not;
+	if ((n = host(r)) == NULL) {
+		errno = 0;
+		return (-1);
+	}
+	return append_addr_host(b, n, test, not);
 }
 
 /*
- * same as previous function, but with the ability to "negate" the result.
+ * same as previous function, but with a pre-parsed input and the ability
+ * to "negate" the result.
  * not:
- *	setting it to 1 is equivalent to adding "!" in front of parameter s.
+ *      setting it to 1 is equivalent to adding "!" in front of parameter s.
  */
 int
-append_addr_not(struct pfr_buffer *b, char *s, int test, int not)
+append_addr_host(struct pfr_buffer *b, struct node_host *n, int test, int not)
 {
-	char			 buf[256], *r;
 	int			 bits;
-	struct node_host	*n, *h;
+	struct node_host	*h;
 	struct pfr_addr		 addr;
 
-	for (r = s; *r == '!'; r++)
-		not = !not;
-	if (strlcpy(buf, r, sizeof(buf)) >= sizeof(buf)) {
-		errno = EINVAL;
-		return (-1);
-	}
-	if ((n = host(buf)) == NULL) {
-		errno = 0;
-		return (-1);
-	}
 	do {
 		bzero(&addr, sizeof(addr));
-		addr.pfra_not = not;
+		addr.pfra_not = n->not ^ not;
 		addr.pfra_af = n->af;
 		addr.pfra_net = unmask(&n->addr.v.a.mask, n->af);
 		switch (n->af) {
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index 48b5d7ab184..bcb4b8d3f2a 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_parser.h,v 1.63 2003/07/03 21:09:13 cedric Exp $ */
+/*	$OpenBSD: pfctl_parser.h,v 1.64 2003/07/11 08:29:34 cedric Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -67,7 +67,6 @@ struct pfctl {
 	struct pfioc_rule *prule[PF_RULESET_MAX];
 	struct pfioc_altq *paltq;
 	struct pfioc_queue *pqueue;
-	struct pfr_buffer *ab;		/* address buffer */
 	const char *anchor;
 	const char *ruleset;
 };
@@ -126,6 +125,15 @@ struct node_queue_opt {
 	}			 data;
 };
 
+SIMPLEQ_HEAD(node_tinithead, node_tinit);
+struct node_tinit {	/* table initializer */
+	SIMPLEQ_ENTRY(node_tinit)	 entries;
+	struct node_host		*host;
+	char				*file;
+};
+
+struct pfr_buffer;	/* forward definition */
+
 int	pfctl_rules(int, char *, int, char *, char *);
 
 int	pfctl_add_rule(struct pfctl *, struct pf_rule *);
@@ -144,6 +152,7 @@ int	pfctl_load_anchors(int, int);
 
 void	print_pool(struct pf_pool *, u_int16_t, u_int16_t, sa_family_t, int);
 void	print_rule(struct pf_rule *, int);
+void	print_tabledef(const char *, int, int, struct node_tinithead *);
 void	print_status(struct pf_status *);
 
 int	eval_pfaltq(struct pfctl *, struct pf_altq *, struct node_queue_bw *,
@@ -156,7 +165,7 @@ void	 print_altq(const struct pf_altq *, unsigned, struct node_queue_bw *,
 void	 print_queue(const struct pf_altq *, unsigned, struct node_queue_bw *,
 	     int, struct node_queue_opt *);
 
-int	pfctl_define_table(char *, int, int, int, const char *, const char *,
+int	pfctl_define_table(char *, int, int, const char *, const char *,
 	    struct pfr_buffer *, int);
 
 struct icmptypeent {
@@ -197,6 +206,7 @@ struct node_host	*ifa_lookup(const char *, enum pfctl_iflookup_mode);
 struct node_host	*host(const char *);
 
 int			 append_addr(struct pfr_buffer *, char *, int);
-int			 append_addr_not(struct pfr_buffer *, char *, int, int);
+int			 append_addr_host(struct pfr_buffer *,
+			    struct node_host *, int, int);
 
 #endif /* _PFCTL_PARSER_H_ */
diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
index ebe9dfa340b..c0500379872 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_table.c,v 1.47 2003/07/03 21:09:13 cedric Exp $ */
+/*	$OpenBSD: pfctl_table.c,v 1.48 2003/07/11 08:29:34 cedric Exp $ */
 
 /*
  * Copyright (c) 2002 Cedric Berger
@@ -442,31 +442,23 @@ radix_perror(void)
 }
 
 int
-pfctl_define_table(char *name, int flags, int addrs, int noaction,
-    const char *anchor, const char *ruleset, struct pfr_buffer *ab,
-    int ticket)
+pfctl_define_table(char *name, int flags, int addrs, const char *anchor,
+    const char *ruleset, struct pfr_buffer *ab, int ticket)
 {
 	struct pfr_table tbl;
-	int rv = 0;
-
-	if (!noaction) {
-		bzero(&tbl, sizeof(tbl));
-		if (strlcpy(tbl.pfrt_name, name,
-		    sizeof(tbl.pfrt_name)) >= sizeof(tbl.pfrt_name) ||
-		    strlcpy(tbl.pfrt_anchor, anchor,
-		    sizeof(tbl.pfrt_anchor)) >= sizeof(tbl.pfrt_anchor) ||
-		    strlcpy(tbl.pfrt_ruleset, ruleset,
-		    sizeof(tbl.pfrt_ruleset)) >= sizeof(tbl.pfrt_ruleset))
-			errx(1, "pfctl_define_table: strlcpy");
-		tbl.pfrt_flags = flags;
-
-		if (pfr_ina_define(&tbl, ab->pfrb_caddr, ab->pfrb_size, NULL,
-		    NULL, ticket, addrs ? PFR_FLAG_ADDRSTOO : 0) != 0) {
-			rv = -1;
-		}
-	}
-	pfr_buf_clear(ab);
-	return (rv);
+
+	bzero(&tbl, sizeof(tbl));
+	if (strlcpy(tbl.pfrt_name, name,
+	    sizeof(tbl.pfrt_name)) >= sizeof(tbl.pfrt_name) ||
+	    strlcpy(tbl.pfrt_anchor, anchor,
+	    sizeof(tbl.pfrt_anchor)) >= sizeof(tbl.pfrt_anchor) ||
+	    strlcpy(tbl.pfrt_ruleset, ruleset,
+	    sizeof(tbl.pfrt_ruleset)) >= sizeof(tbl.pfrt_ruleset))
+		errx(1, "pfctl_define_table: strlcpy");
+	tbl.pfrt_flags = flags;
+
+	return pfr_ina_define(&tbl, ab->pfrb_caddr, ab->pfrb_size, NULL,
+	    NULL, ticket, addrs ? PFR_FLAG_ADDRSTOO : 0);
 }
 
 void