Re: [PATCH net-next iproute2 2/3] tc: Introduce tc ct action

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Thu, Jul 11, 2019 at 07:21:51AM +0000, Paul Blakey wrote:
> 
> On 7/9/2019 6:36 PM, Marcelo Ricardo Leitner wrote:
> > On Tue, Jul 09, 2019 at 06:58:36AM +0000, Paul Blakey wrote:
> >> On 7/8/2019 8:54 PM, Marcelo Ricardo Leitner wrote:
> >>> On Sun, Jul 07, 2019 at 11:53:47AM +0300, Paul Blakey wrote:
> >>>> New tc action to send packets to conntrack module, commit
> >>>> them, and set a zone, labels, mark, and nat on the connection.
> >>>>
> >>>> It can also clear the packet's conntrack state by using clear.
> >>>>
> >>>> Usage:
> >>>>      ct clear
> >>>>      ct commit [force] [zone] [mark] [label] [nat]
> >>> Isn't the 'commit' also optional? More like
> >>>       ct [commit [force]] [zone] [mark] [label] [nat]
> >>>
> >>>>      ct [nat] [zone]
> >>>>
> >>>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> >>>> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> >>>> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
> >>>> Acked-by: Jiri Pirko <jiri@mellanox.com>
> >>>> Acked-by: Roi Dayan <roid@mellanox.com>
> >>>> ---
> >>> ...
> >>>> +static void
> >>>> +usage(void)
> >>>> +{
> >>>> +	fprintf(stderr,
> >>>> +		"Usage: ct clear\n"
> >>>> +		"	ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n"
> >>> Ditto here then.
> >>
> >> In commit msg and here, it means there is multiple modes of operation. I
> >> think it's easier to split those.
> > Yep, that is good.
> > More below.
> >
> >> "ct clear" to clear it , not other options can be added here.
> >>
> >> "ct commit  [force].... " sends to conntrack and commit a connection,
> >> and only for commit can you specify force mark  label, and nat with
> >> nat_spec....
> >>
> >> and the last one, "ct [nat] [zone ZONE]" is to just send the packet to
> >> conntrack on some zone [optional], restore nat [optional].
> >>
> >>
> >>>> +		"	ct [nat] [zone ZONE]\n"
> >>>> +		"Where: ZONE is the conntrack zone table number\n"
> >>>> +		"	NAT_SPEC is {src|dst} addr addr1[-addr2] [port port1[-port2]]\n"
> >>>> +		"\n");
> >>>> +	exit(-1);
> >>>> +}
> >>> ...
> >>>
> >>> The validation below doesn't enforce that commit must be there for
> >>> such case.
> >> which case? commit is optional. the above are the three valid patterns.
> > That's the point. But the 2nd example is saying 'commit' word is
> > mandatory in that mode. It is written as it is a command that was
> > selected.
> >
> > One may use just:
> >      ct [zone]
> > And not
> >      ct commit [zone]
> > Right?
> 
> It is optional in the overall syntax.
> 
> 
> But I split it into modes:
> 
> clear, commit, and "restore" (I unofficial call it like that, because it 
> usually used to get the +est state on the packet and can restore nat, it 
> doesn't actually restore anything for the first packet on the -trk rule)
> 
> It is mandatory in the second mode (commit), if you don't specify commit 
> or clear, you can only use the third form - "restore", which is to send 
> to ct on some optional zone, and optionally and restore nat (so we get 
> ct [zone] [nat]).

I see. Thanks Paul.

  Marcelo