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

[Thomas Haller <thaller@redhat.com>]

On Tue, 2023-07-18 at 11:33 +0200, Phil Sutter wrote:
> 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.

you are right. A NFT_CTX_INPUT_JSON (or TRY_JSON?) flag makes sense
beside NO_JSON/FORCE_BISON and FORCE_JSON/ALWAYS_JSON to enable to try
both.

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


makes sense. But doesn't block the addition of
nft_ctx_input_set_flags(), because I would not try to automatically add
the NFT_CTX_INPUT_JSON flag (based on the output flag). Setting the
input flag should still be an explicit user action, so the flag can be
added any time later.


Thomas