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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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);
> +			}
> +			;