[nft PATCH 0/7] libnftables preparations

[nft PATCH 0/7] libnftables preparations

[Phil Sutter]

The following series prepares libnftables libarary split-off by moving
API functions into src/libnftables.c, introducing
include/nftables/nftables.h and enhancing the code by a number of
getters and setters for applications to change configurable parts of
struct nft_ctx without knowledge of that struct's internals.

The 'nft' binary will become the first "demo" user of libnftables and
acts as a reference for library design and usability.

Phil Sutter (7):
  nft_ctx_free: Fix for wrong argument passed to cache_release
  libnftables: Move library stuff out of main.c
  libnftables: Introduce nft_ctx_flush_cache()
  cli: Use nft_run_cmd_from_buffer()
  libnftables: Introduce nft_ctx_set_dry_run()
  libnftables: Provide an API for include path handling
  libnftables: Add remaining getters and setters

 include/Makefile.am          |   3 +-
 include/cli.h                |   6 +-
 include/nftables.h           |  48 +-----
 include/nftables/Makefile.am |   1 +
 include/nftables/nftables.h  |  72 +++++++++
 src/Makefile.am              |   3 +-
 src/cli.c                    |  24 +--
 src/libnftables.c            | 360 +++++++++++++++++++++++++++++++++++++++++++
 src/main.c                   | 285 +++-------------------------------
 src/scanner.l                |   4 +-
 10 files changed, 468 insertions(+), 338 deletions(-)
 create mode 100644 include/nftables/Makefile.am
 create mode 100644 include/nftables/nftables.h
 create mode 100644 src/libnftables.c

-- 
2.13.1

[nft PATCH 1/7] nft_ctx_free: Fix for wrong argument passed to cache_release

[Phil Sutter]

nft_ctx_free() should not refer to the global 'nft' variable, this will
break as soon as the function is moved away from main.c. In order to use
the cache reference from passed argument, the latter must not be const.

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

diff --git a/src/main.c b/src/main.c
index b59c932ad4299..1b26838058a4a 100644
--- a/src/main.c
+++ b/src/main.c
@@ -305,13 +305,13 @@ static struct nft_ctx *nft_ctx_new(uint32_t flags)
 	return ctx;
 }
 
-static void nft_ctx_free(const struct nft_ctx *ctx)
+static void nft_ctx_free(struct nft_ctx *ctx)
 {
 	if (ctx->nf_sock)
 		netlink_close_sock(ctx->nf_sock);
 
 	iface_cache_release();
-	cache_release(&nft->cache);
+	cache_release(&ctx->cache);
 	xfree(ctx);
 	nft_exit();
 }
-- 
2.13.1

Re: [nft PATCH 1/7] nft_ctx_free: Fix for wrong argument passed to cache_release

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:41AM +0200, Phil Sutter wrote:
> nft_ctx_free() should not refer to the global 'nft' variable, this will
> break as soon as the function is moved away from main.c. In order to use
> the cache reference from passed argument, the latter must not be const.

Applied, thanks.

[nft PATCH 2/7] libnftables: Move library stuff out of main.c

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/Makefile.am          |   3 +-
 include/nftables.h           |  65 +----------
 include/nftables/Makefile.am |   1 +
 include/nftables/nftables.h  |  88 +++++++++++++++
 src/Makefile.am              |   3 +-
 src/libnftables.c            | 261 +++++++++++++++++++++++++++++++++++++++++++
 src/main.c                   | 253 +----------------------------------------
 7 files changed, 356 insertions(+), 318 deletions(-)
 create mode 100644 include/nftables/Makefile.am
 create mode 100644 include/nftables/nftables.h
 create mode 100644 src/libnftables.c

diff --git a/include/Makefile.am b/include/Makefile.am
index 5dd73d81f427e..a74ffbfa8de0a 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -1,4 +1,5 @@
-SUBDIRS = linux
+SUBDIRS =		linux		\
+			nftables
 
 noinst_HEADERS = 	cli.h		\
 			datatype.h	\
diff --git a/include/nftables.h b/include/nftables.h
index 01d72a87212ea..a633e1a2cc2e2 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -4,63 +4,7 @@
 #include <stdbool.h>
 #include <stdarg.h>
 #include <utils.h>
-
-enum numeric_level {
-	NUMERIC_NONE,
-	NUMERIC_ADDR,
-	NUMERIC_PORT,
-	NUMERIC_ALL,
-};
-
-enum debug_level {
-	DEBUG_SCANNER		= 0x1,
-	DEBUG_PARSER		= 0x2,
-	DEBUG_EVALUATION	= 0x4,
-	DEBUG_NETLINK		= 0x8,
-	DEBUG_MNL		= 0x10,
-	DEBUG_PROTO_CTX		= 0x20,
-	DEBUG_SEGTREE		= 0x40,
-};
-
-#define INCLUDE_PATHS_MAX	16
-
-struct output_ctx {
-	unsigned int numeric;
-	unsigned int stateless;
-	unsigned int ip2name;
-	unsigned int handle;
-	unsigned int echo;
-	FILE *output_fp;
-};
-
-struct nft_cache {
-	bool			initialized;
-	struct list_head	list;
-	uint32_t		seqnum;
-};
-
-struct mnl_socket;
-
-struct nft_ctx {
-	struct mnl_socket	*nf_sock;
-	const char		*include_paths[INCLUDE_PATHS_MAX];
-	unsigned int		num_include_paths;
-	unsigned int		parser_max_errors;
-	unsigned int		debug_mask;
-	struct output_ctx	output;
-	bool			check;
-	struct nft_cache	cache;
-	uint32_t		flags;
-};
-
-#define NFT_CTX_DEFAULT		0
-
-enum nftables_exit_codes {
-	NFT_EXIT_SUCCESS	= 0,
-	NFT_EXIT_FAILURE	= 1,
-	NFT_EXIT_NOMEM		= 2,
-	NFT_EXIT_NONL		= 3,
-};
+#include <nftables/nftables.h>
 
 struct input_descriptor;
 struct location {
@@ -128,13 +72,6 @@ struct input_descriptor {
 	off_t				line_offset;
 };
 
-struct parser_state;
-struct mnl_socket;
-
-int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-	    void *scanner, struct parser_state *state,
-	    struct list_head *msgs);
-
 void ct_label_table_init(void);
 void mark_table_init(void);
 void gmp_init(void);
diff --git a/include/nftables/Makefile.am b/include/nftables/Makefile.am
new file mode 100644
index 0000000000000..9e31d519599c1
--- /dev/null
+++ b/include/nftables/Makefile.am
@@ -0,0 +1 @@
+noinst_HEADERS = nftables.h
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
new file mode 100644
index 0000000000000..052a77bfb5371
--- /dev/null
+++ b/include/nftables/nftables.h
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2017 Eric Leblond <eric@regit.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#ifndef LIB_NFTABLES_H
+#define LIB_NFTABLES_H
+
+struct parser_state;
+struct mnl_socket;
+
+struct nft_cache {
+	bool			initialized;
+	struct list_head	list;
+	uint32_t		seqnum;
+};
+
+#define INCLUDE_PATHS_MAX	16
+
+struct output_ctx {
+	unsigned int numeric;
+	unsigned int stateless;
+	unsigned int ip2name;
+	unsigned int handle;
+	unsigned int echo;
+	FILE *output_fp;
+};
+
+struct nft_ctx {
+	struct mnl_socket	*nf_sock;
+	const char		*include_paths[INCLUDE_PATHS_MAX];
+	unsigned int		num_include_paths;
+	unsigned int		parser_max_errors;
+	unsigned int		debug_mask;
+	struct output_ctx	output;
+	bool			check;
+	struct nft_cache	cache;
+	uint32_t		flags;
+};
+
+enum debug_level {
+	DEBUG_SCANNER		= 0x1,
+	DEBUG_PARSER		= 0x2,
+	DEBUG_EVALUATION	= 0x4,
+	DEBUG_NETLINK		= 0x8,
+	DEBUG_MNL		= 0x10,
+	DEBUG_PROTO_CTX		= 0x20,
+	DEBUG_SEGTREE		= 0x40,
+};
+
+enum numeric_level {
+	NUMERIC_NONE,
+	NUMERIC_ADDR,
+	NUMERIC_PORT,
+	NUMERIC_ALL,
+};
+
+/**
+ * Possible flags to pass to nft_ctx_new()
+ */
+#define NFT_CTX_DEFAULT		0
+
+/**
+ * Exit codes returned by nft_run_cmd_from_*()
+ */
+enum nftables_exit_codes {
+	NFT_EXIT_SUCCESS	= 0,
+	NFT_EXIT_FAILURE	= 1,
+	NFT_EXIT_NOMEM		= 2,
+	NFT_EXIT_NONL		= 3,
+};
+
+
+struct nft_ctx *nft_ctx_new(uint32_t flags);
+void nft_ctx_free(struct nft_ctx *ctx);
+FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
+
+int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
+	    void *scanner, struct parser_state *state,
+	    struct list_head *msgs);
+
+int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen);
+int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename);
+
+#endif /* LIB_NFTABLES_H */
diff --git a/src/Makefile.am b/src/Makefile.am
index 99eef7bb849b6..4d613a731dfb9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -56,7 +56,8 @@ nft_SOURCES =	main.c				\
 		mergesort.c			\
 		scanner.l			\
 		tcpopt.c			\
-		parser_bison.y
+		parser_bison.y			\
+		libnftables.c
 
 if BUILD_CLI
 nft_SOURCES +=	cli.c
diff --git a/src/libnftables.c b/src/libnftables.c
new file mode 100644
index 0000000000000..187747c66af21
--- /dev/null
+++ b/src/libnftables.c
@@ -0,0 +1,261 @@
+/*
+ * Copyright (c) 2017 Eric Leblond <eric@regit.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <erec.h>
+#include <errno.h>
+#include <mnl.h>
+#include <parser.h>
+#include <string.h>
+#include <utils.h>
+#include <iface.h>
+#include <nftables/nftables.h>
+
+static int nft_netlink(struct nft_ctx *nft,
+		       struct parser_state *state, struct list_head *msgs,
+		       struct mnl_socket *nf_sock)
+{
+	uint32_t batch_seqnum, seqnum = 0;
+	struct nftnl_batch *batch;
+	struct netlink_ctx ctx;
+	struct cmd *cmd;
+	struct mnl_err *err, *tmp;
+	LIST_HEAD(err_list);
+	bool batch_supported = netlink_batch_supported(nf_sock, &seqnum);
+	int ret = 0;
+
+	batch = mnl_batch_init();
+
+	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
+	list_for_each_entry(cmd, &state->cmds, list) {
+		memset(&ctx, 0, sizeof(ctx));
+		ctx.msgs = msgs;
+		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
+		ctx.batch = batch;
+		ctx.batch_supported = batch_supported;
+		ctx.octx = &nft->output;
+		ctx.nf_sock = nf_sock;
+		ctx.cache = &nft->cache;
+		ctx.debug_mask = nft->debug_mask;
+		init_list_head(&ctx.list);
+		ret = do_command(&ctx, cmd);
+		if (ret < 0)
+			goto out;
+	}
+	if (!nft->check)
+		mnl_batch_end(batch, mnl_seqnum_alloc(&seqnum));
+
+	if (!mnl_batch_ready(batch))
+		goto out;
+
+	ret = netlink_batch_send(&ctx, &err_list);
+
+	list_for_each_entry_safe(err, tmp, &err_list, head) {
+		list_for_each_entry(cmd, &state->cmds, list) {
+			if (err->seqnum == cmd->seqnum ||
+			    err->seqnum == batch_seqnum) {
+				netlink_io_error(&ctx, &cmd->location,
+						 "Could not process rule: %s",
+						 strerror(err->err));
+				errno = err->err;
+				if (err->seqnum == cmd->seqnum) {
+					mnl_err_list_free(err);
+					break;
+				}
+			}
+		}
+	}
+out:
+	mnl_batch_reset(batch);
+	return ret;
+}
+
+int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
+	    void *scanner, struct parser_state *state,
+	    struct list_head *msgs)
+{
+	struct cmd *cmd, *next;
+	int ret;
+
+	ret = nft_parse(nft, scanner, state);
+	if (ret != 0 || state->nerrs > 0) {
+		ret = -1;
+		goto err1;
+	}
+
+	list_for_each_entry(cmd, &state->cmds, list)
+		nft_cmd_expand(cmd);
+
+	ret = nft_netlink(nft, state, msgs, nf_sock);
+err1:
+	list_for_each_entry_safe(cmd, next, &state->cmds, list) {
+		list_del(&cmd->list);
+		cmd_free(cmd);
+	}
+
+	return ret;
+}
+
+static void nft_init(void)
+{
+	mark_table_init();
+	realm_table_rt_init();
+	devgroup_table_init();
+	realm_table_meta_init();
+	ct_label_table_init();
+	gmp_init();
+#ifdef HAVE_LIBXTABLES
+	xt_init();
+#endif
+}
+
+static void nft_exit(void)
+{
+	ct_label_table_exit();
+	realm_table_rt_exit();
+	devgroup_table_exit();
+	realm_table_meta_exit();
+	mark_table_exit();
+}
+
+static void nft_ctx_netlink_init(struct nft_ctx *ctx)
+{
+	ctx->nf_sock = netlink_open_sock();
+}
+
+struct nft_ctx *nft_ctx_new(uint32_t flags)
+{
+	struct nft_ctx *ctx;
+
+	nft_init();
+	ctx = xzalloc(sizeof(struct nft_ctx));
+
+	ctx->include_paths[0]	= DEFAULT_INCLUDE_PATH;
+	ctx->num_include_paths	= 1;
+	ctx->parser_max_errors	= 10;
+	init_list_head(&ctx->cache.list);
+	ctx->flags = flags;
+
+	if (flags == NFT_CTX_DEFAULT)
+		nft_ctx_netlink_init(ctx);
+
+	return ctx;
+}
+
+void nft_ctx_free(struct nft_ctx *ctx)
+{
+	if (ctx->nf_sock)
+		netlink_close_sock(ctx->nf_sock);
+
+	iface_cache_release();
+	cache_release(&ctx->cache);
+	xfree(ctx);
+	nft_exit();
+}
+
+FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
+{
+	FILE *old = ctx->output.output_fp;
+
+	ctx->output.output_fp = fp;
+
+	return old;
+}
+
+static const struct input_descriptor indesc_cmdline = {
+	.type	= INDESC_BUFFER,
+	.name	= "<cmdline>",
+};
+
+int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
+{
+	int rc = NFT_EXIT_SUCCESS;
+	struct parser_state state;
+	LIST_HEAD(msgs);
+	void *scanner;
+	FILE *fp;
+
+	parser_init(nft->nf_sock, &nft->cache, &state,
+		    &msgs, nft->debug_mask, &nft->output);
+	scanner = scanner_init(&state);
+	scanner_push_buffer(scanner, &indesc_cmdline, buf);
+
+	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
+		rc = NFT_EXIT_FAILURE;
+
+	fp = nft_ctx_set_output(nft, stderr);
+	erec_print_list(&nft->output, &msgs, nft->debug_mask);
+	nft_ctx_set_output(nft, fp);
+	scanner_destroy(scanner);
+
+	return rc;
+}
+
+int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
+{
+	struct parser_state state;
+	LIST_HEAD(msgs);
+	void *scanner;
+	int rc;
+	FILE *fp;
+
+	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
+			  nft->debug_mask, &nft->output);
+	if (rc < 0)
+		return NFT_EXIT_FAILURE;
+
+	parser_init(nft->nf_sock, &nft->cache, &state,
+		    &msgs, nft->debug_mask, &nft->output);
+	scanner = scanner_init(&state);
+	if (scanner_read_file(scanner, filename, &internal_location) < 0) {
+		rc = NFT_EXIT_FAILURE;
+		goto err;
+	}
+
+	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
+		rc = NFT_EXIT_FAILURE;
+err:
+	fp = nft_ctx_set_output(nft, stderr);
+	erec_print_list(&nft->output, &msgs, nft->debug_mask);
+	nft_ctx_set_output(nft, fp);
+	scanner_destroy(scanner);
+
+	return rc;
+}
+
+int nft_print(struct output_ctx *octx, const char *fmt, ...)
+{
+	int ret;
+	va_list arg;
+
+	if (!octx->output_fp)
+		return -1;
+
+	va_start(arg, fmt);
+	ret = vfprintf(octx->output_fp, fmt, arg);
+	va_end(arg);
+	fflush(octx->output_fp);
+
+	return ret;
+}
+
+int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
+{
+	int ret;
+	va_list arg;
+
+	if (!octx->output_fp)
+		return -1;
+
+	va_start(arg, fmt);
+	ret = gmp_vfprintf(octx->output_fp, fmt, arg);
+	va_end(arg);
+	fflush(octx->output_fp);
+
+	return ret;
+}
+
diff --git a/src/main.c b/src/main.c
index 1b26838058a4a..a7da460f28ca5 100644
--- a/src/main.c
+++ b/src/main.c
@@ -18,15 +18,9 @@
 #include <fcntl.h>
 #include <sys/types.h>
 
-#include <nftables.h>
-#include <utils.h>
 #include <parser.h>
-#include <rule.h>
-#include <netlink.h>
-#include <erec.h>
-#include <mnl.h>
-#include <iface.h>
 #include <cli.h>
+#include <nftables/nftables.h>
 
 static struct nft_ctx *nft;
 
@@ -169,251 +163,6 @@ static const struct {
 	},
 };
 
-static const struct input_descriptor indesc_cmdline = {
-	.type	= INDESC_BUFFER,
-	.name	= "<cmdline>",
-};
-
-static int nft_netlink(struct nft_ctx *nft,
-		       struct parser_state *state, struct list_head *msgs,
-		       struct mnl_socket *nf_sock)
-{
-	uint32_t batch_seqnum, seqnum = 0;
-	struct nftnl_batch *batch;
-	struct netlink_ctx ctx;
-	struct cmd *cmd;
-	struct mnl_err *err, *tmp;
-	LIST_HEAD(err_list);
-	bool batch_supported = netlink_batch_supported(nf_sock, &seqnum);
-	int ret = 0;
-
-	batch = mnl_batch_init();
-
-	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
-	list_for_each_entry(cmd, &state->cmds, list) {
-		memset(&ctx, 0, sizeof(ctx));
-		ctx.msgs = msgs;
-		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
-		ctx.batch = batch;
-		ctx.batch_supported = batch_supported;
-		ctx.octx = &nft->output;
-		ctx.nf_sock = nf_sock;
-		ctx.cache = &nft->cache;
-		ctx.debug_mask = nft->debug_mask;
-		init_list_head(&ctx.list);
-		ret = do_command(&ctx, cmd);
-		if (ret < 0)
-			goto out;
-	}
-	if (!nft->check)
-		mnl_batch_end(batch, mnl_seqnum_alloc(&seqnum));
-
-	if (!mnl_batch_ready(batch))
-		goto out;
-
-	ret = netlink_batch_send(&ctx, &err_list);
-
-	list_for_each_entry_safe(err, tmp, &err_list, head) {
-		list_for_each_entry(cmd, &state->cmds, list) {
-			if (err->seqnum == cmd->seqnum ||
-			    err->seqnum == batch_seqnum) {
-				netlink_io_error(&ctx, &cmd->location,
-						 "Could not process rule: %s",
-						 strerror(err->err));
-				errno = err->err;
-				if (err->seqnum == cmd->seqnum) {
-					mnl_err_list_free(err);
-					break;
-				}
-			}
-		}
-	}
-out:
-	mnl_batch_reset(batch);
-	return ret;
-}
-
-int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-	    void *scanner, struct parser_state *state,
-	    struct list_head *msgs)
-{
-	struct cmd *cmd, *next;
-	int ret;
-
-	ret = nft_parse(nft, scanner, state);
-	if (ret != 0 || state->nerrs > 0) {
-		ret = -1;
-		goto err1;
-	}
-
-	list_for_each_entry(cmd, &state->cmds, list)
-		nft_cmd_expand(cmd);
-
-	ret = nft_netlink(nft, state, msgs, nf_sock);
-err1:
-	list_for_each_entry_safe(cmd, next, &state->cmds, list) {
-		list_del(&cmd->list);
-		cmd_free(cmd);
-	}
-
-	return ret;
-}
-
-static void nft_init(void)
-{
-	mark_table_init();
-	realm_table_rt_init();
-	devgroup_table_init();
-	realm_table_meta_init();
-	ct_label_table_init();
-	gmp_init();
-#ifdef HAVE_LIBXTABLES
-	xt_init();
-#endif
-}
-
-static void nft_exit(void)
-{
-	ct_label_table_exit();
-	realm_table_rt_exit();
-	devgroup_table_exit();
-	realm_table_meta_exit();
-	mark_table_exit();
-}
-
-static void nft_ctx_netlink_init(struct nft_ctx *ctx)
-{
-	ctx->nf_sock = netlink_open_sock();
-}
-
-static struct nft_ctx *nft_ctx_new(uint32_t flags)
-{
-	struct nft_ctx *ctx;
-
-	nft_init();
-	ctx = xzalloc(sizeof(struct nft_ctx));
-
-	ctx->include_paths[0]	= DEFAULT_INCLUDE_PATH;
-	ctx->num_include_paths	= 1;
-	ctx->parser_max_errors	= 10;
-	init_list_head(&ctx->cache.list);
-	ctx->flags = flags;
-
-	if (flags == NFT_CTX_DEFAULT)
-		nft_ctx_netlink_init(ctx);
-
-	return ctx;
-}
-
-static void nft_ctx_free(struct nft_ctx *ctx)
-{
-	if (ctx->nf_sock)
-		netlink_close_sock(ctx->nf_sock);
-
-	iface_cache_release();
-	cache_release(&ctx->cache);
-	xfree(ctx);
-	nft_exit();
-}
-
-static FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
-{
-	FILE *old = ctx->output.output_fp;
-
-	ctx->output.output_fp = fp;
-
-	return old;
-}
-
-static int nft_run_cmd_from_buffer(struct nft_ctx *nft,
-				   char *buf, size_t buflen)
-{
-	int rc = NFT_EXIT_SUCCESS;
-	struct parser_state state;
-	LIST_HEAD(msgs);
-	void *scanner;
-	FILE *fp;
-
-	parser_init(nft->nf_sock, &nft->cache, &state,
-		    &msgs, nft->debug_mask, &nft->output);
-	scanner = scanner_init(&state);
-	scanner_push_buffer(scanner, &indesc_cmdline, buf);
-
-	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
-
-	fp = nft_ctx_set_output(nft, stderr);
-	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	nft_ctx_set_output(nft, fp);
-	scanner_destroy(scanner);
-
-	return rc;
-}
-
-static int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
-{
-	struct parser_state state;
-	LIST_HEAD(msgs);
-	void *scanner;
-	int rc;
-	FILE *fp;
-
-	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
-			  nft->debug_mask, &nft->output);
-	if (rc < 0)
-		return NFT_EXIT_FAILURE;
-
-	parser_init(nft->nf_sock, &nft->cache, &state,
-		    &msgs, nft->debug_mask, &nft->output);
-	scanner = scanner_init(&state);
-	if (scanner_read_file(scanner, filename, &internal_location) < 0) {
-		rc = NFT_EXIT_FAILURE;
-		goto err;
-	}
-
-	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
-err:
-	fp = nft_ctx_set_output(nft, stderr);
-	erec_print_list(&nft->output, &msgs, nft->debug_mask);
-	nft_ctx_set_output(nft, fp);
-	scanner_destroy(scanner);
-
-	return rc;
-}
-
-int nft_print(struct output_ctx *octx, const char *fmt, ...)
-{
-	int ret;
-	va_list arg;
-
-	if (!octx->output_fp)
-		return -1;
-
-	va_start(arg, fmt);
-	ret = vfprintf(octx->output_fp, fmt, arg);
-	va_end(arg);
-	fflush(octx->output_fp);
-
-	return ret;
-}
-
-int nft_gmp_print(struct output_ctx *octx, const char *fmt, ...)
-{
-	int ret;
-	va_list arg;
-
-	if (!octx->output_fp)
-		return -1;
-
-	va_start(arg, fmt);
-	ret = gmp_vfprintf(octx->output_fp, fmt, arg);
-	va_end(arg);
-	fflush(octx->output_fp);
-
-	return ret;
-}
-
 int main(int argc, char * const *argv)
 {
 	char *buf = NULL, *filename = NULL;
-- 
2.13.1

Re: [nft PATCH 2/7] libnftables: Move library stuff out of main.c

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:42AM +0200, Phil Sutter wrote:
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/Makefile.am          |   3 +-
>  include/nftables.h           |  65 +----------
>  include/nftables/Makefile.am |   1 +
>  include/nftables/nftables.h  |  88 +++++++++++++++
>  src/Makefile.am              |   3 +-
>  src/libnftables.c            | 261 +++++++++++++++++++++++++++++++++++++++++++
>  src/main.c                   | 253 +----------------------------------------
>  7 files changed, 356 insertions(+), 318 deletions(-)
>  create mode 100644 include/nftables/Makefile.am
>  create mode 100644 include/nftables/nftables.h
>  create mode 100644 src/libnftables.c
> 
> diff --git a/include/Makefile.am b/include/Makefile.am
> index 5dd73d81f427e..a74ffbfa8de0a 100644
> --- a/include/Makefile.am
> +++ b/include/Makefile.am
> @@ -1,4 +1,5 @@
> -SUBDIRS = linux
> +SUBDIRS =		linux		\
> +			nftables
>  
>  noinst_HEADERS = 	cli.h		\
>  			datatype.h	\
> diff --git a/include/nftables.h b/include/nftables.h
> index 01d72a87212ea..a633e1a2cc2e2 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -4,63 +4,7 @@
>  #include <stdbool.h>
>  #include <stdarg.h>
>  #include <utils.h>
> -
> -enum numeric_level {
> -	NUMERIC_NONE,
> -	NUMERIC_ADDR,
> -	NUMERIC_PORT,
> -	NUMERIC_ALL,
> -};
> -
> -enum debug_level {
> -	DEBUG_SCANNER		= 0x1,
> -	DEBUG_PARSER		= 0x2,
> -	DEBUG_EVALUATION	= 0x4,
> -	DEBUG_NETLINK		= 0x8,
> -	DEBUG_MNL		= 0x10,
> -	DEBUG_PROTO_CTX		= 0x20,
> -	DEBUG_SEGTREE		= 0x40,
> -};
> -
> -#define INCLUDE_PATHS_MAX	16
> -
> -struct output_ctx {
> -	unsigned int numeric;
> -	unsigned int stateless;
> -	unsigned int ip2name;
> -	unsigned int handle;
> -	unsigned int echo;
> -	FILE *output_fp;
> -};
> -
> -struct nft_cache {
> -	bool			initialized;
> -	struct list_head	list;
> -	uint32_t		seqnum;
> -};
> -
> -struct mnl_socket;
> -
> -struct nft_ctx {
> -	struct mnl_socket	*nf_sock;
> -	const char		*include_paths[INCLUDE_PATHS_MAX];
> -	unsigned int		num_include_paths;
> -	unsigned int		parser_max_errors;
> -	unsigned int		debug_mask;
> -	struct output_ctx	output;
> -	bool			check;
> -	struct nft_cache	cache;
> -	uint32_t		flags;
> -};
> -
> -#define NFT_CTX_DEFAULT		0
> -
> -enum nftables_exit_codes {
> -	NFT_EXIT_SUCCESS	= 0,
> -	NFT_EXIT_FAILURE	= 1,
> -	NFT_EXIT_NOMEM		= 2,
> -	NFT_EXIT_NONL		= 3,
> -};
> +#include <nftables/nftables.h>
>  
>  struct input_descriptor;
>  struct location {
> @@ -128,13 +72,6 @@ struct input_descriptor {
>  	off_t				line_offset;
>  };
>  
> -struct parser_state;
> -struct mnl_socket;
> -
> -int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
> -	    void *scanner, struct parser_state *state,
> -	    struct list_head *msgs);
> -
>  void ct_label_table_init(void);
>  void mark_table_init(void);
>  void gmp_init(void);
> diff --git a/include/nftables/Makefile.am b/include/nftables/Makefile.am
> new file mode 100644
> index 0000000000000..9e31d519599c1
> --- /dev/null
> +++ b/include/nftables/Makefile.am
> @@ -0,0 +1 @@
> +noinst_HEADERS = nftables.h
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> new file mode 100644
> index 0000000000000..052a77bfb5371
> --- /dev/null
> +++ b/include/nftables/nftables.h

Is this nftables/nftables.h file what we will expose later on as
header for this library?

If so... see below.

> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (c) 2017 Eric Leblond <eric@regit.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#ifndef LIB_NFTABLES_H
> +#define LIB_NFTABLES_H
> +
> +struct parser_state;
> +struct mnl_socket;
> +
> +struct nft_cache {
> +	bool			initialized;
> +	struct list_head	list;
> +	uint32_t		seqnum;
> +};
> +
> +#define INCLUDE_PATHS_MAX	16
> +
> +struct output_ctx {
> +	unsigned int numeric;
> +	unsigned int stateless;
> +	unsigned int ip2name;
> +	unsigned int handle;
> +	unsigned int echo;
> +	FILE *output_fp;
> +};

I think these structure should be just like:

struct output_ctx;

as a forward declaration. So we enforce users to use getters and
setters.

So we can just move easily in a follow up patch to expose the library
API to everyone, right?

Thanks.

Re: [nft PATCH 2/7] libnftables: Move library stuff out of main.c

[Phil Sutter]

Hi,

On Fri, Oct 20, 2017 at 02:12:02PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 19, 2017 at 10:18:42AM +0200, Phil Sutter wrote:
[...]
> > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> > new file mode 100644
> > index 0000000000000..052a77bfb5371
> > --- /dev/null
> > +++ b/include/nftables/nftables.h
> 
> Is this nftables/nftables.h file what we will expose later on as
> header for this library?

Yes, exactly.

[...]
> > @@ -0,0 +1,88 @@
> > +/*
> > + * Copyright (c) 2017 Eric Leblond <eric@regit.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +#ifndef LIB_NFTABLES_H
> > +#define LIB_NFTABLES_H
> > +
> > +struct parser_state;
> > +struct mnl_socket;
> > +
> > +struct nft_cache {
> > +	bool			initialized;
> > +	struct list_head	list;
> > +	uint32_t		seqnum;
> > +};
> > +
> > +#define INCLUDE_PATHS_MAX	16
> > +
> > +struct output_ctx {
> > +	unsigned int numeric;
> > +	unsigned int stateless;
> > +	unsigned int ip2name;
> > +	unsigned int handle;
> > +	unsigned int echo;
> > +	FILE *output_fp;
> > +};
> 
> I think these structure should be just like:
> 
> struct output_ctx;
> 
> as a forward declaration. So we enforce users to use getters and
> setters.

Ultimately, I want to forward-declare struct nft_ctx as a whole. Is this
fine with you (also from advanced API point of view)?

Thanks, Phil

Re: [nft PATCH 2/7] libnftables: Move library stuff out of main.c

[Pablo Neira Ayuso]

On Fri, Oct 20, 2017 at 07:02:13PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Oct 20, 2017 at 02:12:02PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 19, 2017 at 10:18:42AM +0200, Phil Sutter wrote:
> [...]
> > > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> > > new file mode 100644
> > > index 0000000000000..052a77bfb5371
> > > --- /dev/null
> > > +++ b/include/nftables/nftables.h
> > 
> > Is this nftables/nftables.h file what we will expose later on as
> > header for this library?
> 
> Yes, exactly.
> 
> [...]
> > > @@ -0,0 +1,88 @@
> > > +/*
> > > + * Copyright (c) 2017 Eric Leblond <eric@regit.org>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + */
> > > +#ifndef LIB_NFTABLES_H
> > > +#define LIB_NFTABLES_H
> > > +
> > > +struct parser_state;
> > > +struct mnl_socket;
> > > +
> > > +struct nft_cache {
> > > +	bool			initialized;
> > > +	struct list_head	list;
> > > +	uint32_t		seqnum;
> > > +};
> > > +
> > > +#define INCLUDE_PATHS_MAX	16
> > > +
> > > +struct output_ctx {
> > > +	unsigned int numeric;
> > > +	unsigned int stateless;
> > > +	unsigned int ip2name;
> > > +	unsigned int handle;
> > > +	unsigned int echo;
> > > +	FILE *output_fp;
> > > +};
> > 
> > I think these structure should be just like:
> > 
> > struct output_ctx;
> > 
> > as a forward declaration. So we enforce users to use getters and
> > setters.
> 
> Ultimately, I want to forward-declare struct nft_ctx as a whole. Is this
> fine with you (also from advanced API point of view)?

Yes. No layout exposes, so we can freely changed them in the future.

[nft PATCH 3/7] libnftables: Introduce nft_ctx_flush_cache()

[Phil Sutter]

This allows an application to explicitly flush caches associated with a
given nft context.

Note that this is a bit inconsistent in that it releases the global
interface cache, but nft_ctx_free() does the same so at least it's not a
regression.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables/nftables.h | 1 +
 src/libnftables.c           | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 052a77bfb5371..fbc6fd4252a97 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -77,6 +77,7 @@ enum nftables_exit_codes {
 struct nft_ctx *nft_ctx_new(uint32_t flags);
 void nft_ctx_free(struct nft_ctx *ctx);
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
+void nft_ctx_flush_cache(struct nft_ctx *ctx);
 
 int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
 	    void *scanner, struct parser_state *state,
diff --git a/src/libnftables.c b/src/libnftables.c
index 187747c66af21..0de50c854d572 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -146,13 +146,18 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	return ctx;
 }
 
+void nft_ctx_flush_cache(struct nft_ctx *ctx)
+{
+	iface_cache_release();
+	cache_release(&ctx->cache);
+}
+
 void nft_ctx_free(struct nft_ctx *ctx)
 {
 	if (ctx->nf_sock)
 		netlink_close_sock(ctx->nf_sock);
 
-	iface_cache_release();
-	cache_release(&ctx->cache);
+	nft_ctx_flush_cache(ctx);
 	xfree(ctx);
 	nft_exit();
 }
-- 
2.13.1

Re: [nft PATCH 3/7] libnftables: Introduce nft_ctx_flush_cache()

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote:
> This allows an application to explicitly flush caches associated with a
> given nft context.
> 
> Note that this is a bit inconsistent in that it releases the global
> interface cache, but nft_ctx_free() does the same so at least it's not a
> regression.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/nftables/nftables.h | 1 +
>  src/libnftables.c           | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> index 052a77bfb5371..fbc6fd4252a97 100644
> --- a/include/nftables/nftables.h
> +++ b/include/nftables/nftables.h
> @@ -77,6 +77,7 @@ enum nftables_exit_codes {
>  struct nft_ctx *nft_ctx_new(uint32_t flags);
>  void nft_ctx_free(struct nft_ctx *ctx);
>  FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
> +void nft_ctx_flush_cache(struct nft_ctx *ctx);
>  
>  int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
>  	    void *scanner, struct parser_state *state,
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 187747c66af21..0de50c854d572 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -146,13 +146,18 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
>  	return ctx;
>  }
>  
> +void nft_ctx_flush_cache(struct nft_ctx *ctx)
> +{
> +	iface_cache_release();
> +	cache_release(&ctx->cache);
> +}

This flush allows us to release the cache, but nft_ctx_alloc()
populates it. I'm missing something here, can we force a context
repopulation?

If there is no usecase for this yet, I would keep this behind by now.

Re: [nft PATCH 3/7] libnftables: Introduce nft_ctx_flush_cache()

[Phil Sutter]

Hi,

On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote:
[...]
> > +void nft_ctx_flush_cache(struct nft_ctx *ctx)
> > +{
> > +	iface_cache_release();
> > +	cache_release(&ctx->cache);
> > +}
> 
> This flush allows us to release the cache, but nft_ctx_alloc()
> populates it. I'm missing something here, can we force a context
> repopulation?

No, nft_ctx_alloc() does not populate the cache, but just initialize
cache list head (which is not undone by cache_release()). Cache
population happens during command execution depending on whether a cache
is needed or not.

> If there is no usecase for this yet, I would keep this behind by now.

The use-case for the above is cli_complete(), which
explicitly drops the cache after execution of every command (probably
because it's potentially long-lived and therefore things might change in
background).

Cheers, Phil

Re: [nft PATCH 3/7] libnftables: Introduce nft_ctx_flush_cache()

[Pablo Neira Ayuso]

On Fri, Oct 20, 2017 at 07:05:13PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote:
> [...]
> > > +void nft_ctx_flush_cache(struct nft_ctx *ctx)
> > > +{
> > > +	iface_cache_release();
> > > +	cache_release(&ctx->cache);
> > > +}
> > 
> > This flush allows us to release the cache, but nft_ctx_alloc()
> > populates it. I'm missing something here, can we force a context
> > repopulation?
> 
> No, nft_ctx_alloc() does not populate the cache, but just initialize
> cache list head (which is not undone by cache_release()). Cache
> population happens during command execution depending on whether a cache
> is needed or not.

I see.

I think cache population should happen from nft_ctx_alloc(), caches
are context after all.

> > If there is no usecase for this yet, I would keep this behind by now.
> 
> The use-case for the above is cli_complete(), which
> explicitly drops the cache after execution of every command (probably
> because it's potentially long-lived and therefore things might change in
> background).

I see. If we follow the approach I'm describe above, then we need
something like nft_ctx_reset(), where we reset all context and we get
a fresh cache.

Makes sense to you?

Re: [nft PATCH 3/7] libnftables: Introduce nft_ctx_flush_cache()

[Phil Sutter]

On Fri, Oct 20, 2017 at 09:10:31PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 20, 2017 at 07:05:13PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Fri, Oct 20, 2017 at 02:13:26PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 19, 2017 at 10:18:43AM +0200, Phil Sutter wrote:
> > [...]
> > > > +void nft_ctx_flush_cache(struct nft_ctx *ctx)
> > > > +{
> > > > +	iface_cache_release();
> > > > +	cache_release(&ctx->cache);
> > > > +}
> > > 
> > > This flush allows us to release the cache, but nft_ctx_alloc()
> > > populates it. I'm missing something here, can we force a context
> > > repopulation?
> > 
> > No, nft_ctx_alloc() does not populate the cache, but just initialize
> > cache list head (which is not undone by cache_release()). Cache
> > population happens during command execution depending on whether a cache
> > is needed or not.
> 
> I see.
> 
> I think cache population should happen from nft_ctx_alloc(), caches
> are context after all.
> 
> > > If there is no usecase for this yet, I would keep this behind by now.
> > 
> > The use-case for the above is cli_complete(), which
> > explicitly drops the cache after execution of every command (probably
> > because it's potentially long-lived and therefore things might change in
> > background).
> 
> I see. If we follow the approach I'm describe above, then we need
> something like nft_ctx_reset(), where we reset all context and we get
> a fresh cache.

I think it's worth keeping the current logic of when to populate the
cache. Remember when I added a call to cache_update() for echo output
support which caused an unnecessary slowdown you later complained about?
Populating the cache unconditionally upon context creation would cause
the same issue again if we follow my plan of making 'nft' binary the
first user of libnftables.

Right now, we only explicitly clear the cache in CLI, and my patch
allows applications to do that if they think it's necessary. Cache
population is completely pointless if an application just wants to e.g.
add a new table to the rule set, so I'd prefer to leave it this way. We
spoke about possible performance implications of cache updates at NFWS
already, keeping the existing (and well defined) logic will at least
limit the impact of this potential bottleneck.

Cheers, Phil

[nft PATCH 4/7] cli: Use nft_run_cmd_from_buffer()

[Phil Sutter]

This simplifies CLI code and allows to reduce libnftables API by not
exporting nft_run().

Since nft_run_cmd_from_buffer() takes care of scanner initialization and
libmnl socket passed to cli_init() is present as nft_ctx field as well,
signature of cli_init() can be reduced to just take nft_ctx pointer as
single argument.

Note that this change introduces two (possibly unwanted) side-effects:

* Input descriptor passed to scanner_push_buffer() is changed from the
  CLI-specific one to the one used by nft_run_cmd_from_buffer().

In practice though, this doesn't make a difference: input descriptor
types INDESC_CLI and INDESC_BUFFER are treated equally by erec_print().
Also, scanner_push_buffer() NULLs input descriptor name, so that is not
used at all in latter code.

* Error messages are printed to stderr instead of cli_nft->output.

This could be fixed by introducing an 'error_output' field in nft_ctx
for nft_run_cmd_from_buffer() to use when printing error messages.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/cli.h               |  6 ++----
 include/nftables/nftables.h |  5 -----
 src/cli.c                   | 24 +++---------------------
 src/libnftables.c           |  6 +++---
 src/main.c                  |  3 +--
 5 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/include/cli.h b/include/cli.h
index 3ae1c459bce2d..3780e0917969d 100644
--- a/include/cli.h
+++ b/include/cli.h
@@ -5,11 +5,9 @@
 
 struct parser_state;
 #ifdef HAVE_LIBREADLINE
-extern int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-		    struct parser_state *state);
+extern int cli_init(struct nft_ctx *nft);
 #else
-static inline int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-			   struct parser_state *state)
+static inline int cli_init(struct nft_ctx *nft)
 {
         return -1;
 }
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index fbc6fd4252a97..b91219d423df9 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -9,7 +9,6 @@
 #ifndef LIB_NFTABLES_H
 #define LIB_NFTABLES_H
 
-struct parser_state;
 struct mnl_socket;
 
 struct nft_cache {
@@ -79,10 +78,6 @@ void nft_ctx_free(struct nft_ctx *ctx);
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
 void nft_ctx_flush_cache(struct nft_ctx *ctx);
 
-int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-	    void *scanner, struct parser_state *state,
-	    struct list_head *msgs);
-
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen);
 int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename);
 
diff --git a/src/cli.c b/src/cli.c
index cadc3af6e8034..37351f2f8b04f 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -35,15 +35,7 @@
 
 #define CMDLINE_HISTFILE	".nft.history"
 
-static const struct input_descriptor indesc_cli = {
-	.type	= INDESC_CLI,
-	.name   = "<cli>",
-};
-
-static struct parser_state *state;
 static struct nft_ctx *cli_nft;
-static struct mnl_socket *cli_nf_sock;
-static void *scanner;
 static char histfile[PATH_MAX];
 static char *multiline;
 static bool eof;
@@ -134,14 +126,9 @@ static void cli_complete(char *line)
 	xfree(line);
 	line = s;
 
-	parser_init(cli_nf_sock, &cli_nft->cache, state, &msgs,
-		    cli_nft->debug_mask, &cli_nft->output);
-	scanner_push_buffer(scanner, &indesc_cli, line);
-	nft_run(cli_nft, cli_nf_sock, scanner, state, &msgs);
-	erec_print_list(&cli_nft->output, &msgs, cli_nft->debug_mask);
+	nft_run_cmd_from_buffer(cli_nft, line, len + 2);
 	xfree(line);
-	cache_release(&cli_nft->cache);
-	iface_cache_release();
+	nft_ctx_flush_cache(cli_nft);
 }
 
 static char **cli_completion(const char *text, int start, int end)
@@ -149,12 +136,10 @@ static char **cli_completion(const char *text, int start, int end)
 	return NULL;
 }
 
-int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-	     struct parser_state *_state)
+int cli_init(struct nft_ctx *nft)
 {
 	const char *home;
 
-	cli_nf_sock = nf_sock;
 	cli_nft = nft;
 	rl_readline_name = "nft";
 	rl_instream  = stdin;
@@ -171,9 +156,6 @@ int cli_init(struct nft_ctx *nft, struct mnl_socket *nf_sock,
 	read_history(histfile);
 	history_set_pos(history_length);
 
-	state	= _state;
-	scanner = scanner_init(state);
-
 	while (!eof)
 		rl_callback_read_char();
 	return 0;
diff --git a/src/libnftables.c b/src/libnftables.c
index 0de50c854d572..d88c299c3647e 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -74,9 +74,9 @@ out:
 	return ret;
 }
 
-int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
-	    void *scanner, struct parser_state *state,
-	    struct list_head *msgs)
+static int nft_run(struct nft_ctx *nft, struct mnl_socket *nf_sock,
+		   void *scanner, struct parser_state *state,
+		   struct list_head *msgs)
 {
 	struct cmd *cmd, *next;
 	int ret;
diff --git a/src/main.c b/src/main.c
index a7da460f28ca5..3c107181305c7 100644
--- a/src/main.c
+++ b/src/main.c
@@ -168,7 +168,6 @@ int main(int argc, char * const *argv)
 	char *buf = NULL, *filename = NULL;
 	unsigned int len;
 	bool interactive = false;
-	struct parser_state state;
 	int i, val, rc;
 
 	nft = nft_ctx_new(NFT_CTX_DEFAULT);
@@ -272,7 +271,7 @@ int main(int argc, char * const *argv)
 	} else if (filename != NULL) {
 		rc = nft_run_cmd_from_filename(nft, filename);
 	} else if (interactive) {
-		if (cli_init(nft, nft->nf_sock, &state) < 0) {
+		if (cli_init(nft) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
 			exit(NFT_EXIT_FAILURE);
-- 
2.13.1

Re: [nft PATCH 4/7] cli: Use nft_run_cmd_from_buffer()

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote:
> This simplifies CLI code and allows to reduce libnftables API by not
> exporting nft_run().
> 
> Since nft_run_cmd_from_buffer() takes care of scanner initialization and
> libmnl socket passed to cli_init() is present as nft_ctx field as well,
> signature of cli_init() can be reduced to just take nft_ctx pointer as
> single argument.

libmnl socket is indeed in nft_ctx, but we're planning a mode that
allows to expose the mnl_socket for advanced handling. In that
scenario, nft->nf_sock will be null.

So I would prefer we don't do changes that we have to undo once the
advanced API is in place.

Re: [nft PATCH 4/7] cli: Use nft_run_cmd_from_buffer()

[Phil Sutter]

On Fri, Oct 20, 2017 at 02:15:34PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote:
> > This simplifies CLI code and allows to reduce libnftables API by not
> > exporting nft_run().
> > 
> > Since nft_run_cmd_from_buffer() takes care of scanner initialization and
> > libmnl socket passed to cli_init() is present as nft_ctx field as well,
> > signature of cli_init() can be reduced to just take nft_ctx pointer as
> > single argument.
> 
> libmnl socket is indeed in nft_ctx, but we're planning a mode that
> allows to expose the mnl_socket for advanced handling. In that
> scenario, nft->nf_sock will be null.
> 
> So I would prefer we don't do changes that we have to undo once the
> advanced API is in place.

IMHO this doesn't contradict what the patch does. Right now we only have
the "simple API", and the patch changes src/cli.c to use just that. CLI
code doesn't need anything which is not fulfilled by simple API at this
point, so I'd say changing it to use advanced API should be done when we
implement features (e.g. transaction control) there.

What do you think?

Cheers, Phil

Re: [nft PATCH 4/7] cli: Use nft_run_cmd_from_buffer()

[Pablo Neira Ayuso]

On Fri, Oct 20, 2017 at 07:10:18PM +0200, Phil Sutter wrote:
> On Fri, Oct 20, 2017 at 02:15:34PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote:
> > > This simplifies CLI code and allows to reduce libnftables API by not
> > > exporting nft_run().
> > > 
> > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and
> > > libmnl socket passed to cli_init() is present as nft_ctx field as well,
> > > signature of cli_init() can be reduced to just take nft_ctx pointer as
> > > single argument.
> > 
> > libmnl socket is indeed in nft_ctx, but we're planning a mode that
> > allows to expose the mnl_socket for advanced handling. In that
> > scenario, nft->nf_sock will be null.
> > 
> > So I would prefer we don't do changes that we have to undo once the
> > advanced API is in place.
> 
> IMHO this doesn't contradict what the patch does. Right now we only have
> the "simple API", and the patch changes src/cli.c to use just that. CLI
> code doesn't need anything which is not fulfilled by simple API at this
> point, so I'd say changing it to use advanced API should be done when we
> implement features (e.g. transaction control) there.
> 
> What do you think?

I have no strong objection against this, I just would like we don't
lose track of the high level API, and that one will need to expose the
netlink socket. So all these calls we will end up needed the nf_sock
parameter again at some point.

I don't have any strong opinion against this, just an observation.

Re: [nft PATCH 4/7] cli: Use nft_run_cmd_from_buffer()

[Phil Sutter]

On Fri, Oct 20, 2017 at 09:18:07PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 20, 2017 at 07:10:18PM +0200, Phil Sutter wrote:
> > On Fri, Oct 20, 2017 at 02:15:34PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 19, 2017 at 10:18:44AM +0200, Phil Sutter wrote:
> > > > This simplifies CLI code and allows to reduce libnftables API by not
> > > > exporting nft_run().
> > > > 
> > > > Since nft_run_cmd_from_buffer() takes care of scanner initialization and
> > > > libmnl socket passed to cli_init() is present as nft_ctx field as well,
> > > > signature of cli_init() can be reduced to just take nft_ctx pointer as
> > > > single argument.
> > > 
> > > libmnl socket is indeed in nft_ctx, but we're planning a mode that
> > > allows to expose the mnl_socket for advanced handling. In that
> > > scenario, nft->nf_sock will be null.
> > > 
> > > So I would prefer we don't do changes that we have to undo once the
> > > advanced API is in place.
> > 
> > IMHO this doesn't contradict what the patch does. Right now we only have
> > the "simple API", and the patch changes src/cli.c to use just that. CLI
> > code doesn't need anything which is not fulfilled by simple API at this
> > point, so I'd say changing it to use advanced API should be done when we
> > implement features (e.g. transaction control) there.
> > 
> > What do you think?
> 
> I have no strong objection against this, I just would like we don't
> lose track of the high level API, and that one will need to expose the
> netlink socket. So all these calls we will end up needed the nf_sock
> parameter again at some point.
> 
> I don't have any strong opinion against this, just an observation.

I'd suggest to review things again once we have a common view of how the
advanced API will look like. Right now, CLI is fine with using the
simple API as proposed by my patches, so passing it ctx->nf_sock
separately just to have it either ignore it or reassign it to
ctx->nf_sock again doesn't make sense to me.

Cheers, Phil

[nft PATCH 5/7] libnftables: Introduce nft_ctx_set_dry_run()

[Phil Sutter]

Provide a convenient interface to configure dry run mode.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables/nftables.h | 3 +++
 src/libnftables.c           | 5 +++++
 src/main.c                  | 2 +-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index b91219d423df9..f0c9bbf3ba3fe 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -75,7 +75,10 @@ enum nftables_exit_codes {
 
 struct nft_ctx *nft_ctx_new(uint32_t flags);
 void nft_ctx_free(struct nft_ctx *ctx);
+
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
+void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
+
 void nft_ctx_flush_cache(struct nft_ctx *ctx);
 
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen);
diff --git a/src/libnftables.c b/src/libnftables.c
index d88c299c3647e..817f537e32618 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -171,6 +171,11 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
 	return old;
 }
 
+void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry)
+{
+	ctx->check = dry;
+}
+
 static const struct input_descriptor indesc_cmdline = {
 	.type	= INDESC_BUFFER,
 	.name	= "<cmdline>",
diff --git a/src/main.c b/src/main.c
index 3c107181305c7..8359367b78654 100644
--- a/src/main.c
+++ b/src/main.c
@@ -187,7 +187,7 @@ int main(int argc, char * const *argv)
 			       PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME);
 			exit(NFT_EXIT_SUCCESS);
 		case OPT_CHECK:
-			nft->check = true;
+			nft_ctx_set_dry_run(nft, true);
 			break;
 		case OPT_FILE:
 			filename = optarg;
-- 
2.13.1

[nft PATCH 6/7] libnftables: Provide an API for include path handling

[Phil Sutter]

In order to keep the API simple, remove INCLUDE_PATHS_MAX restraint and
dynamically allocate nft_ctx field include_paths instead.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables/nftables.h |  6 +++---
 src/libnftables.c           | 34 ++++++++++++++++++++++++++++++++--
 src/main.c                  |  9 ++++-----
 src/scanner.l               |  4 +---
 4 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index f0c9bbf3ba3fe..a752f20d74132 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -17,8 +17,6 @@ struct nft_cache {
 	uint32_t		seqnum;
 };
 
-#define INCLUDE_PATHS_MAX	16
-
 struct output_ctx {
 	unsigned int numeric;
 	unsigned int stateless;
@@ -30,7 +28,7 @@ struct output_ctx {
 
 struct nft_ctx {
 	struct mnl_socket	*nf_sock;
-	const char		*include_paths[INCLUDE_PATHS_MAX];
+	char			**include_paths;
 	unsigned int		num_include_paths;
 	unsigned int		parser_max_errors;
 	unsigned int		debug_mask;
@@ -78,6 +76,8 @@ void nft_ctx_free(struct nft_ctx *ctx);
 
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
 void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
+int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path);
+void nft_ctx_clear_include_paths(struct nft_ctx *ctx);
 
 void nft_ctx_flush_cache(struct nft_ctx *ctx);
 
diff --git a/src/libnftables.c b/src/libnftables.c
index 817f537e32618..2f4275c9a0a94 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -6,10 +6,13 @@
  * published by the Free Software Foundation.
  *
  */
+#define _GNU_SOURCE
 #include <erec.h>
 #include <errno.h>
 #include <mnl.h>
 #include <parser.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <utils.h>
 #include <iface.h>
@@ -122,6 +125,33 @@ static void nft_exit(void)
 	mark_table_exit();
 }
 
+int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
+{
+	char **tmp;
+	int pcount = ctx->num_include_paths;
+
+	tmp = realloc(ctx->include_paths, (pcount + 1) * sizeof(char *));
+	if (!tmp)
+		return -1;
+
+	ctx->include_paths = tmp;
+
+	if (asprintf(&ctx->include_paths[pcount], "%s", path) < 0)
+		return -1;
+
+	ctx->num_include_paths++;
+	return 0;
+}
+
+void nft_ctx_clear_include_paths(struct nft_ctx *ctx)
+{
+	while (ctx->num_include_paths)
+		xfree(ctx->include_paths[--ctx->num_include_paths]);
+
+	xfree(ctx->include_paths);
+	ctx->include_paths = NULL;
+}
+
 static void nft_ctx_netlink_init(struct nft_ctx *ctx)
 {
 	ctx->nf_sock = netlink_open_sock();
@@ -134,8 +164,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
 	nft_init();
 	ctx = xzalloc(sizeof(struct nft_ctx));
 
-	ctx->include_paths[0]	= DEFAULT_INCLUDE_PATH;
-	ctx->num_include_paths	= 1;
+	nft_ctx_add_include_path(ctx, DEFAULT_INCLUDE_PATH);
 	ctx->parser_max_errors	= 10;
 	init_list_head(&ctx->cache.list);
 	ctx->flags = flags;
@@ -158,6 +187,7 @@ void nft_ctx_free(struct nft_ctx *ctx)
 		netlink_close_sock(ctx->nf_sock);
 
 	nft_ctx_flush_cache(ctx);
+	nft_ctx_clear_include_paths(ctx);
 	xfree(ctx);
 	nft_exit();
 }
diff --git a/src/main.c b/src/main.c
index 8359367b78654..de5c115757f44 100644
--- a/src/main.c
+++ b/src/main.c
@@ -196,13 +196,12 @@ int main(int argc, char * const *argv)
 			interactive = true;
 			break;
 		case OPT_INCLUDEPATH:
-			if (nft->num_include_paths >= INCLUDE_PATHS_MAX) {
-				fprintf(stderr, "Too many include paths "
-						"specified, max. %u\n",
-					INCLUDE_PATHS_MAX - 1);
+			if (nft_ctx_add_include_path(nft, optarg)) {
+				fprintf(stderr,
+					"Failed to add include path '%s'\n",
+					optarg);
 				exit(NFT_EXIT_FAILURE);
 			}
-			nft->include_paths[nft->num_include_paths++] = optarg;
 			break;
 		case OPT_NUMERIC:
 			if (++nft->output.numeric > NUMERIC_ALL) {
diff --git a/src/scanner.l b/src/scanner.l
index 594073660c6b1..ee09775ebf1d9 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -794,9 +794,7 @@ int scanner_include_file(struct nft_ctx *nft, void *scanner,
 	int ret = -1;
 
 	if (search_in_include_path(filename)) {
-		for (i = 0; i < INCLUDE_PATHS_MAX; i++) {
-			if (nft->include_paths[i] == NULL)
-				break;
+		for (i = 0; i < nft->num_include_paths; i++) {
 			ret = snprintf(buf, sizeof(buf), "%s/%s",
 				       nft->include_paths[i], filename);
 			if (ret < 0 || ret >= PATH_MAX) {
-- 
2.13.1

Re: [nft PATCH 6/7] libnftables: Provide an API for include path handling

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote:
> In order to keep the API simple, remove INCLUDE_PATHS_MAX restraint and
> dynamically allocate nft_ctx field include_paths instead.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/nftables/nftables.h |  6 +++---
>  src/libnftables.c           | 34 ++++++++++++++++++++++++++++++++--
>  src/main.c                  |  9 ++++-----
>  src/scanner.l               |  4 +---
>  4 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> index f0c9bbf3ba3fe..a752f20d74132 100644
> --- a/include/nftables/nftables.h
> +++ b/include/nftables/nftables.h
> @@ -17,8 +17,6 @@ struct nft_cache {
>  	uint32_t		seqnum;
>  };
>  
> -#define INCLUDE_PATHS_MAX	16
> -
>  struct output_ctx {
>  	unsigned int numeric;
>  	unsigned int stateless;
> @@ -30,7 +28,7 @@ struct output_ctx {
>  
>  struct nft_ctx {
>  	struct mnl_socket	*nf_sock;
> -	const char		*include_paths[INCLUDE_PATHS_MAX];
> +	char			**include_paths;
>  	unsigned int		num_include_paths;
>  	unsigned int		parser_max_errors;
>  	unsigned int		debug_mask;
> @@ -78,6 +76,8 @@ void nft_ctx_free(struct nft_ctx *ctx);
>  
>  FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
>  void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
> +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path);
> +void nft_ctx_clear_include_paths(struct nft_ctx *ctx);
>  
>  void nft_ctx_flush_cache(struct nft_ctx *ctx);
>  
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 817f537e32618..2f4275c9a0a94 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -6,10 +6,13 @@
>   * published by the Free Software Foundation.
>   *
>   */
> +#define _GNU_SOURCE
>  #include <erec.h>
>  #include <errno.h>
>  #include <mnl.h>
>  #include <parser.h>
> +#include <stdio.h>
> +#include <stdlib.h>
>  #include <string.h>
>  #include <utils.h>
>  #include <iface.h>
> @@ -122,6 +125,33 @@ static void nft_exit(void)
>  	mark_table_exit();
>  }
>  
> +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)

Do we want to accept runtime addition/removal of include paths?

I mean, I would just make it nft_ctx_set_include_path(), then add an
unsetter, so we simplify this.

Let me know if I'm overlooking anything, thanks.

Re: [nft PATCH 6/7] libnftables: Provide an API for include path handling

[Phil Sutter]

Hi,

On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote:
[...]
> > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
> 
> Do we want to accept runtime addition/removal of include paths?

Not necessarily, but src/main.c does just that: It calls nft_ctx_new()
first, then adds include paths as it parses them from command line.

> I mean, I would just make it nft_ctx_set_include_path(), then add an
> unsetter, so we simplify this.

The counterpart to nft_ctx_add_include_path() is
nft_ctx_clear_include_paths(), which just drops all the previously set
ones. Does that meet your understanding of an unsetter, or am I missing
something?

The reason why this patch is a bit more complicated is because I wanted
to get rid of the hard upper limit of include paths to avoid introducing
a getter for number of set include paths or to make it necessary for
applications (read: src/main.c) to check what return code
nft_ctx_add_include_path() returned to print a reasonable error message.

Cheers, Phil

Re: [nft PATCH 6/7] libnftables: Provide an API for include path handling

[Pablo Neira Ayuso]

On Fri, Oct 20, 2017 at 07:16:20PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote:
> [...]
> > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
> > 
> > Do we want to accept runtime addition/removal of include paths?
> 
> Not necessarily, but src/main.c does just that: It calls nft_ctx_new()
> first, then adds include paths as it parses them from command line.

So it's more like a one time call to set up the include path, right?
So I think semantically this is just another setter. This _add_ name
made me think you can keep adding including path one after another
anytime.

> > I mean, I would just make it nft_ctx_set_include_path(), then add an
> > unsetter, so we simplify this.
> 
> The counterpart to nft_ctx_add_include_path() is
> nft_ctx_clear_include_paths(), which just drops all the previously set
> ones. Does that meet your understanding of an unsetter, or am I missing
> something?

Do we have a usecase for nft_ctx_clear_include_paths(). If we don't
- I don't see any at least from my side - I'd prefer, to keep it back.

> The reason why this patch is a bit more complicated is because I wanted
> to get rid of the hard upper limit of include paths to avoid introducing
> a getter for number of set include paths or to make it necessary for
> applications (read: src/main.c) to check what return code
> nft_ctx_add_include_path() returned to print a reasonable error message.

I'm fine with removing the upper limit, but that is a different thing.
My only concerns are related to the API we provide to set include
paths.

Re: [nft PATCH 6/7] libnftables: Provide an API for include path handling

[Phil Sutter]

On Fri, Oct 20, 2017 at 09:16:43PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 20, 2017 at 07:16:20PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote:
> > [...]
> > > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
> > > 
> > > Do we want to accept runtime addition/removal of include paths?
> > 
> > Not necessarily, but src/main.c does just that: It calls nft_ctx_new()
> > first, then adds include paths as it parses them from command line.
> 
> So it's more like a one time call to set up the include path, right?
> So I think semantically this is just another setter. This _add_ name
> made me think you can keep adding including path one after another
> anytime.

Yes, the API (or specifically, nft_ctx_add_include_path()) allows that.
The only alternative I could think of would be to introduce something
like:

| int nft_ctx_set_include_paths(struct nft_ctx *ctx, const char **paths)

Which means src/main.c would have to take care of populating the char **
array itself in order to later pass it in one go to the setter. Fine
with me, you decide! :)

> > > I mean, I would just make it nft_ctx_set_include_path(), then add an
> > > unsetter, so we simplify this.
> > 
> > The counterpart to nft_ctx_add_include_path() is
> > nft_ctx_clear_include_paths(), which just drops all the previously set
> > ones. Does that meet your understanding of an unsetter, or am I missing
> > something?
> 
> Do we have a usecase for nft_ctx_clear_include_paths(). If we don't
> - I don't see any at least from my side - I'd prefer, to keep it back.

It's only used in nft_ctx_free() for now, just because it's convenient.
If you don't want to export it (yet), I can make it static so code
readability is kept but it won't be available to applications.

> > The reason why this patch is a bit more complicated is because I wanted
> > to get rid of the hard upper limit of include paths to avoid introducing
> > a getter for number of set include paths or to make it necessary for
> > applications (read: src/main.c) to check what return code
> > nft_ctx_add_include_path() returned to print a reasonable error message.
> 
> I'm fine with removing the upper limit, but that is a different thing.
> My only concerns are related to the API we provide to set include
> paths.

OK, cool. So we only have to agree about above items.

Cheers, Phil

[nft PATCH 7/7] libnftables: Add remaining getters and setters

[Phil Sutter]

Provide API functions for remaining context settings changed by main.c,
then hide struct nft_ctx definition from applications. This allows us to
later change data structures internally without risk of breaking
applications.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h          | 27 +++++++++++++++++++++
 include/nftables/nftables.h | 41 ++++++++++---------------------
 src/libnftables.c           | 59 +++++++++++++++++++++++++++++++++++++++++++++
 src/main.c                  | 18 +++++++++-----
 4 files changed, 111 insertions(+), 34 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index a633e1a2cc2e2..ad72383303bdb 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -6,6 +6,33 @@
 #include <utils.h>
 #include <nftables/nftables.h>
 
+struct nft_cache {
+	bool			initialized;
+	struct list_head	list;
+	uint32_t		seqnum;
+};
+
+struct output_ctx {
+	unsigned int numeric;
+	unsigned int stateless;
+	unsigned int ip2name;
+	unsigned int handle;
+	unsigned int echo;
+	FILE *output_fp;
+};
+
+struct nft_ctx {
+	struct mnl_socket	*nf_sock;
+	char			**include_paths;
+	unsigned int		num_include_paths;
+	unsigned int		parser_max_errors;
+	unsigned int		debug_mask;
+	struct output_ctx	output;
+	bool			check;
+	struct nft_cache	cache;
+	uint32_t		flags;
+};
+
 struct input_descriptor;
 struct location {
 	const struct input_descriptor		*indesc;
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index a752f20d74132..2bc3061457257 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -9,34 +9,7 @@
 #ifndef LIB_NFTABLES_H
 #define LIB_NFTABLES_H
 
-struct mnl_socket;
-
-struct nft_cache {
-	bool			initialized;
-	struct list_head	list;
-	uint32_t		seqnum;
-};
-
-struct output_ctx {
-	unsigned int numeric;
-	unsigned int stateless;
-	unsigned int ip2name;
-	unsigned int handle;
-	unsigned int echo;
-	FILE *output_fp;
-};
-
-struct nft_ctx {
-	struct mnl_socket	*nf_sock;
-	char			**include_paths;
-	unsigned int		num_include_paths;
-	unsigned int		parser_max_errors;
-	unsigned int		debug_mask;
-	struct output_ctx	output;
-	bool			check;
-	struct nft_cache	cache;
-	uint32_t		flags;
-};
+struct nft_ctx;
 
 enum debug_level {
 	DEBUG_SCANNER		= 0x1,
@@ -78,6 +51,18 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
 void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
 int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path);
 void nft_ctx_clear_include_paths(struct nft_ctx *ctx);
+enum numeric_level nft_ctx_output_get_numeric(struct nft_ctx *ctx);
+void nft_ctx_output_set_numeric(struct nft_ctx *ctx, enum numeric_level level);
+bool nft_ctx_output_get_stateless(struct nft_ctx *ctx);
+void nft_ctx_output_set_stateless(struct nft_ctx *ctx, bool val);
+bool nft_ctx_output_get_ip2name(struct nft_ctx *ctx);
+void nft_ctx_output_set_ip2name(struct nft_ctx *ctx, bool val);
+unsigned int nft_ctx_output_get_debug(struct nft_ctx *ctx);
+void nft_ctx_output_set_debug(struct nft_ctx *ctx, unsigned int mask);
+bool nft_ctx_output_get_handle(struct nft_ctx *ctx);
+void nft_ctx_output_set_handle(struct nft_ctx *ctx, bool val);
+bool nft_ctx_output_get_echo(struct nft_ctx *ctx);
+void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool val);
 
 void nft_ctx_flush_cache(struct nft_ctx *ctx);
 
diff --git a/src/libnftables.c b/src/libnftables.c
index 2f4275c9a0a94..925c96d1272a3 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -206,6 +206,65 @@ void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry)
 	ctx->check = dry;
 }
 
+enum numeric_level nft_ctx_output_get_numeric(struct nft_ctx *ctx)
+{
+	return ctx->output.numeric;
+}
+
+void nft_ctx_output_set_numeric(struct nft_ctx *ctx, enum numeric_level level)
+{
+	ctx->output.numeric = level;
+}
+
+bool nft_ctx_output_get_stateless(struct nft_ctx *ctx)
+{
+	return ctx->output.stateless;
+}
+
+void nft_ctx_output_set_stateless(struct nft_ctx *ctx, bool val)
+{
+	ctx->output.stateless = val;
+}
+
+bool nft_ctx_output_get_ip2name(struct nft_ctx *ctx)
+{
+	return ctx->output.ip2name;
+}
+
+void nft_ctx_output_set_ip2name(struct nft_ctx *ctx, bool val)
+{
+	ctx->output.ip2name = val;
+}
+
+unsigned int nft_ctx_output_get_debug(struct nft_ctx *ctx)
+{
+	return ctx->debug_mask;
+}
+void nft_ctx_output_set_debug(struct nft_ctx *ctx, unsigned int mask)
+{
+	ctx->debug_mask = mask;
+}
+
+bool nft_ctx_output_get_handle(struct nft_ctx *ctx)
+{
+	return ctx->output.handle;
+}
+
+void nft_ctx_output_set_handle(struct nft_ctx *ctx, bool val)
+{
+	ctx->output.handle = val;
+}
+
+bool nft_ctx_output_get_echo(struct nft_ctx *ctx)
+{
+	return ctx->output.echo;
+}
+
+void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool val)
+{
+	ctx->output.echo = val;
+}
+
 static const struct input_descriptor indesc_cmdline = {
 	.type	= INDESC_BUFFER,
 	.name	= "<cmdline>",
diff --git a/src/main.c b/src/main.c
index de5c115757f44..c65966bcf5995 100644
--- a/src/main.c
+++ b/src/main.c
@@ -169,6 +169,8 @@ int main(int argc, char * const *argv)
 	unsigned int len;
 	bool interactive = false;
 	int i, val, rc;
+	enum numeric_level numeric;
+	unsigned int debug_mask;
 
 	nft = nft_ctx_new(NFT_CTX_DEFAULT);
 	nft_ctx_set_output(nft, stdout);
@@ -204,20 +206,23 @@ int main(int argc, char * const *argv)
 			}
 			break;
 		case OPT_NUMERIC:
-			if (++nft->output.numeric > NUMERIC_ALL) {
+			numeric = nft_ctx_output_get_numeric(nft);
+			if (numeric == NUMERIC_ALL) {
 				fprintf(stderr, "Too many numeric options "
 						"used, max. %u\n",
 					NUMERIC_ALL);
 				exit(NFT_EXIT_FAILURE);
 			}
+			nft_ctx_output_set_numeric(nft, numeric + 1);
 			break;
 		case OPT_STATELESS:
-			nft->output.stateless++;
+			nft_ctx_output_set_stateless(nft, true);
 			break;
 		case OPT_IP2NAME:
-			nft->output.ip2name++;
+			nft_ctx_output_set_ip2name(nft, true);
 			break;
 		case OPT_DEBUG:
+			debug_mask = nft_ctx_output_get_debug(nft);
 			for (;;) {
 				unsigned int i;
 				char *end;
@@ -229,7 +234,7 @@ int main(int argc, char * const *argv)
 				for (i = 0; i < array_size(debug_param); i++) {
 					if (strcmp(debug_param[i].name, optarg))
 						continue;
-					nft->debug_mask |= debug_param[i].level;
+					debug_mask |= debug_param[i].level;
 					break;
 				}
 
@@ -243,12 +248,13 @@ int main(int argc, char * const *argv)
 					break;
 				optarg = end + 1;
 			}
+			nft_ctx_output_set_debug(nft, debug_mask);
 			break;
 		case OPT_HANDLE_OUTPUT:
-			nft->output.handle++;
+			nft_ctx_output_set_handle(nft, true);
 			break;
 		case OPT_ECHO:
-			nft->output.echo++;
+			nft_ctx_output_set_echo(nft, true);
 			break;
 		case OPT_INVALID:
 			exit(NFT_EXIT_FAILURE);
-- 
2.13.1

Re: [nft PATCH 7/7] libnftables: Add remaining getters and setters

[Pablo Neira Ayuso]

On Thu, Oct 19, 2017 at 10:18:47AM +0200, Phil Sutter wrote:
> Provide API functions for remaining context settings changed by main.c,
> then hide struct nft_ctx definition from applications. This allows us to
> later change data structures internally without risk of breaking
> applications.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/nftables.h          | 27 +++++++++++++++++++++
>  include/nftables/nftables.h | 41 ++++++++++---------------------
>  src/libnftables.c           | 59 +++++++++++++++++++++++++++++++++++++++++++++
>  src/main.c                  | 18 +++++++++-----
>  4 files changed, 111 insertions(+), 34 deletions(-)
> 
> diff --git a/include/nftables.h b/include/nftables.h
> index a633e1a2cc2e2..ad72383303bdb 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -6,6 +6,33 @@
>  #include <utils.h>
>  #include <nftables/nftables.h>
>  
> +struct nft_cache {
> +	bool			initialized;
> +	struct list_head	list;
> +	uint32_t		seqnum;
> +};
> +
> +struct output_ctx {
> +	unsigned int numeric;
> +	unsigned int stateless;
> +	unsigned int ip2name;
> +	unsigned int handle;
> +	unsigned int echo;
> +	FILE *output_fp;
> +};
> +
> +struct nft_ctx {
> +	struct mnl_socket	*nf_sock;
> +	char			**include_paths;
> +	unsigned int		num_include_paths;
> +	unsigned int		parser_max_errors;
> +	unsigned int		debug_mask;
> +	struct output_ctx	output;
> +	bool			check;
> +	struct nft_cache	cache;
> +	uint32_t		flags;
> +};

Oh, I see. Now these structure definitions are coming back to
include/nftables.h. I'm telling this because of what I mentioned in
2/7.

I would prefer we avoid these goes back and forth with this code.

> +
>  struct input_descriptor;
>  struct location {
>  	const struct input_descriptor		*indesc;
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> index a752f20d74132..2bc3061457257 100644
> --- a/include/nftables/nftables.h
> +++ b/include/nftables/nftables.h
> @@ -9,34 +9,7 @@
>  #ifndef LIB_NFTABLES_H
>  #define LIB_NFTABLES_H
>  
> -struct mnl_socket;
> -
> -struct nft_cache {
> -	bool			initialized;
> -	struct list_head	list;
> -	uint32_t		seqnum;
> -};
> -
> -struct output_ctx {
> -	unsigned int numeric;
> -	unsigned int stateless;
> -	unsigned int ip2name;
> -	unsigned int handle;
> -	unsigned int echo;
> -	FILE *output_fp;
> -};
> -
> -struct nft_ctx {
> -	struct mnl_socket	*nf_sock;
> -	char			**include_paths;
> -	unsigned int		num_include_paths;
> -	unsigned int		parser_max_errors;
> -	unsigned int		debug_mask;
> -	struct output_ctx	output;
> -	bool			check;
> -	struct nft_cache	cache;
> -	uint32_t		flags;
> -};
> +struct nft_ctx;
>  
>  enum debug_level {
>  	DEBUG_SCANNER		= 0x1,
> @@ -78,6 +51,18 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
>  void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
>  int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path);
>  void nft_ctx_clear_include_paths(struct nft_ctx *ctx);
> +enum numeric_level nft_ctx_output_get_numeric(struct nft_ctx *ctx);
> +void nft_ctx_output_set_numeric(struct nft_ctx *ctx, enum numeric_level level);
> +bool nft_ctx_output_get_stateless(struct nft_ctx *ctx);
> +void nft_ctx_output_set_stateless(struct nft_ctx *ctx, bool val);
> +bool nft_ctx_output_get_ip2name(struct nft_ctx *ctx);
> +void nft_ctx_output_set_ip2name(struct nft_ctx *ctx, bool val);
> +unsigned int nft_ctx_output_get_debug(struct nft_ctx *ctx);
> +void nft_ctx_output_set_debug(struct nft_ctx *ctx, unsigned int mask);
> +bool nft_ctx_output_get_handle(struct nft_ctx *ctx);
> +void nft_ctx_output_set_handle(struct nft_ctx *ctx, bool val);
> +bool nft_ctx_output_get_echo(struct nft_ctx *ctx);
> +void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool val);
>  
>  void nft_ctx_flush_cache(struct nft_ctx *ctx);
>  
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 2f4275c9a0a94..925c96d1272a3 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -206,6 +206,65 @@ void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry)
>  	ctx->check = dry;
>  }
>  
> +enum numeric_level nft_ctx_output_get_numeric(struct nft_ctx *ctx)
> +{
> +	return ctx->output.numeric;
> +}
> +
> +void nft_ctx_output_set_numeric(struct nft_ctx *ctx, enum numeric_level level)
> +{
> +	ctx->output.numeric = level;
> +}
> +
> +bool nft_ctx_output_get_stateless(struct nft_ctx *ctx)
> +{
> +	return ctx->output.stateless;
> +}
> +
> +void nft_ctx_output_set_stateless(struct nft_ctx *ctx, bool val)
> +{
> +	ctx->output.stateless = val;
> +}
> +
> +bool nft_ctx_output_get_ip2name(struct nft_ctx *ctx)
> +{
> +	return ctx->output.ip2name;
> +}
> +
> +void nft_ctx_output_set_ip2name(struct nft_ctx *ctx, bool val)
> +{
> +	ctx->output.ip2name = val;
> +}
> +
> +unsigned int nft_ctx_output_get_debug(struct nft_ctx *ctx)
> +{
> +	return ctx->debug_mask;
> +}
> +void nft_ctx_output_set_debug(struct nft_ctx *ctx, unsigned int mask)
> +{
> +	ctx->debug_mask = mask;
> +}
> +
> +bool nft_ctx_output_get_handle(struct nft_ctx *ctx)
> +{
> +	return ctx->output.handle;
> +}
> +
> +void nft_ctx_output_set_handle(struct nft_ctx *ctx, bool val)
> +{
> +	ctx->output.handle = val;
> +}
> +
> +bool nft_ctx_output_get_echo(struct nft_ctx *ctx)
> +{
> +	return ctx->output.echo;
> +}
> +
> +void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool val)
> +{
> +	ctx->output.echo = val;
> +}

BTW, why not just add dry_run setter here too?

I see no need for the standalone patch that comes in this series.

Thanks!

Re: [nft PATCH 7/7] libnftables: Add remaining getters and setters

[Phil Sutter]

Hi Pablo,

On Fri, Oct 20, 2017 at 02:18:53PM +0200, Pablo Neira Ayuso wrote:
[...]
> Oh, I see. Now these structure definitions are coming back to
> include/nftables.h. I'm telling this because of what I mentioned in
> 2/7.

I have to admit, it was quite entertaining watching you following my
series through your comments. :)

> I would prefer we avoid these goes back and forth with this code.

Yes, point taken - I will recombine the changes into more sensible
chunks.

Thanks, Phil