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

[Fernando Fernandez Mancera <ffmancera@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>>  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!