[nft PATCH 0/4] A bunch of fixes for --echo option

[nft PATCH 0/4] A bunch of fixes for --echo option

[Phil Sutter]

Echo/monitor testsuite failed for multiple reasons. With this series
applied and a libnftnl with the recently submiited NFTA_CT_TIMEOUT_DATA
parser fix in place, testsuite finally passes again.

Phil Sutter (4):
  monitor: Add missing newline to error message
  Revert "monitor: fix double cache update with --echo"
  tests/monitor: Fix for changed ct timeout format
  rule: Fix for single line ct timeout printing

 src/monitor.c                    | 3 ++-
 src/rule.c                       | 2 +-
 tests/monitor/testcases/object.t | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

-- 
2.23.0

[nft PATCH 1/4] monitor: Add missing newline to error message

[Phil Sutter]

These shouldn't happen in practice and printing to stderr is not the
right thing either, but fix this anyway.

Fixes: f9563c0feb24d ("src: add events reporting")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/monitor.c b/src/monitor.c
index 40c381149cdaa..20810a5de0cfb 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -388,7 +388,7 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
 
 	set = set_lookup_global(family, table, setname, &monh->ctx->nft->cache);
 	if (set == NULL) {
-		fprintf(stderr, "W: Received event for an unknown set.");
+		fprintf(stderr, "W: Received event for an unknown set.\n");
 		goto out;
 	}
 
-- 
2.23.0

Re: [nft PATCH 1/4] monitor: Add missing newline to error message

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> These shouldn't happen in practice and printing to stderr is not the
> right thing either, but fix this anyway.
> 
> Fixes: f9563c0feb24d ("src: add events reporting")

Acked-by: Florian Westphal <fw@strlen.de>

Re: [nft PATCH 1/4] monitor: Add missing newline to error message

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:03:19AM +0200, Phil Sutter wrote:
> These shouldn't happen in practice and printing to stderr is not the
> right thing either, but fix this anyway.
> 
> Fixes: f9563c0feb24d ("src: add events reporting")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  src/monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/monitor.c b/src/monitor.c
> index 40c381149cdaa..20810a5de0cfb 100644
> --- a/src/monitor.c
> +++ b/src/monitor.c
> @@ -388,7 +388,7 @@ static int netlink_events_setelem_cb(const struct nlmsghdr *nlh, int type,
>  
>  	set = set_lookup_global(family, table, setname, &monh->ctx->nft->cache);
>  	if (set == NULL) {
> -		fprintf(stderr, "W: Received event for an unknown set.");
> +		fprintf(stderr, "W: Received event for an unknown set.\n");
>  		goto out;
>  	}
>  
> -- 
> 2.23.0
> 

[nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter]

This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.

While it is true that a cache exists, we still need to capture new sets
and their elements if they are anonymous. This is because the name
changes and rules will refer to them by name.

Given that there is no easy way to identify the anonymous set in cache
(kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
just go with cache updates. Assuming that echo option is typically used
for single commands, there is not much cache updating happening anyway.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/monitor.c b/src/monitor.c
index 20810a5de0cfb..f353c5b09cf5d 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -900,6 +900,7 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
 		.ctx = ctx,
 		.loc = &netlink_location,
 		.monitor_flags = 0xffffffff,
+		.cache_needed = true,
 	};
 
 	if (!nft_output_echo(&echo_monh.ctx->nft->output))
-- 
2.23.0

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> 
> While it is true that a cache exists, we still need to capture new sets
> and their elements if they are anonymous. This is because the name
> changes and rules will refer to them by name.
> 
> Given that there is no easy way to identify the anonymous set in cache
> (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> just go with cache updates. Assuming that echo option is typically used
> for single commands, there is not much cache updating happening anyway.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
>
> While it is true that a cache exists, we still need to capture new sets
> and their elements if they are anonymous. This is because the name
> changes and rules will refer to them by name.
>
> Given that there is no easy way to identify the anonymous set in cache
> (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> just go with cache updates. Assuming that echo option is typically used
> for single commands, there is not much cache updating happening anyway.

This was fixing a real bug, if this is breaking anything, then I think
we are not getting to the root cause.

But reverting it does not make things any better.

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> >
> > While it is true that a cache exists, we still need to capture new sets
> > and their elements if they are anonymous. This is because the name
> > changes and rules will refer to them by name.

Please, tell me how I can reproduce this here with a simple snippet
and I will have a look. Thanks!

> > Given that there is no easy way to identify the anonymous set in cache
> > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > just go with cache updates. Assuming that echo option is typically used
> > for single commands, there is not much cache updating happening anyway.
> 
> This was fixing a real bug, if this is breaking anything, then I think
> we are not getting to the root cause.
> 
> But reverting it does not make things any better.

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter]

Hi Pablo,

On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > >
> > > While it is true that a cache exists, we still need to capture new sets
> > > and their elements if they are anonymous. This is because the name
> > > changes and rules will refer to them by name.
> 
> Please, tell me how I can reproduce this here with a simple snippet
> and I will have a look. Thanks!

Just run tests/monitor testsuite, echo testing simple.t will fail.
Alternatively, add a rule with anonymous set like so:
| # nft --echo add rule inet t c tcp dport '{ 22, 80 }'

> > > Given that there is no easy way to identify the anonymous set in cache
> > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > just go with cache updates. Assuming that echo option is typically used
> > > for single commands, there is not much cache updating happening anyway.
> > 
> > This was fixing a real bug, if this is breaking anything, then I think
> > we are not getting to the root cause.
> > 
> > But reverting it does not make things any better.

With all respect, this wasn't obvious. There is no test case covering
it, commit message reads like it is an optimization (apart from the
subject containing 'fix').

Also, I tried to find a real solution (assuming that I'm merely supposed
to avoid pointless cache insertion) but ended up with code updating cache
for "no cache needed" case for anonymous sets and their elements. I
didn't revert out of laziness but because I deemed anything else not
feasible, after having tried two alternatives already.

Cheers, Phil

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > > >
> > > > While it is true that a cache exists, we still need to capture new sets
> > > > and their elements if they are anonymous. This is because the name
> > > > changes and rules will refer to them by name.
> > 
> > Please, tell me how I can reproduce this here with a simple snippet
> > and I will have a look. Thanks!
> 
> Just run tests/monitor testsuite, echo testing simple.t will fail.
> Alternatively, add a rule with anonymous set like so:
> | # nft --echo add rule inet t c tcp dport '{ 22, 80 }'

let me have a look.

> > > > Given that there is no easy way to identify the anonymous set in cache
> > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > > just go with cache updates. Assuming that echo option is typically used
> > > > for single commands, there is not much cache updating happening anyway.
> > > 
> > > This was fixing a real bug, if this is breaking anything, then I think
> > > we are not getting to the root cause.
> > > 
> > > But reverting it does not make things any better.
> 
> With all respect, this wasn't obvious. There is no test case covering
> it, commit message reads like it is an optimization (apart from the
> subject containing 'fix').

This patch is fixing --echo with nft using a batch via -f. I started
updating the test infrastructure but I never finished this.

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter]

Hi,

On Thu, Oct 17, 2019 at 01:09:57PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > > > >
> > > > > While it is true that a cache exists, we still need to capture new sets
> > > > > and their elements if they are anonymous. This is because the name
> > > > > changes and rules will refer to them by name.
> > > 
> > > Please, tell me how I can reproduce this here with a simple snippet
> > > and I will have a look. Thanks!
> > 
> > Just run tests/monitor testsuite, echo testing simple.t will fail.
> > Alternatively, add a rule with anonymous set like so:
> > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }'
> 
> let me have a look.

Thanks!

> > > > > Given that there is no easy way to identify the anonymous set in cache
> > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > > > just go with cache updates. Assuming that echo option is typically used
> > > > > for single commands, there is not much cache updating happening anyway.
> > > > 
> > > > This was fixing a real bug, if this is breaking anything, then I think
> > > > we are not getting to the root cause.
> > > > 
> > > > But reverting it does not make things any better.
> > 
> > With all respect, this wasn't obvious. There is no test case covering
> > it, commit message reads like it is an optimization (apart from the
> > subject containing 'fix').
> 
> This patch is fixing --echo with nft using a batch via -f. I started
> updating the test infrastructure but I never finished this.

Ah, I guess extending tests/monitor is not feasible for that. Maybe just
add a case to tests/shell/testcases/nft-f? There is one shell
test for echo option already (cache/0007_echo_cache_init_0).

Cheers, Phil

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > > >
> > > > While it is true that a cache exists, we still need to capture new sets
> > > > and their elements if they are anonymous. This is because the name
> > > > changes and rules will refer to them by name.
> > 
> > Please, tell me how I can reproduce this here with a simple snippet
> > and I will have a look. Thanks!
> 
> Just run tests/monitor testsuite, echo testing simple.t will fail.
> Alternatively, add a rule with anonymous set like so:
> | # nft --echo add rule inet t c tcp dport '{ 22, 80 }'
> 
> > > > Given that there is no easy way to identify the anonymous set in cache
> > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > > just go with cache updates. Assuming that echo option is typically used
> > > > for single commands, there is not much cache updating happening anyway.
> > > 
> > > This was fixing a real bug, if this is breaking anything, then I think
> > > we are not getting to the root cause.
> > > 
> > > But reverting it does not make things any better.
> 
> With all respect, this wasn't obvious. There is no test case covering
> it, commit message reads like it is an optimization (apart from the
> subject containing 'fix').

After reverting:

# ./run-tests.sh testcases/sets/0036add_set_element_expiration_0
I: using nft binary ./../../src/nft

W: [FAILED]     testcases/sets/0036add_set_element_expiration_0: got 139

I: results: [OK] 0 [FAILED] 1 [TOTAL] 1

so I made the test for this fix.

commit 44348edfb9fa414152d53bcf705db882899ddc4e
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Mon Jul 1 18:34:42 2019 +0200

    tests: shell: restore element expiration

    This patch adds a test for 24f33c710e8c ("src: enable set expiration
    date for set elements").

    This is also implicitly testing for a cache corruption bug that is fixed
    by 9b032cd6477b ("monitor: fix double cache update with --echo").

Re: [nft PATCH 2/4] Revert "monitor: fix double cache update with --echo"

[Phil Sutter]

On Thu, Oct 17, 2019 at 01:29:17PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote:
> > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote:
> > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754.
> > > > >
> > > > > While it is true that a cache exists, we still need to capture new sets
> > > > > and their elements if they are anonymous. This is because the name
> > > > > changes and rules will refer to them by name.
> > > 
> > > Please, tell me how I can reproduce this here with a simple snippet
> > > and I will have a look. Thanks!
> > 
> > Just run tests/monitor testsuite, echo testing simple.t will fail.
> > Alternatively, add a rule with anonymous set like so:
> > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }'
> > 
> > > > > Given that there is no easy way to identify the anonymous set in cache
> > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name,
> > > > > just go with cache updates. Assuming that echo option is typically used
> > > > > for single commands, there is not much cache updating happening anyway.
> > > > 
> > > > This was fixing a real bug, if this is breaking anything, then I think
> > > > we are not getting to the root cause.
> > > > 
> > > > But reverting it does not make things any better.
> > 
> > With all respect, this wasn't obvious. There is no test case covering
> > it, commit message reads like it is an optimization (apart from the
> > subject containing 'fix').
> 
> After reverting:
> 
> # ./run-tests.sh testcases/sets/0036add_set_element_expiration_0
> I: using nft binary ./../../src/nft
> 
> W: [FAILED]     testcases/sets/0036add_set_element_expiration_0: got 139
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> so I made the test for this fix.
> 
> commit 44348edfb9fa414152d53bcf705db882899ddc4e
> Author: Pablo Neira Ayuso <pablo@netfilter.org>
> Date:   Mon Jul 1 18:34:42 2019 +0200
> 
>     tests: shell: restore element expiration
> 
>     This patch adds a test for 24f33c710e8c ("src: enable set expiration
>     date for set elements").
> 
>     This is also implicitly testing for a cache corruption bug that is fixed
>     by 9b032cd6477b ("monitor: fix double cache update with --echo").

Ah, thanks for the pointer! I'll check what's going wrong there.

Cheers, Phil

[nft PATCH 3/4] tests/monitor: Fix for changed ct timeout format

[Phil Sutter]

Commit a9b0c385a1d5e ("rule: print space between policy and timeout")
changed spacing in ct timeout objects but missed to adjust related test
case.

Fixes: a9b0c385a1d5e ("rule: print space between policy and timeout")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/monitor/testcases/object.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/monitor/testcases/object.t b/tests/monitor/testcases/object.t
index dacfed29d639e..2afe33c812571 100644
--- a/tests/monitor/testcases/object.t
+++ b/tests/monitor/testcases/object.t
@@ -37,7 +37,7 @@ I delete ct helper ip t cth
 O -
 J {"delete": {"ct helper": {"family": "ip", "name": "cth", "table": "t", "handle": 0, "type": "sip", "protocol": "tcp", "l3proto": "ip"}}}
 
-I add ct timeout ip t ctt { protocol udp; l3proto ip; policy = { unreplied: 15, replied: 12 }; }
+I add ct timeout ip t ctt { protocol udp; l3proto ip; policy = { unreplied : 15, replied : 12 }; }
 O -
 J {"add": {"ct timeout": {"family": "ip", "name": "ctt", "table": "t", "handle": 0, "protocol": "udp", "l3proto": "ip", "policy": {"unreplied": 15, "replied": 12}}}}
 
-- 
2.23.0

Re: [nft PATCH 3/4] tests/monitor: Fix for changed ct timeout format

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Commit a9b0c385a1d5e ("rule: print space between policy and timeout")
> changed spacing in ct timeout objects but missed to adjust related test
> case.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [nft PATCH 3/4] tests/monitor: Fix for changed ct timeout format

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:03:21AM +0200, Phil Sutter wrote:
> Commit a9b0c385a1d5e ("rule: print space between policy and timeout")
> changed spacing in ct timeout objects but missed to adjust related test
> case.
> 
> Fixes: a9b0c385a1d5e ("rule: print space between policy and timeout")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Phil Sutter]

Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
removed an extra semicolon at end of line, but thereby broke single line
output. The correct fix is to use opts->stmt_separator which holds
either newline or semicolon chars depending on output mode.

Fixes: 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/rule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index 2d35bae44c9e5..3c7c8d63f8cdf 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1869,7 +1869,7 @@ static void obj_print_data(const struct obj *obj,
 		nft_print(octx, "%s", opts->nl);
 		nft_print(octx, "%s%sprotocol ", opts->tab, opts->tab);
 		print_proto_name_proto(obj->ct_timeout.l4proto, octx);
-		nft_print(octx, "%s", opts->nl);
+		nft_print(octx, "%s", opts->stmt_separator);
 		nft_print(octx, "%s%sl3proto %s%s",
 			  opts->tab, opts->tab,
 			  family2str(obj->ct_timeout.l3proto),
-- 
2.23.0

Re: [nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> removed an extra semicolon at end of line, but thereby broke single line
> output. The correct fix is to use opts->stmt_separator which holds
> either newline or semicolon chars depending on output mode.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:03:22AM +0200, Phil Sutter wrote:
> Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> removed an extra semicolon at end of line, but thereby broke single line
> output. The correct fix is to use opts->stmt_separator which holds
> either newline or semicolon chars depending on output mode.

What output mode this breaks? It looks indeed like I overlook
something while fixing up this.

Thanks.

> Fixes: 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/rule.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/rule.c b/src/rule.c
> index 2d35bae44c9e5..3c7c8d63f8cdf 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1869,7 +1869,7 @@ static void obj_print_data(const struct obj *obj,
>  		nft_print(octx, "%s", opts->nl);
>  		nft_print(octx, "%s%sprotocol ", opts->tab, opts->tab);
>  		print_proto_name_proto(obj->ct_timeout.l4proto, octx);
> -		nft_print(octx, "%s", opts->nl);
> +		nft_print(octx, "%s", opts->stmt_separator);
>  		nft_print(octx, "%s%sl3proto %s%s",
>  			  opts->tab, opts->tab,
>  			  family2str(obj->ct_timeout.l3proto),
> -- 
> 2.23.0
> 

Re: [nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Phil Sutter]

On Thu, Oct 17, 2019 at 01:14:37PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 17, 2019 at 01:03:22AM +0200, Phil Sutter wrote:
> > Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> > removed an extra semicolon at end of line, but thereby broke single line
> > output. The correct fix is to use opts->stmt_separator which holds
> > either newline or semicolon chars depending on output mode.
> 
> What output mode this breaks? It looks indeed like I overlook
> something while fixing up this.

It breaks syntax of monitor and echo output. We don't propagate it, but
the goal always has been for those to print syntactically correct
commands.

The concrete test case in tests/monitor/testcases/object.t is:

| add ct timeout ip t ctt { protocol udp; l3proto ip; policy = { unreplied : 15, replied : 12 }; }

Omitting the semicolon before 'l3proto' is illegal syntax.

Cheers, Phil

Re: [nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:29:10PM +0200, Phil Sutter wrote:
> On Thu, Oct 17, 2019 at 01:14:37PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 17, 2019 at 01:03:22AM +0200, Phil Sutter wrote:
> > > Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> > > removed an extra semicolon at end of line, but thereby broke single line
> > > output. The correct fix is to use opts->stmt_separator which holds
> > > either newline or semicolon chars depending on output mode.
> > 
> > What output mode this breaks? It looks indeed like I overlook
> > something while fixing up this.
> 
> It breaks syntax of monitor and echo output. We don't propagate it, but
> the goal always has been for those to print syntactically correct
> commands.
> 
> The concrete test case in tests/monitor/testcases/object.t is:
> 
> | add ct timeout ip t ctt { protocol udp; l3proto ip; policy = { unreplied : 15, replied : 12 }; }
> 
> Omitting the semicolon before 'l3proto' is illegal syntax.

Ah, the echo mode indeed. LGTM. Thanks for explaining.

Re: [nft PATCH 4/4] rule: Fix for single line ct timeout printing

[Pablo Neira Ayuso]

On Thu, Oct 17, 2019 at 01:03:22AM +0200, Phil Sutter wrote:
> Commit 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> removed an extra semicolon at end of line, but thereby broke single line
> output. The correct fix is to use opts->stmt_separator which holds
> either newline or semicolon chars depending on output mode.
> 
> Fixes: 43ae7a48ae3de ("rule: do not print semicolon in ct timeout")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  src/rule.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/rule.c b/src/rule.c
> index 2d35bae44c9e5..3c7c8d63f8cdf 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1869,7 +1869,7 @@ static void obj_print_data(const struct obj *obj,
>  		nft_print(octx, "%s", opts->nl);
>  		nft_print(octx, "%s%sprotocol ", opts->tab, opts->tab);
>  		print_proto_name_proto(obj->ct_timeout.l4proto, octx);
> -		nft_print(octx, "%s", opts->nl);
> +		nft_print(octx, "%s", opts->stmt_separator);
>  		nft_print(octx, "%s%sl3proto %s%s",
>  			  opts->tab, opts->tab,
>  			  family2str(obj->ct_timeout.l3proto),
> -- 
> 2.23.0
>