On Fri, 2023-11-03 at 22:47 +0100, Phil Sutter wrote:
> On Fri, Nov 03, 2023 at 05:25:14PM +0100, Thomas Haller wrote:
> > All "struct stmt_ops" really must have a json hook set, to handle
> > the
> > statement. And almost all of them do, except "struct
> > chain_stmt_ops".
> >
> > Soon a unit test will be added, to check that all stmt_ops have a
> > json()
> > hook. Also, the missing hook in "struct chain_stmt_ops" is a bug,
> > that
> > is now understood and shall be fixed soon/later.
> >
> > Note that we can already hit the bug, if we would call `nft -j list
> > ruleset` at the end of test "tests/shell/testcases/nft-f/sample-
> > ruleset":
> >
> > warning: stmt ops chain have no json callback
> >
> > Soon tests will be added, that hit this condition. Printing a
> > message to
> > stderr breaks those tests, and blocks adding the tests.
>
> Why not make the tests tolerate messages on stderr instead?
Right. That's what the v2 of the patchset does:
+ $NFT -j list ruleset > "$NFT_TEST_TESTTMPDIR/ruleset-after.json" 2> "$NFT_TEST_TESTTMPDIR/chkdump" || rc=$?
+
+ # Workaround known bug in stmt_print_json(), due to
+ # "chain_stmt_ops.json" being NULL. This spams stderr.
+ sed -i '/^warning: stmt ops chain have no json callback$/d' "$NFT_TEST_TESTTMPDIR/chkdump"
I'd prefer not to add the workaround at other places, but at what the
problem is. But both works!
>
> > Drop this warning on stderr, so we can add those other tests
> > sooner, as
> > those tests are useful for testing JSON code in general. The
> > warning
> > stderr was useful for finding the problem, but the problem is now
> > understood and will be addressed separately. Drop the message to
> > unblock
> > adding those tests.
>
> What do you mean with "the problem is now understood"?
I mean,
It's understood that "chain_stmt_ops.json" has this problem and needs
fixing. It should be planned for doing that (a bugzilla?).
Other potential future issues in this regard (accidentally forgetting
"json" hook in a chain_stmt_ops) will be prevented by a unit test and
more tests (automatically run `nft -j list ruleset`). Those tests are
on the way.
That makes the area of code handled ("understood").
>
> > Signed-off-by: Thomas Haller <thaller@xxxxxxxxxx>
> > ---
> > src/json.c | 10 ++++++++--
> > src/statement.c | 1 +
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/json.c b/src/json.c
> > index 25e349155394..8fff401dfb3e 100644
> > --- a/src/json.c
> > +++ b/src/json.c
> > @@ -83,8 +83,14 @@ static json_t *stmt_print_json(const struct stmt
> > *stmt, struct output_ctx *octx)
> > if (stmt->ops->json)
> > return stmt->ops->json(stmt, octx);
> >
> > - fprintf(stderr, "warning: stmt ops %s have no json
> > callback\n",
> > - stmt->ops->name);
>
> Converting this to using octx->error_fp does not help?
>
> > + /* In general, all "struct stmt_ops" must implement json()
> > hook. Otherwise
> > + * we have a bug, and a unit test should check that all
> > ops are correct.
> > + *
> > + * Currently, "chain_stmt_ops.json" is known to be NULL.
> > That is a bug that
> > + * needs fixing.
> > + *
> > + * After the bug is fixed, and the unit test in place,
> > this fallback code
> > + * can be dropped. */
>
> How will those unit tests cover new statements added at a later time?
> We
> don't have a registration process, are you planning to discover them
> based on enum stmt_types or something like that?
Good point! Some extra effort will be necessary to register/find the
stmt_ops.
I would propose
https://gitlab.freedesktop.org/thaller/nftables/-/commit/6ac04f812948ee6df49ad90a0507b62ed877ead7#118691ec02f9e8625350a91de8a6490b82a51928_262_285
which requires one extra line per ops.
The test checks that all stmt_types are found, so you almost cannot
forget the registration.
Thomas
