| Message ID | 20231031185449.1033380-3-thaller@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add and check dump files for JSON in tests/shell | expand |
Thomas Haller <thaller@redhat.com> wrote: > This message purely depends on the internal callbacks and at the program > code. This is not useful. What is the user going to do with this warning? Report a bug. > Maybe there is a bug here, but then we shouldn't print a warning but fix > the bug. How on earth do we do that, fix a bug before we know its there? > For example, calling `nft -j list ruleset` after test "tests/shell/testcases/chains/0041chain_binding_0" > will trigger messages like: > > warning: stmt ops chain have no json callback > warning: stmt ops chain have no json callback That means that chain ops needs a bug fix as they do not support json output?
On Thu, 2023-11-02 at 09:04 +0100, Florian Westphal wrote: > Thomas Haller <thaller@redhat.com> wrote: > > This message purely depends on the internal callbacks and at the > > program > > code. This is not useful. What is the user going to do with this > > warning? > > Report a bug. The way the message is worded ("warning:"), doesn't sound as if a user should feel invited to report a bug. > > Maybe there is a bug here, but then we shouldn't print a warning > > but fix > > the bug. > > How on earth do we do that, fix a bug before we know its there? Writing tests that perform basic consistency checks of things that must hold. A unit test should check whether ops structs are consistent. This patchset also enables a test that hits this problem (for "chain" ops). If there is a bug, then an assertion (or just a plain crash) would be preferable over printing a "warning" to stderr. > > > For example, calling `nft -j list ruleset` after test > > "tests/shell/testcases/chains/0041chain_binding_0" > > will trigger messages like: > > > > warning: stmt ops chain have no json callback > > warning: stmt ops chain have no json callback > > That means that chain ops needs a bug fix as they do not support > json output? I don't know. The message might just trying to be helpful, and there is no actual problem. The condition is attempted to be handled by the following code. I don't know which it is. Me sending the patch is basically asking that question. Thomas
On Thu, 2023-11-02 at 09:39 +0100, Thomas Haller wrote: > On Thu, 2023-11-02 at 09:04 +0100, Florian Westphal wrote: > > > I don't know which it is. Me sending the patch is basically asking > that > question. This patch 2/7 is obsoleted/replaced by [PATCH nft 1/2] json: implement json() hook for "symbol_expr_ops"/"variabl_expr_ops" [PATCH nft 2/2] json: drop handling missing json() hook for "struct expr_ops" Unit tests for consistency of "struct expr_ops" are still missing (I will send them, once some groundwork patches are in). Thomas
diff --git a/src/json.c b/src/json.c index c0ccf06d85b4..c66b58f8c6d5 100644 --- a/src/json.c +++ b/src/json.c @@ -52,9 +52,6 @@ static json_t *expr_print_json(const struct expr *expr, struct output_ctx *octx) 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"); @@ -95,9 +92,6 @@ 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); - fp = octx->output_fp; octx->output_fp = fmemopen(buf, 1024, "w");
This message purely depends on the internal callbacks and at the program code. This is not useful. What is the user going to do with this warning? Maybe there is a bug here, but then we shouldn't print a warning but fix the bug. For example, calling `nft -j list ruleset` after test "tests/shell/testcases/chains/0041chain_binding_0" will trigger messages like: warning: stmt ops chain have no json callback warning: stmt ops chain have no json callback Signed-off-by: Thomas Haller <thaller@redhat.com> --- src/json.c | 6 ------ 1 file changed, 6 deletions(-)