Re: [nft v2 PATCH 1/3] nftables: add input flags for nft_ctx

[Phil Sutter <phil@nwl.cc>]

On Tue, Jul 18, 2023 at 11:05:45AM +0200, Thomas Haller wrote:
> On Fri, 2023-07-14 at 12:16 +0200, Phil Sutter wrote:
> > On Fri, Jul 14, 2023 at 10:48:51AM +0200, Thomas Haller wrote:
> > [...]
> > > +=== nft_ctx_input_get_flags() and nft_ctx_input_set_flags()
> > > +The flags setting controls the input format.
> > 
> > Note how we turn on JSON input parsing if JSON output is enabled.
> > 
> > I think we could tidy things up by introducing NFT_CTX_INPUT_JSON and
> > flip it from nft_ctx_output_set_flags() to match NFT_CTX_OUTPUT_JSON
> > for
> > compatibility?
> 
> The doc says:
> 
> doc/libnftables.adoc:NFT_CTX_OUTPUT_JSON::
> doc/libnftables.adoc-    If enabled at compile-time, libnftables accepts input in JSON format and is able to print output in JSON format as well.
> doc/libnftables.adoc-    See *libnftables-json*(5) for a description of the supported schema.
> doc/libnftables.adoc-    This flag controls JSON output format, input is auto-detected.
> 
> which is a bit inaccurate, as JSON is auto-detect if-and-only-if
> NFT_CTX_OUTPUT_JSON is set.

Yes, I'd even call it incorrect. :)

> src/libnftables.c:  if (nft_output_json(&nft->output))
> src/libnftables.c-       rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
> src/libnftables.c-  if (rc == -EINVAL)
> src/libnftables.c-       rc = nft_parse_bison_buffer(nft, nlbuf, &msgs, &cmds,
> src/libnftables.c-                          &indesc_cmdline);
> 
> 
> I think, that toggling the input flag when setting an output flag
> should not be done. It's confusing to behave differently depending on
> the order in which nft_ctx_output_set_flags() and
> nft_ctx_input_set_flags() are called. And it's confusing that setting
> output flags would mangle input flags.

That's a valid point, indeed.

> And for the sake of backwards compatibility, the current behavior must
> be kept anyway. So there is only a place for overruling the current
> automatism via some NFT_CTX_INPUT_NO_JSON (aka
> NFT_CTX_INOUT_FORCE_BISON) or NFT_CTX_INPUT_FORCE_JSON flags, like
> 
>     try_json = TRUE;
>     try_bison = TRUE;
>     if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_NO_JSON)
>         try_json = FALSE;
>     else if (nft_ctx_input_get_flags(ctx) & NFT_CTX_INPUT_FORCE_JSON)
>         try_bison = FALSE;
>     else if (nft_output_json(&ctx->output)) {
>         /* try both, JSON first */
>     } else
>         try_json = FALSE;
> 
>     if (try_json)
>         rc = nft_parse_json_buffer(nft, nlbuf, &msgs, &cmds);
>     if (try_bison && (!try_json || rc == -EINVAL))
>         rc = nft_parse_bison_buffer(nft, nlbuf, ...);
> 
> 
> This would not require to mangle input flags during
> nft_ctx_output_set_flags().
> 
>  
> But I find those two flags unnecessary. Both can be added any time
> later when needed. The addition of nft_ctx_input_set_flags() does not
> force a resolution now.
> 
> 
> Also, depending on the semantics, I don't understand how
> NFT_CTX_INPUT_JSON extends the current behavior in a backward
> compatible way. The default would be to not have that flag set, which
> already means to enable JSON parsing depending on NFT_CTX_OUTPUT_JSON.
> If NFT_CTX_INPUT_JSON merely means to explicitly enable JSON input,
> then that is already fully configurable today. Having this flag does
> not provide something new (unlike NO_JSON/FORCE_BISON or FORCE_JSON
> flags would).

The use-case I had in mind was to enable JSON parsing while keeping
standard output. This was possible with setting NFT_CTX_INPUT_JSON and
keeping NFT_CTX_OUTPUT_JSON unset.

The reason for libnftables' odd behaviour probably stems from nft using
just a single flag ('-j') to control JSON "mode" and I wanted to still
support non-JSON input - I tend to (mis-)use it as JSON-translator in
the testsuite and personally. ;)

You're right, we may just leave JSON input/output toggles alone for now.
I also didn't intend to block the patches - giving it a thought (as you
did) is fine from my side.

Thanks, Phil