Hi Florian, On Fri, Oct 18, 2019 at 04:05:08PM +0200, Florian Westphal wrote: > Phil Sutter <phil@xxxxxx> wrote: > > Xtables-restore tries to reject rule commands in input which contain a > > --table parameter (since it is adding this itself based on the previous > > table line). Sadly getopt_long's flexibility makes it hard to get this > > check right: Since the last fix, comments starting with a dash and > > containing a 't' character somewhere later were rejected. Simple > > example: > > > > | *filter > > | -A FORWARD -m comment --comment "- allow this one" -j ACCEPT > > | COMMIT > > > > To hopefully sort this once and for all, introduce is_table_param() > > which should cover all possible variants of legal and illegal > > parameters. Also add a test to make sure it does what it is supposed to. > > Thanks for adding a test for this. > 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. ;) The problem with dropping that check is the potential mess we get when users add '-t' parameter to rules in dumps. While *tables-restore adds '-t' option for the current table itself, arg parsing in at least do_commandeb and do_commandx accepts multiple '-t' options (so the last one wins). Assuming that this checking for '-t' presence is a mess and we should get rid of it, I can imagine two alternatives: 1) Disallow multiple '-t' options. A nice and easy solution, but not backwards compatible. 2) Make *tables-restore add the '-t' option last. This is a bit of a hack and will cause unexpected behaviour for users trying to add '-t' option in dumps. What do you think? Or should I respin after adding a bunch of comments to is_table_param() to make it more clear? Thanks, Phil
