On Thu, Feb 07, 2019 at 09:24:09PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Feb 07, 2019 at 09:00:50PM +0100, Phil Sutter wrote:
> > On Thu, Feb 07, 2019 at 07:29:10PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Feb 07, 2019 at 05:48:53PM +0100, Florian Westphal wrote:
> > > > Phil Sutter <phil@xxxxxx> wrote:
> > > > > > What it should show instead:
> > > > > > Bridge chain: FOOBAR, entries: 1, policy: RETURN
> > > > > > -j DROP
> > > > > > -d 01:02:03:04:05:06 -j CONTINUE
> > > > > >
> > > > > > (because thats whats the actual state -- the last rule is unreachable).
> > > > >
> > > > > Hmm. Yes, that's ugly. Also, if you perform a change to the ruleset in
> > > > > that state (no matter what, e.g. just create another chain or add a rule
> > > > > somewhere else), the policy rule will be moved to the end. Not sure how
> > > > > we could handle this.
> > > >
> > > > I think in that case ebtables-nft should make policy be 'RETURN', i.e.,
> > > > not re-add a new policy chain.
> > > >
> > > > I think we could even avoid the 'user comment' and just examine the last
> > > > rule in the chain -- check if its unconditional DROP/ACCEPT and then
> > > > handle that as the 'policy'.
> > >
> > > We can probably add UDATA_TYPE_EBTABLES_POLICY, so we don't need to
> > > guess if this is an autogenerated policy rule at the end of non-base
> > > chain. Just search for the rule with this flag. For nft this
> > > autogenerated rule will be transparent.
> >
> > I just tried. There is a minor issue with
> > nft_rule_to_iptables_command_state(): It assumes that if
> > NFTNL_RULE_USERDATA is present, it is a comment. If I either drop that
> > code (it is there only for backwards compatibility) or make it aware
> > that NFTNL_RULE_USERDATA not necessarily contains a comment, things work
> > fine.
> >
> > What do you think? Should I send a v3?
>
> You could rename get_comment() to get_userdata() which returns an
> ipt_userdata structure that looks like:
>
> struct ipt_userdata {
> const char *comment;
> bool is_ebtables_policy;
> };
>
> that you can pass to get_userdata() to fetch things. You can probably
> store this userdata in the command_state structure, so we don't need
> to parse userdata over and over again from different spots.
>
> But this is just a suggestion.
It is a nice idea, but probably not worth it, userdata is parsed in just
two spots. When populating the iptables_command_state, a "comment" match
is allocated to hold the data. When used for ebtables policy rule, code
operates on struct nftnl_rule directly, so no point in keeping things in
iptables_command_state, either.
I'll send a v3 making use of a custom userdata attribute.
Thanks, Phil
