From: Phil Sutter <phil@nwl.cc>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [iptables PATCH v2 00/17] Eliminate dedicated arptables-nft parser
Date: Fri, 15 Oct 2021 13:01:24 +0200 [thread overview]
Message-ID: <20211015110124.GL1668@orbyte.nwl.cc> (raw)
In-Reply-To: <YWiZacKr4s3mkdhU@salvia>
Hi Pablo,
On Thu, Oct 14, 2021 at 10:56:09PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 30, 2021 at 04:04:02PM +0200, Phil Sutter wrote:
> > Commandline parsing was widely identical with iptables and ip6tables.
> > This series adds the necessary code-changes to unify the parsers into a
> > common one.
> >
> > Changes since v1:
> > - Fix patch 12, the parser has to check existence of proto_parse
> > callback before dereferencing it. Otherwise arptables-nft segfaults if
> > '-p' option is given.
>
> LGTM.
Thanks for your review!
> > - Patches 13-17 add all the arptables quirks to restore compatibility
> > with arptables-legacy. I didn't consider them important enough to push
> > them unless someone complains. Yet breaking existing scripts is bad
> > indeed. Please consider them RFC: If you consider (one of) them not
> > important, please NACk and I will drop them before pushing.
>
> For patch 13-16, you could display a warning for people to fix their
> scripts, so this particular (strange) behaviour in some cases can be
> dropped (at least, 13-15 look like left-over/bugs). For the
> check_inverse logic, I'd suggest to display a warning too, this is
> what it was done in iptables time ago to address this inconsistency.
I wonder how likely it is for someone to rely upon the behaviour. I can
imagine a script passing an empty interface name and expecting the
argument to be ignored (patch 13). Though what are the odds someone
actually calls arptables with '-m something' (patch 14) or a bogus table
name (patch 15)?
> I'd probably keep back patch 17/17, the max chain name length was
> reduced by when the revision field was introduced and this resulted in
> no issue being reported.
If that's OK with you, I would turn the empty interface name error into
a warning for arptables-nft, reintroduce the warning for intrapositioned
negations and drop the remaining quirks as they are likely hiding a bug.
Cheers, Phil
next prev parent reply other threads:[~2021-10-15 11:01 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 14:04 [iptables PATCH v2 00/17] Eliminate dedicated arptables-nft parser Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 01/17] nft: Introduce builtin_tables_lookup() Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 02/17] xshared: Store optstring in xtables_globals Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 03/17] nft-shared: Introduce init_cs family ops callback Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 04/17] xtables: Simplify addr_mask freeing Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 05/17] nft: Add family ops callbacks wrapping different nft_cmd_* functions Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 06/17] xtables-standalone: Drop version number from init errors Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 07/17] libxtables: Introduce xtables_globals print_help callback Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 08/17] arptables: Use standard data structures when parsing Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 09/17] nft-arp: Introduce post_parse callback Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 10/17] nft-shared: Make nft_check_xt_legacy() family agnostic Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 11/17] xtables: Derive xtables_globals from family Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 12/17] nft: Merge xtables-arp-standalone.c into xtables-standalone.c Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 13/17] xtables: arptables doesn't warn about empty interface Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 14/17] xtables: arptables accepts but ignores '-m' Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 15/17] xtables: arptables ignores wrong -t values Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 16/17] xtables: Support '!' betwen option and argument Phil Sutter
2021-09-30 14:04 ` [iptables PATCH v2 17/17] nft: Store maximum allowed chain name length in family ops Phil Sutter
2021-10-14 20:56 ` [iptables PATCH v2 00/17] Eliminate dedicated arptables-nft parser Pablo Neira Ayuso
2021-10-15 11:01 ` Phil Sutter [this message]
2021-10-15 11:25 ` Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211015110124.GL1668@orbyte.nwl.cc \
--to=phil@nwl.cc \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.