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
