[nft PATCH 0/4] libnftables: minor cleanups initalizing nf_sock instance of nft_ctx

[nft PATCH 0/4] libnftables: minor cleanups initalizing nf_sock instance of nft_ctx

[Thomas Haller]

There is some unnecessary or redundant code in "src/libnftables.c".
Clean up.

This was motivated by an attempt to add a new flag for nft_ctx_new(),
beside NFT_CTX_DEFAULT. The current "if (flags == NFT_CTX_DEFAULT)"
check is an odd usage for flags (because we would want that behavior for
all flags).

Thomas Haller (4):
  libnftables: always initialize netlink socket in nft_ctx_new()
  libnftables: drop unused argument nf_sock from nft_netlink()
  libnftables: inline creation of nf_sock in nft_ctx_new()
  libnftables: drop check for nf_sock in nft_ctx_free()

 src/libnftables.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

-- 
2.41.0

[nft PATCH 1/4] libnftables: always initialize netlink socket in nft_ctx_new()

[Thomas Haller]

nft_ctx_new() has a flags argument, but currently no flags are
supported. The documentation suggests to pass 0 (NFT_CTX_DEFAULT).

Initializing the netlink socket happens by default already, we should do
it for all flags. Also because  nft_ctx_netlink_init() is not public
API so it's not clear how the user gets a functioning context instance
otherwise.

If we ever want to not initialize the netlink socket for a context
instance, then there should be a dedicated flag for doing that (and
additional API for making that mode of operation usable).

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/libnftables.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index de16d203a017..57e0fc77f989 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -219,8 +219,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	ctx->output.error_fp = stderr;
 	init_list_head(&ctx->vars_ctx.indesc_list);
 
-	if (flags == NFT_CTX_DEFAULT)
-		nft_ctx_netlink_init(ctx);
+	nft_ctx_netlink_init(ctx);
 
 	return ctx;
 }
-- 
2.41.0

[nft PATCH 2/4] libnftables: drop unused argument nf_sock from nft_netlink()

[Thomas Haller]

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/libnftables.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 57e0fc77f989..5b3eb2dc3df4 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -17,8 +17,7 @@
 #include <string.h>
 
 static int nft_netlink(struct nft_ctx *nft,
-		       struct list_head *cmds, struct list_head *msgs,
-		       struct mnl_socket *nf_sock)
+		       struct list_head *cmds, struct list_head *msgs)
 {
 	uint32_t batch_seqnum, seqnum = 0, last_seqnum = UINT32_MAX, num_cmds = 0;
 	struct netlink_ctx ctx = {
@@ -595,7 +594,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, const char *buf)
 		goto err;
 	}
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &cmds, &msgs) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
@@ -691,7 +690,7 @@ static int __nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename
 		goto err;
 	}
 
-	if (nft_netlink(nft, &cmds, &msgs, nft->nf_sock) != 0)
+	if (nft_netlink(nft, &cmds, &msgs) != 0)
 		rc = -1;
 err:
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-- 
2.41.0

[nft PATCH 3/4] libnftables: inline creation of nf_sock in nft_ctx_new()

[Thomas Haller]

The function only has one caller. It's not clear how to extend this in a
useful way, so that it makes sense to keep the initialization in a
separate function.

Simplify the code, by inlining and dropping the static function
nft_ctx_netlink_init(). There was only one caller.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/libnftables.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 5b3eb2dc3df4..79dfdfc7c6ec 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -186,11 +186,6 @@ void nft_ctx_clear_include_paths(struct nft_ctx *ctx)
 	ctx->include_paths = NULL;
 }
 
-static void nft_ctx_netlink_init(struct nft_ctx *ctx)
-{
-	ctx->nf_sock = nft_mnl_socket_open();
-}
-
 EXPORT_SYMBOL(nft_ctx_new);
 struct nft_ctx *nft_ctx_new(uint32_t flags)
 {
@@ -218,7 +213,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	ctx->output.error_fp = stderr;
 	init_list_head(&ctx->vars_ctx.indesc_list);
 
-	nft_ctx_netlink_init(ctx);
+	ctx->nf_sock = nft_mnl_socket_open();
 
 	return ctx;
 }
-- 
2.41.0

[nft PATCH 4/4] libnftables: drop check for nf_sock in nft_ctx_free()

[Thomas Haller]

The "nft_ctx" API does not provide a way to change or reconnect the
netlink socket. And none of the users would rely on that.

Also note that nft_ctx_new() initializes nf_sock via
nft_mnl_socket_open(), which panics of the socket could not be
initialized.

This means, the check is unnecessary and needlessly confusing. Drop it.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/libnftables.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 79dfdfc7c6ec..6fc4f7db6760 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -337,8 +337,7 @@ const char *nft_ctx_get_error_buffer(struct nft_ctx *ctx)
 EXPORT_SYMBOL(nft_ctx_free);
 void nft_ctx_free(struct nft_ctx *ctx)
 {
-	if (ctx->nf_sock)
-		mnl_socket_close(ctx->nf_sock);
+	mnl_socket_close(ctx->nf_sock);
 
 	exit_cookie(&ctx->output.output_cookie);
 	exit_cookie(&ctx->output.error_cookie);
-- 
2.41.0

Re: [nft PATCH 0/4] libnftables: minor cleanups initalizing nf_sock instance of nft_ctx

[Pablo Neira Ayuso]

On Mon, Jul 10, 2023 at 10:45:15AM +0200, Thomas Haller wrote:
> There is some unnecessary or redundant code in "src/libnftables.c".
> Clean up.
> 
> This was motivated by an attempt to add a new flag for nft_ctx_new(),
> beside NFT_CTX_DEFAULT. The current "if (flags == NFT_CTX_DEFAULT)"
> check is an odd usage for flags (because we would want that behavior for
> all flags).

Series applied, thanks