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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

Cheers, Phil

[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux