[libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[Phil Sutter]

This reverts the rule-facing part of commit
c759027a526ac09ce413dc88c308a4ed98b33416.

It can't be right: Rules without userdata are printed with a trailing
newline, so this commit made behaviour inconsistent.

Fixes: c759027a526ac ("rule, set_elem: remove trailing \n in userdata snprintf")
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 811d5a213f835..51d778d095317 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -601,7 +601,7 @@ static int nftnl_rule_snprintf_default(char *buf, size_t remain,
 			SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 		}
 
-		ret = snprintf(buf + offset, remain, " }");
+		ret = snprintf(buf + offset, remain, " }\n");
 		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
 
 	}
-- 
2.43.0

Re: [libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[Pablo Neira Ayuso]

On Tue, Oct 01, 2024 at 01:20:54PM +0200, Phil Sutter wrote:
> This reverts the rule-facing part of commit
> c759027a526ac09ce413dc88c308a4ed98b33416.
> 
> It can't be right: Rules without userdata are printed with a trailing
> newline, so this commit made behaviour inconsistent.

Did you run tests/py with this? It is the primary user for this.

> Fixes: c759027a526ac ("rule, set_elem: remove trailing \n in userdata snprintf")
> 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 811d5a213f835..51d778d095317 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -601,7 +601,7 @@ static int nftnl_rule_snprintf_default(char *buf, size_t remain,
>  			SNPRINTF_BUFFER_SIZE(ret, remain, offset);
>  		}
>  
> -		ret = snprintf(buf + offset, remain, " }");
> +		ret = snprintf(buf + offset, remain, " }\n");
>  		SNPRINTF_BUFFER_SIZE(ret, remain, offset);
>  
>  	}
> -- 
> 2.43.0
> 

Re: [libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[Phil Sutter]

Hi Pablo,

On Tue, Oct 01, 2024 at 01:23:58PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 01, 2024 at 01:20:54PM +0200, Phil Sutter wrote:
> > This reverts the rule-facing part of commit
> > c759027a526ac09ce413dc88c308a4ed98b33416.
> > 
> > It can't be right: Rules without userdata are printed with a trailing
> > newline, so this commit made behaviour inconsistent.
> 
> Did you run tests/py with this? It is the primary user for this.

It doesn't cover this because there's no test containing a rule with a
comment. I just added a respective test, but only to notice it does not
matter because nft-test.py compares rules' payload individually per-rule
and thus does not care whether output has a trailing newline or not.

I noticed it when testing the iptables compat ext stuff. You can easily
reproduce it like so:

| # nft --check --debug=netlink 'table t { chain c { accept comment mycomment; accept; accept;};}'
| ip (null) (null) use 0
| ip t c
|   [ immediate reg 0 accept ]
|   userdata = { \x00\x0amycomment\x00 }
| ip t c
|   [ immediate reg 0 accept ]
| 
| ip t c
|   [ immediate reg 0 accept ]

Note the missing empty line after the first rule.

Cheers, Phil

Re: [libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[Pablo Neira Ayuso]

On Tue, Oct 01, 2024 at 02:39:52PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Tue, Oct 01, 2024 at 01:23:58PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 01, 2024 at 01:20:54PM +0200, Phil Sutter wrote:
> > > This reverts the rule-facing part of commit
> > > c759027a526ac09ce413dc88c308a4ed98b33416.
> > > 
> > > It can't be right: Rules without userdata are printed with a trailing
> > > newline, so this commit made behaviour inconsistent.
> > 
> > Did you run tests/py with this? It is the primary user for this.
> 
> It doesn't cover this because there's no test containing a rule with a
> comment. I just added a respective test, but only to notice it does not
> matter because nft-test.py compares rules' payload individually per-rule
> and thus does not care whether output has a trailing newline or not.
> 
> I noticed it when testing the iptables compat ext stuff. You can easily
> reproduce it like so:
> 
> | # nft --check --debug=netlink 'table t { chain c { accept comment mycomment; accept; accept;};}'
> | ip (null) (null) use 0
> | ip t c
> |   [ immediate reg 0 accept ]
> |   userdata = { \x00\x0amycomment\x00 }
> | ip t c
> |   [ immediate reg 0 accept ]
> | 
> | ip t c
> |   [ immediate reg 0 accept ]
> 
> Note the missing empty line after the first rule.

None of the existing libnftnl _snprintf functions terminate string
with line break, right?. I mean, for consistency with other existing
_snprintf functions. Maybe fix nft instead?

Re: [libnftnl PATCH] Partially revert "rule, set_elem: remove trailing \n in userdata snprintf"

[Phil Sutter]

On Tue, Oct 01, 2024 at 02:47:01PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 01, 2024 at 02:39:52PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Tue, Oct 01, 2024 at 01:23:58PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 01, 2024 at 01:20:54PM +0200, Phil Sutter wrote:
> > > > This reverts the rule-facing part of commit
> > > > c759027a526ac09ce413dc88c308a4ed98b33416.
> > > > 
> > > > It can't be right: Rules without userdata are printed with a trailing
> > > > newline, so this commit made behaviour inconsistent.
> > > 
> > > Did you run tests/py with this? It is the primary user for this.
> > 
> > It doesn't cover this because there's no test containing a rule with a
> > comment. I just added a respective test, but only to notice it does not
> > matter because nft-test.py compares rules' payload individually per-rule
> > and thus does not care whether output has a trailing newline or not.
> > 
> > I noticed it when testing the iptables compat ext stuff. You can easily
> > reproduce it like so:
> > 
> > | # nft --check --debug=netlink 'table t { chain c { accept comment mycomment; accept; accept;};}'
> > | ip (null) (null) use 0
> > | ip t c
> > |   [ immediate reg 0 accept ]
> > |   userdata = { \x00\x0amycomment\x00 }
> > | ip t c
> > |   [ immediate reg 0 accept ]
> > | 
> > | ip t c
> > |   [ immediate reg 0 accept ]
> > 
> > Note the missing empty line after the first rule.
> 
> None of the existing libnftnl _snprintf functions terminate string
> with line break, right?. I mean, for consistency with other existing
> _snprintf functions. Maybe fix nft instead?

Correct, though others don't emit newlines at all. I'll change rule
debug output to never append a newline and see what breaks.

Thanks, Phil