| Message ID | 20221116073312.177786-2-roid@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Stephen Hemminger |
| Headers | show |
| Series | two fixes for tc ct command | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, 16 Nov 2022 09:33:11 +0200 Roi Dayan <roid@nvidia.com> wrote: > Action ct commit should accept nat src/dst without an addr. Fix it. > > Fixes: c8a494314c40 ("tc: Introduce tc ct action") > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@nvidia.com> > --- > man/man8/tc-ct.8 | 2 +- > tc/m_ct.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 > index 2fb81ca29aa4..78d05e430c36 100644 > --- a/man/man8/tc-ct.8 > +++ b/man/man8/tc-ct.8 > @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). > Specify a masked 128bit label to set for the connection (only valid with commit). > .TP > .BI nat " NAT_SPEC" > -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" > +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" > > Specify src/dst and range of nat to configure for the connection (only valid with commit). > .RS > diff --git a/tc/m_ct.c b/tc/m_ct.c > index a02bf0cc1655..1b8984075a67 100644 > --- a/tc/m_ct.c > +++ b/tc/m_ct.c > @@ -23,7 +23,7 @@ usage(void) > " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" > " 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" > + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" > "\n"); > exit(-1); > } > @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, > > NEXT_ARG(); > if (matches(*argv, "addr") != 0) > - usage(); > + continue; > This confuses me. Doing continue here will cause the current argument to be reprocessed so it would expect it to be zone | nat | clear | commit | force | index | mark | label which is not right.
On 16/11/2022 20:21, Stephen Hemminger wrote: > On Wed, 16 Nov 2022 09:33:11 +0200 > Roi Dayan <roid@nvidia.com> wrote: > >> Action ct commit should accept nat src/dst without an addr. Fix it. >> >> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> Reviewed-by: Paul Blakey <paulb@nvidia.com> >> --- >> man/man8/tc-ct.8 | 2 +- >> tc/m_ct.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >> index 2fb81ca29aa4..78d05e430c36 100644 >> --- a/man/man8/tc-ct.8 >> +++ b/man/man8/tc-ct.8 >> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >> Specify a masked 128bit label to set for the connection (only valid with commit). >> .TP >> .BI nat " NAT_SPEC" >> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >> >> Specify src/dst and range of nat to configure for the connection (only valid with commit). >> .RS >> diff --git a/tc/m_ct.c b/tc/m_ct.c >> index a02bf0cc1655..1b8984075a67 100644 >> --- a/tc/m_ct.c >> +++ b/tc/m_ct.c >> @@ -23,7 +23,7 @@ usage(void) >> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >> " 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" >> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >> "\n"); >> exit(-1); >> } >> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >> >> NEXT_ARG(); >> if (matches(*argv, "addr") != 0) >> - usage(); >> + continue; >> > > This confuses me. Doing continue here will cause the current argument to be reprocessed so > it would expect it to be zone | nat | clear | commit | force | index | mark | label > which is not right. > > its the opposite. "nat" came first. if matches() didn't find "addr" it continues the loop of args. if matches did find "addr" it continues to next line which is ct_parse_nat_addr_range() to parse the address.
On 17/11/2022 7:35, Roi Dayan wrote: > > > On 16/11/2022 20:21, Stephen Hemminger wrote: >> On Wed, 16 Nov 2022 09:33:11 +0200 >> Roi Dayan <roid@nvidia.com> wrote: >> >>> Action ct commit should accept nat src/dst without an addr. Fix it. >>> >>> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>> Reviewed-by: Paul Blakey <paulb@nvidia.com> >>> --- >>> man/man8/tc-ct.8 | 2 +- >>> tc/m_ct.c | 4 ++-- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >>> index 2fb81ca29aa4..78d05e430c36 100644 >>> --- a/man/man8/tc-ct.8 >>> +++ b/man/man8/tc-ct.8 >>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >>> Specify a masked 128bit label to set for the connection (only valid with commit). >>> .TP >>> .BI nat " NAT_SPEC" >>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >>> >>> Specify src/dst and range of nat to configure for the connection (only valid with commit). >>> .RS >>> diff --git a/tc/m_ct.c b/tc/m_ct.c >>> index a02bf0cc1655..1b8984075a67 100644 >>> --- a/tc/m_ct.c >>> +++ b/tc/m_ct.c >>> @@ -23,7 +23,7 @@ usage(void) >>> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >>> " 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" >>> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >>> "\n"); >>> exit(-1); >>> } >>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >>> >>> NEXT_ARG(); >>> if (matches(*argv, "addr") != 0) >>> - usage(); >>> + continue; >>> >> >> This confuses me. Doing continue here will cause the current argument to be reprocessed so >> it would expect it to be zone | nat | clear | commit | force | index | mark | label >> which is not right. >> >> > > its the opposite. "nat" came first. if matches() didn't find "addr" > it continues the loop of args. if matches did find "addr" it continues > to next line which is ct_parse_nat_addr_range() to parse the address. > > Got your comment wrong so yes the current arg will be reprocessed and this is what we want. This will make "addr" optional and there should be some action after ct commit nat. next loop iteration should break and continue parse next action usually a goto action.
On 17/11/2022 16:00, Roi Dayan wrote: > > > On 17/11/2022 7:35, Roi Dayan wrote: >> >> >> On 16/11/2022 20:21, Stephen Hemminger wrote: >>> On Wed, 16 Nov 2022 09:33:11 +0200 >>> Roi Dayan <roid@nvidia.com> wrote: >>> >>>> Action ct commit should accept nat src/dst without an addr. Fix it. >>>> >>>> Fixes: c8a494314c40 ("tc: Introduce tc ct action") >>>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>>> Reviewed-by: Paul Blakey <paulb@nvidia.com> >>>> --- >>>> man/man8/tc-ct.8 | 2 +- >>>> tc/m_ct.c | 4 ++-- >>>> 2 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 >>>> index 2fb81ca29aa4..78d05e430c36 100644 >>>> --- a/man/man8/tc-ct.8 >>>> +++ b/man/man8/tc-ct.8 >>>> @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). >>>> Specify a masked 128bit label to set for the connection (only valid with commit). >>>> .TP >>>> .BI nat " NAT_SPEC" >>>> -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" >>>> +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" >>>> >>>> Specify src/dst and range of nat to configure for the connection (only valid with commit). >>>> .RS >>>> diff --git a/tc/m_ct.c b/tc/m_ct.c >>>> index a02bf0cc1655..1b8984075a67 100644 >>>> --- a/tc/m_ct.c >>>> +++ b/tc/m_ct.c >>>> @@ -23,7 +23,7 @@ usage(void) >>>> " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" >>>> " 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" >>>> + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" >>>> "\n"); >>>> exit(-1); >>>> } >>>> @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, >>>> >>>> NEXT_ARG(); >>>> if (matches(*argv, "addr") != 0) >>>> - usage(); >>>> + continue; >>>> >>> >>> This confuses me. Doing continue here will cause the current argument to be reprocessed so >>> it would expect it to be zone | nat | clear | commit | force | index | mark | label >>> which is not right. >>> >>> >> >> its the opposite. "nat" came first. if matches() didn't find "addr" >> it continues the loop of args. if matches did find "addr" it continues >> to next line which is ct_parse_nat_addr_range() to parse the address. >> >> > > Got your comment wrong so yes the current arg will be reprocessed > and this is what we want. > This will make "addr" optional and there should be some action > after ct commit nat. next loop iteration should break and > continue parse next action usually a goto action. > Hi Stephen, Can you look at this again please? this is the same as done in other args like "port" right after and probably in other actions. If "addr" is not the next arg we continue the loop and parse again as expected for the other ct args or getting to else and breaking to continue. Thanks, Roi
diff --git a/man/man8/tc-ct.8 b/man/man8/tc-ct.8 index 2fb81ca29aa4..78d05e430c36 100644 --- a/man/man8/tc-ct.8 +++ b/man/man8/tc-ct.8 @@ -47,7 +47,7 @@ Specify a masked 32bit mark to set for the connection (only valid with commit). Specify a masked 128bit label to set for the connection (only valid with commit). .TP .BI nat " NAT_SPEC" -.BI Where " NAT_SPEC " ":= {src|dst} addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]" +.BI Where " NAT_SPEC " ":= {src|dst} [addr" " addr1" "[-" "addr2" "] [port " "port1" "[-" "port2" "]]]" Specify src/dst and range of nat to configure for the connection (only valid with commit). .RS diff --git a/tc/m_ct.c b/tc/m_ct.c index a02bf0cc1655..1b8984075a67 100644 --- a/tc/m_ct.c +++ b/tc/m_ct.c @@ -23,7 +23,7 @@ usage(void) " ct commit [force] [zone ZONE] [mark MASKED_MARK] [label MASKED_LABEL] [nat NAT_SPEC]\n" " 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" + " NAT_SPEC is {src|dst} [addr addr1[-addr2] [port port1[-port2]]]\n" "\n"); exit(-1); } @@ -234,7 +234,7 @@ parse_ct(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, NEXT_ARG(); if (matches(*argv, "addr") != 0) - usage(); + continue; NEXT_ARG(); ret = ct_parse_nat_addr_range(*argv, n);
Action ct commit should accept nat src/dst without an addr. Fix it. Fixes: c8a494314c40 ("tc: Introduce tc ct action") Signed-off-by: Roi Dayan <roid@nvidia.com> Reviewed-by: Paul Blakey <paulb@nvidia.com> --- man/man8/tc-ct.8 | 2 +- tc/m_ct.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)