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

[Phil Sutter <phil@nwl.cc>]

Alexandre, will you follow-up on this one? Feel free to ping me if
something is unclear or you're stuck somewhere - we're there to help if
needed!

Thanks, Phil

On Fri, Nov 14, 2025 at 02:36:43PM +0100, Florian Westphal wrote:
> 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.
> 
>