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