[PATCH RFC 0/2] nftables: fix time parsing

[PATCH RFC 0/2] nftables: fix time parsing

[Patrick McHardy]

These patches attempt to fix time parsing to not require quotation marks
around the time spec and loosen the checks so times can also be specified
in smaller units.

Some testing indicates it works fine, however I'd put them into a branch
for 4.1 since I mainly need this fixed for the set timeout support.


Patrick McHardy (2):
  datatype: fix parsing of time type
  datatype: less strict time parsing

 src/datatype.c | 16 ----------------
 src/scanner.l  |  7 +++++++
 2 files changed, 7 insertions(+), 16 deletions(-)

-- 
2.1.0

[PATCH 1/2] datatype: fix parsing of time type

[Patrick McHardy]

Properly detect time strings in the lexer without quotation marks.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/datatype.c | 4 ----
 src/scanner.l  | 7 +++++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index c93f76a..0772b50 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -775,8 +775,6 @@ static void time_type_print(const struct expr *expr)
 	minutes = seconds / 60;
 	seconds %= 60;
 
-	printf("\"");
-
 	if (days > 0)
 		printf("%"PRIu64"d", days);
 	if (hours > 0)
@@ -785,8 +783,6 @@ static void time_type_print(const struct expr *expr)
 		printf("%"PRIu64"m", minutes);
 	if (seconds > 0)
 		printf("%"PRIu64"s", seconds);
-
-	printf("\"");
 }
 
 enum {
diff --git a/src/scanner.l b/src/scanner.l
index 73c4f8b..27d95bf 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -117,6 +117,8 @@ quotedstring	\"[^"]*\"
 comment		#.*$
 slash		\/
 
+timestring	([0-9]+d)?([0-9]+h)?([0-9]+m)?([0-9]+s)?
+
 hex4		([[:xdigit:]]{1,4})
 v680		(({hex4}:){7}{hex4})
 v670		((:)((:{hex4}){7}))
@@ -457,6 +459,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 				return STRING;
 			}
 
+{timestring}		{
+				yylval->string = xstrdup(yytext);
+				return STRING;
+			}
+
 {decstring}		{
 				errno = 0;
 				yylval->val = strtoull(yytext, NULL, 0);
-- 
2.1.0

[PATCH 2/2] datatype: less strict time parsing

[Patrick McHardy]

Don't require hours to be in range 0-23 and minutes/seconds in range 0-59.
The time_type is used for relative times where it is entirely reasonable
to specify 180s instead of 3m.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/datatype.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index 0772b50..1c83715 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -831,10 +831,6 @@ static struct error_record *time_type_parse(const struct expr *sym,
 			}
 			h = str2int(tmp, c, k);
 			k = 0;
-			if (h > 23) {
-				return error(&sym->location,
-					     "Hour needs to be 0-23");
-			}
 			mask |= HOUR;
 			break;
 		case 'm':
@@ -844,10 +840,6 @@ static struct error_record *time_type_parse(const struct expr *sym,
 			}
 			m = str2int(tmp, c, k);
 			k = 0;
-			if (m > 59) {
-				return error(&sym->location,
-					     "Minute needs to be 0-59");
-			}
 			mask |= MIN;
 			break;
 		case 's':
@@ -857,10 +849,6 @@ static struct error_record *time_type_parse(const struct expr *sym,
 			}
 			s = str2int(tmp, c, k);
 			k = 0;
-			if (s > 59) {
-				return error(&sym->location,
-					     "second needs to be 0-59");
-			}
 			mask |= SECS;
 			break;
 		default:
-- 
2.1.0

Re: [PATCH RFC 0/2] nftables: fix time parsing

[Pablo Neira Ayuso]

On Sat, Apr 11, 2015 at 03:00:28PM +0100, Patrick McHardy wrote:
> These patches attempt to fix time parsing to not require quotation marks
> around the time spec and loosen the checks so times can also be specified
> in smaller units.
> 
> Some testing indicates it works fine, however I'd put them into a branch
> for 4.1 since I mainly need this fixed for the set timeout support.

Thanks, this looks fine to me.

I still think that we need to consider flex start conditions or
something similar to make the scanner a bit smarter (stateful) to
avoid complexity on the parser.

I think that may also help to avoid the side issue related to this
patch:

http://patchwork.ozlabs.org/patch/443091/

Basically, the scanner takes this:

tcp sport vmap { 25:accept, 28:drop };ok
                 ^^^^^

as a tc handle.

Re: [PATCH RFC 0/2] nftables: fix time parsing

[Patrick McHardy]

On 11.04, Pablo Neira Ayuso wrote:
> On Sat, Apr 11, 2015 at 03:00:28PM +0100, Patrick McHardy wrote:
> > These patches attempt to fix time parsing to not require quotation marks
> > around the time spec and loosen the checks so times can also be specified
> > in smaller units.
> > 
> > Some testing indicates it works fine, however I'd put them into a branch
> > for 4.1 since I mainly need this fixed for the set timeout support.
> 
> Thanks, this looks fine to me.

Still reworking it slightly for the set related timeouts. Will post
a final version later tonight I think.

> I still think that we need to consider flex start conditions or
> something similar to make the scanner a bit smarter (stateful) to
> avoid complexity on the parser.
> 
> I think that may also help to avoid the side issue related to this
> patch:
> 
> http://patchwork.ozlabs.org/patch/443091/
> 
> Basically, the scanner takes this:
> 
> tcp sport vmap { 25:accept, 28:drop };ok
>                  ^^^^^
> 
> as a tc handle.

The problem with start conditions is that we need the parser to actually
start them. This means the parser will have to know, in advance, what
to expect. TC handles are actually one of the easier cases since they
can only occur on the RHS of a meta stmt, but it gets ugly very quickly.

Overall, I don't think they will provide much help, have already tried
many times.

I'm thinking - basically these cases are not too common, keywords require
seperators anyways. Maybe greedily matching for non-keywords until clear
seperators like ",", whitespace, {, }, ... might help. Not sure if flex
is capable of doing that.

I can look into that at the end of next week.