Hi, On Fri, Oct 18, 2019 at 10:58:08PM +0200, Florian Westphal wrote: > Phil Sutter <phil@xxxxxx> wrote: > > > How did you generate it? The added code is pure voodoo magic to me, > > > so I wonder if we can just remove the 'test for -t in iptables-restore > > > files' code. > > > > Sorry, I didn't mean to create such unreadable code. I guess after > > managing to wrap my head around to understand the old code, the new one > > seemed much more clear to me. ;) > > Fair enough, my main point was where the test cases come from, i.e. > did you see such rule dumps in the wild, or did you create this manually > to catch all corner cases? > > I see you have a test for things like "-?t", so I wondered where that > came from. Ah! Originally this comes from a Red Hat BZ, not sure what reporter actually tested with but as long as the comment started with a dash and contained a 't' somewhere it would trigger the bug. I wrote the test case along with the new implementation, searching for things that could be mismatched. The '-?t' for instance is to make sure combined short-options are matched correctly: Since '?' is not a valid short option (at least not in iptables), this must not match as '-t' option. > > What do you think? Or should I respin after adding a bunch of comments > > to is_table_param() to make it more clear? > > I think thats the best option, I don't have any objections at the check > per se given older iptables does this too. I don't quite like this check, hence I don't overly cling to it. As you see, checking for presence of an option in getopt() format is not easy and we do that for every option of every rule in a dump. Maybe we should really just append the explicit table param and accept that user's table option is not rejected but simply ignored. Cheers, Phil
