Re: [PATCH nft v3 1/2] jump: Introduce chain_expr in jump and goto statements

[Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>]

On Thu, May 16, 2019 at 10:45:58PM +0200, Fernando Fernandez Mancera wrote:
> Now we can introduce expressions as a chain in jump and goto statements. This
> is going to be used to support variables as a chain in the following patches.

Something is wrong with json:

json.c: In function ‘verdict_expr_json’:
json.c:683:11: warning: assignment from incompatible pointer type
[-Wincompatible-pointer-types]
     chain = expr->chain;
           ^
parser_json.c: In function ‘json_parse_verdict_expr’:
parser_json.c:1086:8: warning: passing argument 3 of
‘verdict_expr_alloc’ from incompatible pointer type
[-Wincompatible-pointer-types]
        chain ? xstrdup(chain) : NULL);
        ^~~~~

Most likely --enable-json missing there.

 diff --git a/src/datatype.c b/src/datatype.c
> index ac9f2af..10f185b 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -254,6 +254,8 @@ const struct datatype invalid_type = {
>  
>  static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  {
> +	char chain[NFT_CHAIN_MAXNAMELEN];
> +
>  	switch (expr->verdict) {
>  	case NFT_CONTINUE:
>  		nft_print(octx, "continue");
> @@ -262,10 +264,26 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  		nft_print(octx, "break");
>  		break;
>  	case NFT_JUMP:
> -		nft_print(octx, "jump %s", expr->chain);
> +		if (expr->chain->etype == EXPR_VALUE) {
> +			mpz_export_data(chain, expr->chain->value,
> +					BYTEORDER_HOST_ENDIAN,
> +					NFT_CHAIN_MAXNAMELEN);
> +			nft_print(octx, "jump %s", chain);
> +		} else {
> +			nft_print(octx, "jump ");
> +			expr_print(expr->chain, octx);
> +		}

I think this should be fine:

        case NFT_JUMP:
		nft_print(octx, "jump ");
		expr_print(expr->chain, octx);
                break;

Any reason to have the 'if (expr->chain->etype == EXPR_VALUE) {'
check?

>  		break;
>  	case NFT_GOTO:
> -		nft_print(octx, "goto %s", expr->chain);
> +		if (expr->chain->etype == EXPR_VALUE) {
> +			mpz_export_data(chain, expr->chain->value,
> +					BYTEORDER_HOST_ENDIAN,
> +					NFT_CHAIN_MAXNAMELEN);
> +			nft_print(octx, "goto %s", chain);
> +		} else {
> +			nft_print(octx, "goto ");
> +			expr_print(expr->chain, octx);

Same thing here.

Apart from those nitpicks, this looks good :)