[PATCH] parser: simplify monitor command parsing

[PATCH] parser: simplify monitor command parsing

[Patrick McHardy]

Add tokens for "new" and "destroy". Split up the monitor flags into an
event and an object to avoid lots of duplicated code.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/parser.y  | 181 ++++++++++++----------------------------------------------
 src/scanner.l |   3 +
 2 files changed, 39 insertions(+), 145 deletions(-)

diff --git a/src/parser.y b/src/parser.y
index 6737aad..653c764 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -93,21 +93,6 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 
 #define YYLLOC_DEFAULT(Current, Rhs, N)	location_update(&Current, Rhs, N)
 
-enum {
-	NFT_EVENT_NEW	= 0,
-	NFT_EVENT_DEL,
-};
-
-static int monitor_lookup_event(const char *event)
-{
-	if (strcmp(event, "new") == 0)
-		return NFT_EVENT_NEW;
-	else if (strcmp(event, "destroy") == 0)
-		return NFT_EVENT_DEL;
-
-	return -1;
-}
-
 %}
 
 /* Declaration section */
@@ -209,6 +194,9 @@ static int monitor_lookup_event(const char *event)
 %token GOTO			"goto"
 %token RETURN			"return"
 
+%token NEW			"new"
+%token DESTROY			"destroy"
+
 %token CONSTANT			"constant"
 %token INTERVAL			"interval"
 %token ELEMENTS			"elements"
@@ -525,7 +513,7 @@ static int monitor_lookup_event(const char *event)
 %destructor { expr_free($$); }	ct_expr
 %type <val>			ct_key
 
-%type <val>			export_format	output_format	monitor_flags
+%type <val>			export_format	output_format	monitor_event monitor_object
 
 %%
 
@@ -797,16 +785,16 @@ export_cmd		:	export_format
 			}
 			;
 
-monitor_cmd		:	monitor_flags	output_format
+monitor_cmd		:	monitor_event	monitor_object	output_format
 			{
 				struct handle h = { .family = NFPROTO_UNSPEC };
 				$$ = cmd_alloc(CMD_MONITOR, CMD_OBJ_RULESET, &h, &@$, NULL);
-				$$->monitor_flags = $1;
-				$$->format = $2;
+				$$->monitor_flags = $1 & $2;
+				$$->format = $3;
 			}
 			;
 
-monitor_flags		:	/* empty */
+monitor_event		:	/* empty */
 			{
 				$$ = (1 << NFT_MSG_NEWRULE)	|
 				     (1 << NFT_MSG_DELRULE)	|
@@ -819,159 +807,62 @@ monitor_flags		:	/* empty */
 				     (1 << NFT_MSG_NEWTABLE)	|
 				     (1 << NFT_MSG_DELTABLE);
 			}
-			|	STRING
+			|	NEW
 			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
+				$$ = (1 << NFT_MSG_NEWTABLE)	|
+				     (1 << NFT_MSG_NEWCHAIN)	|
+				     (1 << NFT_MSG_NEWRULE)	|
+				     (1 << NFT_MSG_NEWSET)	|
+				     (1 << NFT_MSG_NEWSETELEM);
+			}
+			|	DESTROY
+			{
+				$$ = (1 << NFT_MSG_DELTABLE)	|
+				     (1 << NFT_MSG_DELCHAIN)	|
+				     (1 << NFT_MSG_DELRULE)	|
+				     (1 << NFT_MSG_DELSET)	|
+				     (1 << NFT_MSG_DELSETELEM);
+			}
+			;
 
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWTABLE)	|
-					     (1 << NFT_MSG_NEWCHAIN)	|
-					     (1 << NFT_MSG_NEWRULE)	|
-					     (1 << NFT_MSG_NEWSET)	|
-					     (1 << NFT_MSG_NEWSETELEM);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELTABLE)	|
-					     (1 << NFT_MSG_DELCHAIN)	|
-					     (1 << NFT_MSG_DELRULE)	|
-					     (1 << NFT_MSG_DELSET)	|
-					     (1 << NFT_MSG_DELSETELEM);
-					break;
-				}
+monitor_object		:	/* empty */
+			{
+				$$ = (1 << NFT_MSG_NEWRULE)	|
+				     (1 << NFT_MSG_DELRULE)	|
+				     (1 << NFT_MSG_NEWSET)	|
+				     (1 << NFT_MSG_DELSET)	|
+				     (1 << NFT_MSG_NEWSETELEM)	|
+				     (1 << NFT_MSG_DELSETELEM)	|
+				     (1 << NFT_MSG_NEWCHAIN)	|
+				     (1 << NFT_MSG_DELCHAIN)	|
+				     (1 << NFT_MSG_NEWTABLE)	|
+				     (1 << NFT_MSG_DELTABLE);
 			}
 			|	TABLES
 			{
 				$$ = (1 << NFT_MSG_NEWTABLE) |
 				     (1 << NFT_MSG_DELTABLE);
 			}
-			|	STRING 	TABLES
-			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
-
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWTABLE);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELTABLE);
-					break;
-				}
-			}
 			|	CHAINS
 			{
 				$$ = (1 << NFT_MSG_NEWCHAIN) |
 				     (1 << NFT_MSG_DELCHAIN);
 			}
-			|	STRING	CHAINS
-			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
-
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWCHAIN);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELCHAIN);
-					break;
-				}
-			}
 			|	SETS
 			{
 				$$ = (1 << NFT_MSG_NEWSET) |
 				     (1 << NFT_MSG_DELSET);
 			}
-			|	STRING	SETS
-			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
-
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWSET);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELSET);
-					break;
-				}
-			}
 			|	RULES
 			{
 				$$ = (1 << NFT_MSG_NEWRULE) |
 				     (1 << NFT_MSG_DELRULE);
 			}
-			|	STRING 	RULES
-			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
-
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWRULE);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELRULE);
-					break;
-				}
-			}
 			|	ELEMENTS
 			{
 				$$ = (1 << NFT_MSG_NEWSETELEM) |
 				     (1 << NFT_MSG_DELSETELEM);
 			}
-			|	STRING	ELEMENTS
-			{
-				int event;
-
-				event = monitor_lookup_event($1);
-				if (event < 0) {
-					erec_queue(error(&@1, "unknown event type %s", $1),
-						   state->msgs);
-					YYERROR;
-				}
-
-				switch (event) {
-				case NFT_EVENT_NEW:
-					$$ = (1 << NFT_MSG_NEWSETELEM);
-					break;
-				case NFT_EVENT_DEL:
-					$$ = (1 << NFT_MSG_DELSETELEM);
-					break;
-				}
-			}
 			;
 
 output_format		:	/* empty */
diff --git a/src/scanner.l b/src/scanner.l
index 601ea3c..8aab38f 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -261,6 +261,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "export"		{ return EXPORT; }
 "monitor"		{ return MONITOR; }
 
+"new"			{ return NEW; }
+"destroy"		{ return DESTROY; }
+
 "position"		{ return POSITION; }
 "comment"		{ return COMMENT; }
 
-- 
1.9.3

Re: [PATCH] parser: simplify monitor command parsing

[Arturo Borrero Gonzalez]

On 17 September 2014 09:45, Patrick McHardy <kaber@trash.net> wrote:
> Add tokens for "new" and "destroy". Split up the monitor flags into an
> event and an object to avoid lots of duplicated code.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---

I think the 'new' keyword may clash with 'ct new', don't you?. That's
why Pablo did the parser that way.

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] parser: simplify monitor command parsing

[Patrick McHardy]

On Wed, Sep 17, 2014 at 09:52:11AM +0200, Arturo Borrero Gonzalez wrote:
> On 17 September 2014 09:45, Patrick McHardy <kaber@trash.net> wrote:
> > Add tokens for "new" and "destroy". Split up the monitor flags into an
> > event and an object to avoid lots of duplicated code.
> >
> > Signed-off-by: Patrick McHardy <kaber@trash.net>
> > ---
> 
> I think the 'new' keyword may clash with 'ct new', don't you?. That's
> why Pablo did the parser that way.

Right. The split into event and object still saves a lot of code, I'll
add back the strcmp if I can't think of a nicer way to fix this.

I'm working on command line completion based on the bison grammar, so
I'd prefer to only use strings for identifiers, not for keywords.

Re: [PATCH] parser: simplify monitor command parsing

[Eric Leblond]

Hi,

On Wed, 2014-09-17 at 10:01 +0200, Patrick McHardy wrote:
> On Wed, Sep 17, 2014 at 09:52:11AM +0200, Arturo Borrero Gonzalez wrote:
> > On 17 September 2014 09:45, Patrick McHardy <kaber@trash.net> wrote:
> > > Add tokens for "new" and "destroy". Split up the monitor flags into an
> > > event and an object to avoid lots of duplicated code.
> > >
> > > Signed-off-by: Patrick McHardy <kaber@trash.net>
> > > ---
> > 
> > I think the 'new' keyword may clash with 'ct new', don't you?. That's
> > why Pablo did the parser that way.
> 
> Right. The split into event and object still saves a lot of code, I'll
> add back the strcmp if I can't think of a nicer way to fix this.
> 
> I'm working on command line completion based on the bison grammar, so
> I'd prefer to only use strings for identifiers, not for keywords.

I confirm that 102c300 "parser: simplify monitor command parsing" breaks
the "ct state new" filter so current nftables git is not usable. 

For the record the error is the following:
 nft> add rule inet filter input ct state new
 <cli>:1:37-39: Error: syntax error, unexpected new
 add rule inet filter input ct state new
                                     ^^^ 

And this is also caught by regression tests:
nftables/tests/regression# ./nft-test.py any/ct.t 
any/ct.t: ERROR: line 10: nft add rule -nnn ip test-ip4 output ct state new,established, related, untracked: This rule should not have failed.
any/ct.t: ERROR: line 12: nft add rule -nnn ip test-ip4 output ct state {new,established, related, untracked}: This rule should not have failed.

Patrick, let me know if you can look into it. If not I will try to have
a look this week end or next week.

BR,
-- 
Eric Leblond <eric@regit.org>

Re: [PATCH] parser: simplify monitor command parsing

[Patrick McHardy]

On Wed, Sep 24, 2014 at 10:46:46AM +0200, Eric Leblond wrote:
> Hi,
> 
> On Wed, 2014-09-17 at 10:01 +0200, Patrick McHardy wrote:
> > On Wed, Sep 17, 2014 at 09:52:11AM +0200, Arturo Borrero Gonzalez wrote:
> > > On 17 September 2014 09:45, Patrick McHardy <kaber@trash.net> wrote:
> > > > Add tokens for "new" and "destroy". Split up the monitor flags into an
> > > > event and an object to avoid lots of duplicated code.
> > > >
> > > > Signed-off-by: Patrick McHardy <kaber@trash.net>
> > > > ---
> > > 
> > > I think the 'new' keyword may clash with 'ct new', don't you?. That's
> > > why Pablo did the parser that way.
> > 
> > Right. The split into event and object still saves a lot of code, I'll
> > add back the strcmp if I can't think of a nicer way to fix this.
> > 
> > I'm working on command line completion based on the bison grammar, so
> > I'd prefer to only use strings for identifiers, not for keywords.
> 
> I confirm that 102c300 "parser: simplify monitor command parsing" breaks
> the "ct state new" filter so current nftables git is not usable. 
> 
> For the record the error is the following:
>  nft> add rule inet filter input ct state new
>  <cli>:1:37-39: Error: syntax error, unexpected new
>  add rule inet filter input ct state new
>                                      ^^^ 
> 
> And this is also caught by regression tests:
> nftables/tests/regression# ./nft-test.py any/ct.t 
> any/ct.t: ERROR: line 10: nft add rule -nnn ip test-ip4 output ct state new,established, related, untracked: This rule should not have failed.
> any/ct.t: ERROR: line 12: nft add rule -nnn ip test-ip4 output ct state {new,established, related, untracked}: This rule should not have failed.
> 
> Patrick, let me know if you can look into it. If not I will try to have
> a look this week end or next week.

I have a rather large patchset that reorganizes the parser and fixes
this for all constants that clash with keywords, but it will take a
day or two.