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
