[nft PATCH] cli: Make valgrind happy

[nft PATCH] cli: Make valgrind happy

[Phil Sutter]

Missing call to nft_ctx_free() upsets valgrind enough to suspect
possible losses, add them where sensible. This fixes reports with
readline-lined builds at least. The same code is shared for libedit
though, and there's an obvious spot for linenoise.

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

diff --git a/src/cli.c b/src/cli.c
index 11fc85abeaa2b..bc7f64ef0b762 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -126,6 +126,7 @@ static void cli_complete(char *line)
 	if (line == NULL) {
 		printf("\n");
 		cli_exit();
+		nft_ctx_free(cli_nft);
 		exit(0);
 	}
 
@@ -141,6 +142,7 @@ static void cli_complete(char *line)
 
 	if (!strcmp(line, CMDLINE_QUIT)) {
 		cli_exit();
+		nft_ctx_free(cli_nft);
 		exit(0);
 	}
 
@@ -244,6 +246,7 @@ int cli_init(struct nft_ctx *nft)
 		linenoiseFree(line);
 	}
 	cli_exit();
+	nft_ctx_free(nft);
 	exit(0);
 }
 
-- 
2.40.0

Re: [nft PATCH] cli: Make valgrind happy

[Pablo Neira Ayuso]

On Tue, Jun 20, 2023 at 04:03:52PM +0200, Phil Sutter wrote:
> Missing call to nft_ctx_free() upsets valgrind enough to suspect
> possible losses, add them where sensible. This fixes reports with
> readline-lined builds at least. The same code is shared for libedit
> though, and there's an obvious spot for linenoise.

Maybe call nft_ctx_free() from cli_exit() ?

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/cli.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/cli.c b/src/cli.c
> index 11fc85abeaa2b..bc7f64ef0b762 100644
> --- a/src/cli.c
> +++ b/src/cli.c
> @@ -126,6 +126,7 @@ static void cli_complete(char *line)
>  	if (line == NULL) {
>  		printf("\n");
>  		cli_exit();
> +		nft_ctx_free(cli_nft);
>  		exit(0);
>  	}
>  
> @@ -141,6 +142,7 @@ static void cli_complete(char *line)
>  
>  	if (!strcmp(line, CMDLINE_QUIT)) {
>  		cli_exit();
> +		nft_ctx_free(cli_nft);
>  		exit(0);
>  	}
>  
> @@ -244,6 +246,7 @@ int cli_init(struct nft_ctx *nft)
>  		linenoiseFree(line);
>  	}
>  	cli_exit();
> +	nft_ctx_free(nft);
>  	exit(0);
>  }
>  
> -- 
> 2.40.0
> 

Re: [nft PATCH] cli: Make valgrind happy

[Phil Sutter]

On Tue, Jun 20, 2023 at 06:04:33PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jun 20, 2023 at 04:03:52PM +0200, Phil Sutter wrote:
> > Missing call to nft_ctx_free() upsets valgrind enough to suspect
> > possible losses, add them where sensible. This fixes reports with
> > readline-lined builds at least. The same code is shared for libedit
> > though, and there's an obvious spot for linenoise.
> 
> Maybe call nft_ctx_free() from cli_exit() ?

That's doable, but linenoise code does not use the static global cli_nft
variable and thus cli_exit() can't access the struct nft_ctx pointer.

At first, I tried to make main() not exit after calling cli_init() so
final cleanup takes place. But the different CLI variants are a bit of a
mess in that regard: While there are code-paths returning to caller,
most don't. I'm tempted to fix that instead. What do you think?

Thanks, Phil

Re: [nft PATCH] cli: Make valgrind happy

[Pablo Neira Ayuso]

On Wed, Jun 21, 2023 at 02:12:51PM +0200, Phil Sutter wrote:
> On Tue, Jun 20, 2023 at 06:04:33PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jun 20, 2023 at 04:03:52PM +0200, Phil Sutter wrote:
> > > Missing call to nft_ctx_free() upsets valgrind enough to suspect
> > > possible losses, add them where sensible. This fixes reports with
> > > readline-lined builds at least. The same code is shared for libedit
> > > though, and there's an obvious spot for linenoise.
> > 
> > Maybe call nft_ctx_free() from cli_exit() ?
> 
> That's doable, but linenoise code does not use the static global cli_nft
> variable and thus cli_exit() can't access the struct nft_ctx pointer.
> 
> At first, I tried to make main() not exit after calling cli_init() so
> final cleanup takes place. But the different CLI variants are a bit of a
> mess in that regard: While there are code-paths returning to caller,
> most don't. I'm tempted to fix that instead. What do you think?

That's also fine, go ahead update and test all these cli variants.