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().
