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