diff mbox series

[1/2,nft,WIP] src: introduce prio_expr in chain priority

Message ID 20190716090812.873-2-ffmancera@riseup.net
State RFC
Delegated to: Pablo Neira
Headers show
Series Using variables in chain priority | expand

Commit Message

Fernando F. Mancera July 16, 2019, 9:08 a.m. UTC 
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 include/rule.h     |  8 ++++----
 src/evaluate.c     | 29 +++++++++++++++++++----------
 src/parser_bison.y | 25 +++++++++++++++++--------
 src/rule.c         |  4 ++--
 4 files changed, 42 insertions(+), 24 deletions(-)

Comments

Pablo Neira Ayuso July 16, 2019, 6:06 p.m. UTC | #1 
On Tue, Jul 16, 2019 at 11:08:12AM +0200, Fernando Fernandez Mancera wrote:
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  include/rule.h     |  8 ++++----
>  src/evaluate.c     | 29 +++++++++++++++++++----------
>  src/parser_bison.y | 25 +++++++++++++++++--------
>  src/rule.c         |  4 ++--
>  4 files changed, 42 insertions(+), 24 deletions(-)
> 
> diff --git a/include/rule.h b/include/rule.h
> index aefb24d..4d7cec8 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -173,13 +173,13 @@ enum chain_flags {
>   * struct prio_spec - extendend priority specification for mixed
>   *                    textual/numerical parsing.
>   *
> - * @str:  name of the standard priority value
> - * @num:  Numerical value. This MUST contain the parsed value of str after
> + * @prio_expr:  expr of the standard priority value
> + * @num:  Numerical value. This MUST contain the parsed value of prio_expr after
>   *        evaluation.
>   */
>  struct prio_spec {
> -	const char  *str;
> -	int          num;
> +	struct expr	*prio_expr;

Use:

        struct expr     *expr;

instead.

> +	int		num;

You could just store this in the expression, no need for this num
field.

>  	struct location loc;
>  };
>  
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 8086f75..cee65cd 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3181,15 +3181,24 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
>  	return 0;
>  }
>  
> -static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
> +static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
> +			      int family, int hook)
>  {
>  	int priority;
> +	char prio_str[NFT_NAME_MAXLEN];
>  
>  	/* A numeric value has been used to specify priority. */
> -	if (prio->str == NULL)
> +	if (prio->prio_expr == NULL)

prio_expr == NULL never happens.

>  		return true;
>  
> -	priority = std_prio_lookup(prio->str, family, hook);
> +	if (expr_evaluate(ctx, &prio->prio_expr) < 0)
> +		return false;
> +	if (prio->prio_expr->etype == EXPR_VALUE)

You should bail out here with expr_error() is this is not an EXPR_VALUE.

> +		mpz_export_data(prio_str, prio->prio_expr->value,
> +				BYTEORDER_HOST_ENDIAN,
> +				NFT_NAME_MAXLEN);

Remove the branch above, expr_evalute() already deals with
transforming the symbol to value expression.

> +
> +	priority = std_prio_lookup(prio_str, family, hook);
>  	if (priority == NF_IP_PRI_LAST)
>  		return false;
>  	prio->num += priority;
> @@ -3211,10 +3220,10 @@ static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
>  	if (ft->hooknum == NF_INET_NUMHOOKS)
>  		return chain_error(ctx, ft, "invalid hook %s", ft->hookstr);
>  
> -	if (!evaluate_priority(&ft->priority, NFPROTO_NETDEV, ft->hooknum))
> +	if (!evaluate_priority(ctx, &ft->priority, NFPROTO_NETDEV, ft->hooknum))
>  		return __stmt_binary_error(ctx, &ft->priority.loc, NULL,

Better use expr_error() here?

> -					   "'%s' is invalid priority.",
> -					   ft->priority.str);
> +					   "invalid priority expression %s.",

> +					   expr_name(ft->priority.prio_expr));
>  
>  	if (!ft->dev_expr)
>  		return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)");
> @@ -3410,11 +3419,11 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>  			return chain_error(ctx, chain, "invalid hook %s",
>  					   chain->hookstr);
>  
> -		if (!evaluate_priority(&chain->priority, chain->handle.family,
> -				       chain->hooknum))
> +		if (!evaluate_priority(ctx, &chain->priority,
> +				       chain->handle.family, chain->hooknum))
>  			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> -						   "'%s' is invalid priority in this context.",
> -						   chain->priority.str);
> +						   "invalid priority expression %s in this context.",
> +						   expr_name(chain->priority.prio_expr));
>  	}
>  
>  	list_for_each_entry(rule, &chain->rules, list) {
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index a4905f2..c6a43cf 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -626,8 +626,8 @@ int nft_lex(void *, void *, void *);
>  %type <stmt>			meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
>  %destructor { stmt_free($$); }	meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
>  
> -%type <expr>			symbol_expr verdict_expr integer_expr variable_expr chain_expr
> -%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr chain_expr
> +%type <expr>			symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
> +%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
>  %type <expr>			primary_expr shift_expr and_expr
>  %destructor { expr_free($$); }	primary_expr shift_expr and_expr
>  %type <expr>			exclusive_or_expr inclusive_or_expr
> @@ -1926,30 +1926,39 @@ extended_prio_name	:	OUT
>  			|	STRING
>  			;
>  
> +prio_expr		:	extended_prio_name
> +			{
> +				$$ = constant_expr_alloc(&@$, &string_type,
> +							 BYTEORDER_HOST_ENDIAN,
> +							 NFT_NAME_MAXLEN *

this should be strlen($1) * BITS_PER_BYTE

> +							 BITS_PER_BYTE, $1);
> +			}
> +			;
Fernando F. Mancera July 16, 2019, 10:42 p.m. UTC | #2 
Hi Pablo, thanks for reviewing. Comments below.

On 7/16/19 8:06 PM, Pablo Neira Ayuso wrote:
> On Tue, Jul 16, 2019 at 11:08:12AM +0200, Fernando Fernandez Mancera wrote:
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>>  include/rule.h     |  8 ++++----
>>  src/evaluate.c     | 29 +++++++++++++++++++----------
>>  src/parser_bison.y | 25 +++++++++++++++++--------
>>  src/rule.c         |  4 ++--
>>  4 files changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/include/rule.h b/include/rule.h
>> index aefb24d..4d7cec8 100644
>> --- a/include/rule.h
>> +++ b/include/rule.h
>> @@ -173,13 +173,13 @@ enum chain_flags {
>>   * struct prio_spec - extendend priority specification for mixed
>>   *                    textual/numerical parsing.
>>   *
>> - * @str:  name of the standard priority value
>> - * @num:  Numerical value. This MUST contain the parsed value of str after
>> + * @prio_expr:  expr of the standard priority value
>> + * @num:  Numerical value. This MUST contain the parsed value of prio_expr after
>>   *        evaluation.
>>   */
>>  struct prio_spec {
>> -	const char  *str;
>> -	int          num;
>> +	struct expr	*prio_expr;
> 
> Use:
> 
>         struct expr     *expr;
> 
> instead.
> 
>> +	int		num;
> 
> You could just store this in the expression, no need for this num
> field.
> 

I think that would not be possible. Right now, the priority
specification supports combinations of a string and a number. e.g

table inet global {
    chain prerouting {
        type filter hook prerouting priority filter + 3
        policy accept
    }
}

or

table inet global {
    chain prerouting {
        type filter hook prerouting priority filter - 3
        policy accept
    }
}

I don't think we are going to be able to do that using only a single
"struct expr *".

>>  	struct location loc;
>>  };
>>  
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> index 8086f75..cee65cd 100644
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -3181,15 +3181,24 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
>>  	return 0;
>>  }
>>  
>> -static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
>> +static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
>> +			      int family, int hook)
>>  {
>>  	int priority;
>> +	char prio_str[NFT_NAME_MAXLEN];
>>  
>>  	/* A numeric value has been used to specify priority. */
>> -	if (prio->str == NULL)
>> +	if (prio->prio_expr == NULL)
> 
> prio_expr == NULL never happens.
> 

It never happens if we only have a single field in the "struct prio_spec".

Thanks!
diff mbox series

Patch

diff --git a/include/rule.h b/include/rule.h
index aefb24d..4d7cec8 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -173,13 +173,13 @@  enum chain_flags {
  * struct prio_spec - extendend priority specification for mixed
  *                    textual/numerical parsing.
  *
- * @str:  name of the standard priority value
- * @num:  Numerical value. This MUST contain the parsed value of str after
+ * @prio_expr:  expr of the standard priority value
+ * @num:  Numerical value. This MUST contain the parsed value of prio_expr after
  *        evaluation.
  */
 struct prio_spec {
-	const char  *str;
-	int          num;
+	struct expr	*prio_expr;
+	int		num;
 	struct location loc;
 };
 
diff --git a/src/evaluate.c b/src/evaluate.c
index 8086f75..cee65cd 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3181,15 +3181,24 @@  static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	return 0;
 }
 
-static bool evaluate_priority(struct prio_spec *prio, int family, int hook)
+static bool evaluate_priority(struct eval_ctx *ctx, struct prio_spec *prio,
+			      int family, int hook)
 {
 	int priority;
+	char prio_str[NFT_NAME_MAXLEN];
 
 	/* A numeric value has been used to specify priority. */
-	if (prio->str == NULL)
+	if (prio->prio_expr == NULL)
 		return true;
 
-	priority = std_prio_lookup(prio->str, family, hook);
+	if (expr_evaluate(ctx, &prio->prio_expr) < 0)
+		return false;
+	if (prio->prio_expr->etype == EXPR_VALUE)
+		mpz_export_data(prio_str, prio->prio_expr->value,
+				BYTEORDER_HOST_ENDIAN,
+				NFT_NAME_MAXLEN);
+
+	priority = std_prio_lookup(prio_str, family, hook);
 	if (priority == NF_IP_PRI_LAST)
 		return false;
 	prio->num += priority;
@@ -3211,10 +3220,10 @@  static int flowtable_evaluate(struct eval_ctx *ctx, struct flowtable *ft)
 	if (ft->hooknum == NF_INET_NUMHOOKS)
 		return chain_error(ctx, ft, "invalid hook %s", ft->hookstr);
 
-	if (!evaluate_priority(&ft->priority, NFPROTO_NETDEV, ft->hooknum))
+	if (!evaluate_priority(ctx, &ft->priority, NFPROTO_NETDEV, ft->hooknum))
 		return __stmt_binary_error(ctx, &ft->priority.loc, NULL,
-					   "'%s' is invalid priority.",
-					   ft->priority.str);
+					   "invalid priority expression %s.",
+					   expr_name(ft->priority.prio_expr));
 
 	if (!ft->dev_expr)
 		return chain_error(ctx, ft, "Unbound flowtable not allowed (must specify devices)");
@@ -3410,11 +3419,11 @@  static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 			return chain_error(ctx, chain, "invalid hook %s",
 					   chain->hookstr);
 
-		if (!evaluate_priority(&chain->priority, chain->handle.family,
-				       chain->hooknum))
+		if (!evaluate_priority(ctx, &chain->priority,
+				       chain->handle.family, chain->hooknum))
 			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
-						   "'%s' is invalid priority in this context.",
-						   chain->priority.str);
+						   "invalid priority expression %s in this context.",
+						   expr_name(chain->priority.prio_expr));
 	}
 
 	list_for_each_entry(rule, &chain->rules, list) {
diff --git a/src/parser_bison.y b/src/parser_bison.y
index a4905f2..c6a43cf 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -626,8 +626,8 @@  int nft_lex(void *, void *, void *);
 %type <stmt>			meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
 %destructor { stmt_free($$); }	meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
 
-%type <expr>			symbol_expr verdict_expr integer_expr variable_expr chain_expr
-%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr chain_expr
+%type <expr>			symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
+%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr chain_expr prio_expr
 %type <expr>			primary_expr shift_expr and_expr
 %destructor { expr_free($$); }	primary_expr shift_expr and_expr
 %type <expr>			exclusive_or_expr inclusive_or_expr
@@ -1926,30 +1926,39 @@  extended_prio_name	:	OUT
 			|	STRING
 			;
 
+prio_expr		:	extended_prio_name
+			{
+				$$ = constant_expr_alloc(&@$, &string_type,
+							 BYTEORDER_HOST_ENDIAN,
+							 NFT_NAME_MAXLEN *
+							 BITS_PER_BYTE, $1);
+			}
+			;
+
 extended_prio_spec	:	int_num
 			{
 				struct prio_spec spec = {0};
 				spec.num = $1;
 				$$ = spec;
 			}
-			|	extended_prio_name
+			|	prio_expr
 			{
 				struct prio_spec spec = {0};
-				spec.str = $1;
+				spec.prio_expr = $1;
 				$$ = spec;
 			}
-			|	extended_prio_name PLUS NUM
+			|	prio_expr PLUS NUM
 			{
 				struct prio_spec spec = {0};
 				spec.num = $3;
-				spec.str = $1;
+				spec.prio_expr = $1;
 				$$ = spec;
 			}
-			|	extended_prio_name DASH NUM
+			|	prio_expr DASH NUM
 			{
 				struct prio_spec spec = {0};
 				spec.num = -$3;
-				spec.str = $1;
+				spec.prio_expr = $1;
 				$$ = spec;
 			}
 			;
diff --git a/src/rule.c b/src/rule.c
index 0a91917..59a97ac 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -823,7 +823,7 @@  void chain_free(struct chain *chain)
 	xfree(chain->type);
 	if (chain->dev != NULL)
 		xfree(chain->dev);
-	xfree(chain->priority.str);
+	expr_free(chain->priority.prio_expr);
 	xfree(chain);
 }
 
@@ -2020,7 +2020,7 @@  void flowtable_free(struct flowtable *flowtable)
 	if (--flowtable->refcnt > 0)
 		return;
 	handle_free(&flowtable->handle);
-	xfree(flowtable->priority.str);
+	expr_free(flowtable->priority.prio_expr);
 	xfree(flowtable);
 }