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

[Pablo Neira Ayuso]

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 :)