[PATCH nft v3 0/2] drop warning messages from stmt_print_json()/expr_print_json()

[PATCH nft v3 0/2] drop warning messages from stmt_print_json()/expr_print_json()

[Thomas Haller]

The second patch "json: drop warning on stderr for missing json() hook
in stmt_print_json()" is important to add .json-nft tests from

  Subject:    [PATCH nft 0/7] add and check dump files for JSON in tests/shell
  Date:   Tue, 31 Oct 2023 19:53:26 +0100

(while fixing the bug in stmt_print_json()/chain_stmt_ops.json might take longer).

This replaces v1:

  Subject:  [PATCH nft 2/7] json: drop messages "warning: stmt ops chain have no json callback"
  Date: Tue, 31 Oct 2023 19:53:28 +0100

and v2:

  Subject:  [PATCH nft 1/2] json: implement json() hook for "symbol_expr_ops"/"variabl_expr_ops"
  Subject:  [PATCH nft 2/2] json: drop handling missing json() hook for "struct expr_ops"
  Date:   Thu,  2 Nov 2023 12:20:28 +0100

Thomas Haller (2):
  json: drop handling missing json() hook in expr_print_json()
  json: drop warning on stderr for missing json() hook in
    stmt_print_json()

 src/expression.c |  2 ++
 src/json.c       | 38 ++++++++++++++++----------------------
 src/statement.c  |  1 +
 3 files changed, 19 insertions(+), 22 deletions(-)

-- 
2.41.0

[PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Thomas Haller]

There are only two "struct expr_ops" that don't have a json() hook set
("symbol_expr_ops", "variable_exrp_ops"). For those, we never expect to
call expr_print_json().

All other "struct expr_ops" must have a hook set. Soon a unit test shall
be added to also ensure that for all expr_ops (except EXPR_SYMBOL and
EXPR_VARIABLE).

It thus should not be possible to ever call expr_print_json() with a
NULL hook. Drop the code that tries to handle that.

The previous code was essentially a graceful assertion, which only
prints a message to stderr (instead of assert()/abort()) and implemented
a fallback solution. The fallback solution is not really useful, because
it's just the bison string which cannot be parsed back.

This seems too much effort trying to handle a potential bug, for
something that we most likely will not be wrong (once the unit test is
in place). Drop the fallback solution and just require the bug not to be
present. We now get a hard crash if the requirement is violated.

Also add code comments hinting to all of this.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/expression.c |  2 ++
 src/json.c       | 28 ++++++++--------------------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/src/expression.c b/src/expression.c
index a21dfec25722..53fa630e099c 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -321,6 +321,7 @@ static const struct expr_ops symbol_expr_ops = {
 	.type		= EXPR_SYMBOL,
 	.name		= "symbol",
 	.print		= symbol_expr_print,
+	.json		= NULL, /* expr_print_json() must never be called. */
 	.clone		= symbol_expr_clone,
 	.destroy	= symbol_expr_destroy,
 };
@@ -362,6 +363,7 @@ static const struct expr_ops variable_expr_ops = {
 	.type		= EXPR_VARIABLE,
 	.name		= "variable",
 	.print		= variable_expr_print,
+	.json		= NULL, /* expr_print_json() must never be called. */
 	.clone		= variable_expr_clone,
 	.destroy	= variable_expr_destroy,
 };
diff --git a/src/json.c b/src/json.c
index 068c423addc7..25e349155394 100644
--- a/src/json.c
+++ b/src/json.c
@@ -44,26 +44,14 @@
 
 static json_t *expr_print_json(const struct expr *expr, struct output_ctx *octx)
 {
-	const struct expr_ops *ops;
-	char buf[1024];
-	FILE *fp;
-
-	ops = expr_ops(expr);
-	if (ops->json)
-		return ops->json(expr, octx);
-
-	fprintf(stderr, "warning: expr ops %s have no json callback\n",
-		expr_name(expr));
-
-	fp = octx->output_fp;
-	octx->output_fp = fmemopen(buf, 1024, "w");
-
-	ops->print(expr, octx);
-
-	fclose(octx->output_fp);
-	octx->output_fp = fp;
-
-	return json_pack("s", buf);
+	/* The json() hooks of "symbol_expr_ops" and "variable_expr_ops" are
+	 * known to be NULL, but for such expressions we never expect to call
+	 * expr_print_json().
+	 *
+	 * All other expr_ops must have a json() hook.
+	 *
+	 * Unconditionally access the hook (and segfault in case of a bug).  */
+	return expr_ops(expr)->json(expr, octx);
 }
 
 static json_t *set_dtype_json(const struct expr *key)
-- 
2.41.0

[PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Thomas Haller]

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.

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.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 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);
+	/* 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. */
 
 	fp = octx->output_fp;
 	octx->output_fp = fmemopen(buf, 1024, "w");
diff --git a/src/statement.c b/src/statement.c
index f5176e6d87f9..d52b01b9099a 100644
--- a/src/statement.c
+++ b/src/statement.c
@@ -141,6 +141,7 @@ static const struct stmt_ops chain_stmt_ops = {
 	.type		= STMT_CHAIN,
 	.name		= "chain",
 	.print		= chain_stmt_print,
+	.json		= NULL, /* BUG: must be implemented! */
 	.destroy	= chain_stmt_destroy,
 };
 
-- 
2.41.0

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Pablo Neira Ayuso]

On Fri, Nov 03, 2023 at 05:25:14PM +0100, Thomas Haller wrote:
> diff --git a/src/statement.c b/src/statement.c
> index f5176e6d87f9..d52b01b9099a 100644
> --- a/src/statement.c
> +++ b/src/statement.c
> @@ -141,6 +141,7 @@ static const struct stmt_ops chain_stmt_ops = {
>  	.type		= STMT_CHAIN,
>  	.name		= "chain",
>  	.print		= chain_stmt_print,
> +	.json		= NULL, /* BUG: must be implemented! */

This is a bit starting the house from the roof.

Better fix this first, so this ugly patch does not need to be applied.

Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -321,6 +321,7 @@ static const struct expr_ops symbol_expr_ops = {
>  	.type		= EXPR_SYMBOL,
>  	.name		= "symbol",
>  	.print		= symbol_expr_print,
> +	.json		= NULL, /* expr_print_json() must never be called. */

I'd suggest to add a json callback that BUG()s instead,
with a comment explaining that these do not exist anymore
after the initial eval stage.  (symbols will be resolved
to numeric value for example).

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Nov 03, 2023 at 05:25:14PM +0100, Thomas Haller wrote:
> > diff --git a/src/statement.c b/src/statement.c
> > index f5176e6d87f9..d52b01b9099a 100644
> > --- a/src/statement.c
> > +++ b/src/statement.c
> > @@ -141,6 +141,7 @@ static const struct stmt_ops chain_stmt_ops = {
> >  	.type		= STMT_CHAIN,
> >  	.name		= "chain",
> >  	.print		= chain_stmt_print,
> > +	.json		= NULL, /* BUG: must be implemented! */
> 
> This is a bit starting the house from the roof.
> 
> Better fix this first, so this ugly patch does not need to be applied.

Agreed, I would keep the fprintf and all the fallback print code.
We can remove this AFTER expternal means (unit test f.e.) ensure all the
stmt/expr_ops have the needed callbacks.

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Thomas Haller]

On Fri, 2023-11-03 at 17:46 +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 03, 2023 at 05:25:14PM +0100, Thomas Haller wrote:
> > diff --git a/src/statement.c b/src/statement.c
> > index f5176e6d87f9..d52b01b9099a 100644
> > --- a/src/statement.c
> > +++ b/src/statement.c
> > @@ -141,6 +141,7 @@ static const struct stmt_ops chain_stmt_ops = {
> >  	.type		= STMT_CHAIN,
> >  	.name		= "chain",
> >  	.print		= chain_stmt_print,
> > +	.json		= NULL, /* BUG: must be implemented! */
> 
> This is a bit starting the house from the roof.
> 
> Better fix this first, so this ugly patch does not need to be
> applied.
> 

that is going to take a while.

The patches to enable more JSON tests find other issues (and are
ready). Also, implementing a not-entirely-trivial feature
(chain_stmt_ops.json) without the JSON tests in place, is unnecessarily
backwards.

If you dislike the code comments, please drop them. But this
`fprintf()` should go, as spams stderr and [1] checks against that.
That check is useful to find bugs.

[1] https://marc.info/?l=netfilter-devel&m=169877835315739&w=2


Thomas

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Thomas Haller]

On Fri, 2023-11-03 at 17:59 +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Nov 03, 2023 at 05:25:14PM +0100, Thomas Haller wrote:
> > > diff --git a/src/statement.c b/src/statement.c
> > > index f5176e6d87f9..d52b01b9099a 100644
> > > --- a/src/statement.c
> > > +++ b/src/statement.c
> > > @@ -141,6 +141,7 @@ static const struct stmt_ops chain_stmt_ops =
> > > {
> > >  	.type		= STMT_CHAIN,
> > >  	.name		= "chain",
> > >  	.print		= chain_stmt_print,
> > > +	.json		= NULL, /* BUG: must be implemented! */
> > 
> > This is a bit starting the house from the roof.
> > 
> > Better fix this first, so this ugly patch does not need to be
> > applied.
> 
> Agreed, I would keep the fprintf and all the fallback print code.
> We can remove this AFTER expternal means (unit test f.e.) ensure all
> the
> stmt/expr_ops have the needed callbacks.
> 


ACK. Then let's drop these two patches.
I'll add the workaround to the tests instead.

Please don't replace the fprintf() with a BUG(), because that's harder
to workaround.


Thomas

Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Phil Sutter]

On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote:
[...]
> +	/* The json() hooks of "symbol_expr_ops" and "variable_expr_ops" are
> +	 * known to be NULL, but for such expressions we never expect to call
> +	 * expr_print_json().
> +	 *
> +	 * All other expr_ops must have a json() hook.
> +	 *
> +	 * Unconditionally access the hook (and segfault in case of a bug).  */
> +	return expr_ops(expr)->json(expr, octx);

This does not make sense to me. You're deliberately dropping any error
handling and accept a segfault because "it should never happen"? All it
takes is someone to add a new expression type and forget about the JSON
API.

If you absolutely have to remove that fallback code, at least add a
BUG() explaining the situation. The sysadmin looking at the segfault
report in syslog won't see your comment above.

Cheers, Phil

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Phil Sutter]

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?

> 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"?

> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  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?

Cheers, Phil

Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Thomas Haller]

On Fri, 2023-11-03 at 22:37 +0100, Phil Sutter wrote:
> On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote:
> [...]
> > +	/* The json() hooks of "symbol_expr_ops" and
> > "variable_expr_ops" are
> > +	 * known to be NULL, but for such expressions we never
> > expect to call
> > +	 * expr_print_json().
> > +	 *
> > +	 * All other expr_ops must have a json() hook.
> > +	 *
> > +	 * Unconditionally access the hook (and segfault in case
> > of a bug).  */
> > +	return expr_ops(expr)->json(expr, octx);
> 
> This does not make sense to me. You're deliberately dropping any
> error
> handling

Error handling for what is clearly a bug. Don't try to handle bugs.
Avoid bugs and fix them.

> and accept a segfault because "it should never happen"? All it
> takes is someone to add a new expression type and forget about the
> JSON
> API.

There will be a unit test guarding against that, once the unit test
basics are done.

Also, if you "forget" to implement the JSON hook and test it (manually)
only a single time, then you'll notice right away.


> 
> If you absolutely have to remove that fallback code, at least add a
> BUG() explaining the situation. The sysadmin looking at the segfault
> report in syslog won't see your comment above.

I am in favor of adding assertions all over the place. The project
doesn't use enough asserions for my taste.

In this case, it seems hard to mess up the condition, and you get a
very clear signal when you do (segfault). That makes the assert()/BUG()
kinda unecessary.


But sure. Assert()/BUG() works too...



Thomas

Re: [PATCH nft v3 2/2] json: drop warning on stderr for missing json() hook in stmt_print_json()

[Thomas Haller]

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@redhat.com>
> > ---
> >  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

Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Phil Sutter]

On Sat, Nov 04, 2023 at 06:28:30AM +0100, Thomas Haller wrote:
> On Fri, 2023-11-03 at 22:37 +0100, Phil Sutter wrote:
> > On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote:
> > [...]
> > > +	/* The json() hooks of "symbol_expr_ops" and
> > > "variable_expr_ops" are
> > > +	 * known to be NULL, but for such expressions we never
> > > expect to call
> > > +	 * expr_print_json().
> > > +	 *
> > > +	 * All other expr_ops must have a json() hook.
> > > +	 *
> > > +	 * Unconditionally access the hook (and segfault in case
> > > of a bug).  */
> > > +	return expr_ops(expr)->json(expr, octx);
> > 
> > This does not make sense to me. You're deliberately dropping any
> > error
> > handling
> 
> Error handling for what is clearly a bug. Don't try to handle bugs.
> Avoid bugs and fix them.

Yeah, indeed. Let's go ahead and drop all BUG() statements as well.
Seriously, I doubt nftables users agree the software should segfault
instead of aborting with an error message.

> > and accept a segfault because "it should never happen"? All it
> > takes is someone to add a new expression type and forget about the
> > JSON
> > API.
> 
> There will be a unit test guarding against that, once the unit test
> basics are done.
> 
> Also, if you "forget" to implement the JSON hook and test it (manually)
> only a single time, then you'll notice right away.

Actually, all it takes to notice things don't add up is running the py
testsuite with '-j' arg after adding the obligatory "unit" tests there.
Feel free to search the git history for late additions of .json
callbacks. I think the message is pretty clear.

> > If you absolutely have to remove that fallback code, at least add a
> > BUG() explaining the situation. The sysadmin looking at the segfault
> > report in syslog won't see your comment above.
> 
> I am in favor of adding assertions all over the place. The project
> doesn't use enough asserions for my taste.
> 
> In this case, it seems hard to mess up the condition, and you get a
> very clear signal when you do (segfault). That makes the assert()/BUG()
> kinda unecessary.

The clear signal being "oops, my program crashed" when it could be a
dubious "oops, there is no JSON callback for this expression type".

Cheers, Phil

Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Pablo Neira Ayuso]

On Sun, Nov 05, 2023 at 11:40:22AM +0100, Phil Sutter wrote:
> On Sat, Nov 04, 2023 at 06:28:30AM +0100, Thomas Haller wrote:
> > On Fri, 2023-11-03 at 22:37 +0100, Phil Sutter wrote:
> > > On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote:
> > > [...]
> > > > +	/* The json() hooks of "symbol_expr_ops" and
> > > > "variable_expr_ops" are
> > > > +	 * known to be NULL, but for such expressions we never
> > > > expect to call
> > > > +	 * expr_print_json().
> > > > +	 *
> > > > +	 * All other expr_ops must have a json() hook.
> > > > +	 *
> > > > +	 * Unconditionally access the hook (and segfault in case
> > > > of a bug).  */
> > > > +	return expr_ops(expr)->json(expr, octx);
> > > 
> > > This does not make sense to me. You're deliberately dropping any
> > > error
> > > handling
> > 
> > Error handling for what is clearly a bug. Don't try to handle bugs.
> > Avoid bugs and fix them.
> 
> Yeah, indeed. Let's go ahead and drop all BUG() statements as well.
> Seriously, I doubt nftables users agree the software should segfault
> instead of aborting with an error message.

BUG() assertion is better than crash.

> > > and accept a segfault because "it should never happen"? All it
> > > takes is someone to add a new expression type and forget about the
> > > JSON
> > > API.
> > 
> > There will be a unit test guarding against that, once the unit test
> > basics are done.
> > 
> > Also, if you "forget" to implement the JSON hook and test it (manually)
> > only a single time, then you'll notice right away.
> 
> Actually, all it takes to notice things don't add up is running the py
> testsuite with '-j' arg after adding the obligatory "unit" tests there.
> Feel free to search the git history for late additions of .json
> callbacks. I think the message is pretty clear.
> 
> > > If you absolutely have to remove that fallback code, at least add a
> > > BUG() explaining the situation. The sysadmin looking at the segfault
> > > report in syslog won't see your comment above.
> > 
> > I am in favor of adding assertions all over the place. The project
> > doesn't use enough asserions for my taste.
> > 
> > In this case, it seems hard to mess up the condition, and you get a
> > very clear signal when you do (segfault). That makes the assert()/BUG()
> > kinda unecessary.
> 
> The clear signal being "oops, my program crashed" when it could be a
> dubious "oops, there is no JSON callback for this expression type".

This should be turned into BUG().