Re: [PATCH nf-next v4] parser_json: support handle for rule positioning without breaking other objects

[Florian Westphal <fw@strlen.de>]

Phil Sutter <phil@nwl.cc> wrote:
> > +/* Mask for flags that affect expression parsing context */
> > +#define CTX_F_EXPR_MASK	(CTX_F_RHS | CTX_F_STMT | CTX_F_PRIMARY | CTX_F_DTYPE | \
> > +			 CTX_F_SET_RHS | CTX_F_MANGLE | CTX_F_SES | CTX_F_MAP | \
> > +			 CTX_F_CONCAT)
> 
> Maybe define as 'UINT32_MAX & ~(CTX_F_COLLAPSED | CTX_F_IMPLICIT)'
> instead?

> >  struct json_ctx {
> >  	struct nft_ctx *nft;
> > @@ -1725,10 +1731,14 @@ static struct expr *json_parse_expr(struct json_ctx *ctx, json_t *root)
> >  		return NULL;
> >  
> >  	for (i = 0; i < array_size(cb_tbl); i++) {
> > +		uint32_t expr_flags;
> > +
> >  		if (strcmp(type, cb_tbl[i].name))
> >  			continue;
> >  
> > -		if ((cb_tbl[i].flags & ctx->flags) != ctx->flags) {
> > +		/* Only check expression context flags, not command-level flags */
> > +		expr_flags = ctx->flags & CTX_F_EXPR_MASK;
> > +		if ((cb_tbl[i].flags & expr_flags) != expr_flags) {

So when adding CTX_F_BLA as new expr flag, one has to rember to add it
to CTX_F_EXPR_MASK.  Given that I concur with Phil.

> >  	rule = rule_alloc(int_loc, NULL);
> >  
> >  	json_unpack(root, "{s:s}", "comment", &comment);
> > @@ -4352,8 +4374,21 @@ static struct cmd *json_parse_cmd(struct json_ctx *ctx, json_t *root)
> >  
> >  		return parse_cb_table[i].cb(ctx, tmp, parse_cb_table[i].op);
> >  	}
> > -	/* to accept 'list ruleset' output 1:1, try add command */
> > -	return json_parse_cmd_add(ctx, root, CMD_ADD);
> > +	/* to accept 'list ruleset' output 1:1, try add command
> > +	 * Mark as implicit to distinguish from explicit add commands.
> > +	 * This allows explicit {"add": {"rule": ...}} to use handle for positioning
> > +	 * while implicit {"rule": ...} (export format) ignores handles.
> > +	 */
> > +	{
> > +		uint32_t old_flags = ctx->flags;
> > +		struct cmd *cmd;
> > +
> > +		ctx->flags |= CTX_F_IMPLICIT;
> > +		cmd = json_parse_cmd_add(ctx, root, CMD_ADD);
> > +		ctx->flags = old_flags;
> > +
> > +		return cmd;
> > +	}
> 
> This use of nested blocks is uncommon in this project. I suggest to
> either introduce a wrapper function or declare the two variables at the
> start of the function's body.

Right, as json_parse_cmd() is small I would go for the latter.

> > +# Verify all objects were created
> > +$NFT list ruleset > /dev/null || { echo "Failed to list ruleset after add operations"; exit 1; }
> 
> This command does not do what the comment says. To verify object
> creation, either use a series of 'nft list table/chain/set/...' commands
> or compare against a stored ruleset dump. See
> tests/shell/testcases/cache/0010_implicit_chain_0 for a simple example.

Yes, please use stored dump and let the test wrapper validate this if
possible.  I understand this can't work when the script has to validate
intermediate states too, but the last expected state canand should be
handled via dump.  More dumps also enhance fuzzer coverage since the
dumps are used for the initial input pool.

Also run "tools/check-tree.sh" and make sure it doesn't result in new
errors with the new test case.

If you like you can also split the actual patch and the test cases in
multiple patches, but thats up to you.