| Message ID | 20231103162937.3352069-3-thaller@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drop warning messages from stmt_print_json()/expr_print_json() | expand |
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.
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.
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
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
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
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
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, };
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(-)