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

[Thomas Haller <thaller@redhat.com>]

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?


Hi Phil,


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.


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.

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).



Thomas