[nft PATCH v2 0/4] libnftables preparations

[nft PATCH v2 0/4] 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.

Changes since v1:
- Dropped first patch from series as it was applied already.
- Rearranged changes in remaining patches to become more clear and avoid
  moving header file content back and forth between nftables.h and
  nftables/nftables.h.
- Improved patch descriptions.

Phil Sutter (4):
  libnftables: Move library stuff out of main.c
  libnftables: Introduce nft_ctx_flush_cache()
  cli: Use nft_run_cmd_from_buffer()
  libnftables: Introduce getters and setters for everything

 include/Makefile.am          |   3 +-
 include/cli.h                |   6 +-
 include/nftables.h           |  38 +----
 include/nftables/Makefile.am |   1 +
 include/nftables/nftables.h  |  78 ++++++++++
 src/Makefile.am              |   3 +-
 src/cli.c                    |  24 +--
 src/libnftables.c            | 364 +++++++++++++++++++++++++++++++++++++++++++
 src/main.c                   | 285 +++------------------------------
 src/scanner.l                |   4 +-
 10 files changed, 474 insertions(+), 332 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 v2 1/4] libnftables: Move library stuff out of main.c

[Phil Sutter]

This creates src/libnftables.c and include/nftables/nftables.h which
will become the central elements of libnftables.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/Makefile.am          |   3 +-
 include/nftables.h           |  27 +----
 include/nftables/Makefile.am |   1 +
 include/nftables/nftables.h  |  58 ++++++++++
 src/Makefile.am              |   3 +-
 src/libnftables.c            | 262 +++++++++++++++++++++++++++++++++++++++++++
 src/main.c                   | 252 +----------------------------------------
 7 files changed, 327 insertions(+), 279 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..eb39dbd1487dd 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -4,23 +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,
-};
+#include <nftables/nftables.h>
 
 #define INCLUDE_PATHS_MAX	16
 
@@ -53,15 +37,6 @@ struct nft_ctx {
 	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,
-};
-
 struct input_descriptor;
 struct location {
 	const struct input_descriptor		*indesc;
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..44d3e95d399e6
--- /dev/null
+++ b/include/nftables/nftables.h
@@ -0,0 +1,58 @@
+/*
+ * 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
+
+#define _GNU_SOURCE
+#include <stdint.h>
+#include <stdio.h>
+#include <stdbool.h>
+
+struct nft_ctx;
+
+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_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..9bc51dd894d09
--- /dev/null
+++ b/src/libnftables.c
@@ -0,0 +1,262 @@
+/*
+ * 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 <nftables/nftables.h>
+#include <erec.h>
+#include <mnl.h>
+#include <parser.h>
+#include <utils.h>
+#include <iface.h>
+
+#include <errno.h>
+#include <string.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..b9938c9cd86e6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -18,14 +18,9 @@
 #include <fcntl.h>
 #include <sys/types.h>
 
-#include <nftables.h>
+#include <nftables/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>
 
 static struct nft_ctx *nft;
@@ -169,251 +164,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

[nft PATCH v2 2/4] libnftables: Introduce nft_ctx_flush_cache()

[Phil Sutter]

This allows an application to explicitly flush caches associated with a
given nft context, as seen in cli_complete().

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.

Note that there is no need for explicit cache update routine since cache
is populated during command execution depending on whether it is needed
or not.

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

diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 44d3e95d399e6..1207f10cd2457 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -51,6 +51,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_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..3174cfed05a7d 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -140,8 +140,7 @@ static void cli_complete(char *line)
 	nft_run(cli_nft, cli_nf_sock, scanner, state, &msgs);
 	erec_print_list(&cli_nft->output, &msgs, cli_nft->debug_mask);
 	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)
diff --git a/src/libnftables.c b/src/libnftables.c
index 9bc51dd894d09..d34e52759e947 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -147,13 +147,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

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

[Phil Sutter]

Make CLI code adhere to intended libnftables API by not open coding what
nft_run_cmd_from_buffer() does. This way, nft_run() has no users outside
of src/libnftables.c anymore and therefore can become static.

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.h |  7 -------
 src/cli.c          | 21 ++-------------------
 src/libnftables.c  |  6 +++---
 src/main.c         |  5 ++---
 5 files changed, 9 insertions(+), 36 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.h b/include/nftables.h
index eb39dbd1487dd..98d619a3ab8d2 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -103,13 +103,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/src/cli.c b/src/cli.c
index 3174cfed05a7d..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,11 +126,7 @@ 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);
 	nft_ctx_flush_cache(cli_nft);
 }
@@ -148,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;
@@ -170,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 d34e52759e947..51a87dc34bc60 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -75,9 +75,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 b9938c9cd86e6..a28564172e9a0 100644
--- a/src/main.c
+++ b/src/main.c
@@ -20,7 +20,7 @@
 
 #include <nftables/nftables.h>
 #include <utils.h>
-#include <parser.h>
+#include <nftables.h>
 #include <cli.h>
 
 static struct nft_ctx *nft;
@@ -169,7 +169,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);
@@ -273,7 +272,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

[nft PATCH v2 4/4] libnftables: Introduce getters and setters for everything

[Phil Sutter]

This introduces getter/setter pairs for all parts in struct nft_ctx (and
contained structs) which should be configurable.

Most of them are simple ones, just allowing to get/set a given field:

* nft_ctx_{get,set}_dry_run() -> ctx->check
* nft_ctx_output_{get,set}_numeric()	-> ctx->output.numeric
* nft_ctx_output_{get,set}_stateless()	-> ctx->output.stateless
* nft_ctx_output_{get,set}_ip2name()	-> ctx->output.ip2name
* nft_ctx_output_{get,set}_debug()	-> ctx->debug_mask
* nft_ctx_output_{get,set}_handle()	-> ctx->output.handle
* nft_ctx_output_{get,set}_echo()	-> ctx->output.echo

A more complicated case is include paths handling: In order to keep the
API simple, remove INCLUDE_PATHS_MAX restraint and dynamically allocate
nft_ctx field include_paths instead. So there is:

* nft_ctx_add_include_path()	-> add an include path to the list
* nft_ctx_clear_include_paths()	-> flush the list of include paths

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h          |   4 +-
 include/nftables/nftables.h |  19 +++++++++
 src/libnftables.c           | 101 +++++++++++++++++++++++++++++++++++++++++++-
 src/main.c                  |  30 +++++++------
 src/scanner.l               |   4 +-
 5 files changed, 137 insertions(+), 21 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index 98d619a3ab8d2..97a0436693cfa 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -6,8 +6,6 @@
 #include <utils.h>
 #include <nftables/nftables.h>
 
-#define INCLUDE_PATHS_MAX	16
-
 struct output_ctx {
 	unsigned int numeric;
 	unsigned int stateless;
@@ -27,7 +25,7 @@ struct mnl_socket;
 
 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;
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 1207f10cd2457..2dff281183b3c 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -50,7 +50,26 @@ enum nftables_exit_codes {
 
 struct nft_ctx *nft_ctx_new(uint32_t flags);
 void nft_ctx_free(struct nft_ctx *ctx);
+
+bool nft_ctx_get_dry_run(struct nft_ctx *ctx);
+void nft_ctx_set_dry_run(struct nft_ctx *ctx, bool dry);
+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);
+
 FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
+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);
 
 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 51a87dc34bc60..5e70c197846cf 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -14,6 +14,7 @@
 #include <iface.h>
 
 #include <errno.h>
+#include <stdlib.h>
 #include <string.h>
 
 static int nft_netlink(struct nft_ctx *nft,
@@ -123,6 +124,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();
@@ -135,8 +163,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;
@@ -159,6 +186,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();
 }
@@ -172,6 +200,75 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp)
 	return old;
 }
 
+bool nft_ctx_get_dry_run(struct nft_ctx *ctx)
+{
+	return ctx->check;
+}
+
+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 a28564172e9a0..59c39d4570b02 100644
--- a/src/main.c
+++ b/src/main.c
@@ -20,7 +20,6 @@
 
 #include <nftables/nftables.h>
 #include <utils.h>
-#include <nftables.h>
 #include <cli.h>
 
 static struct nft_ctx *nft;
@@ -170,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);
@@ -188,7 +189,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;
@@ -197,29 +198,31 @@ 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) {
+			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;
@@ -231,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;
 				}
 
@@ -245,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);
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 v2 0/4] libnftables preparations

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On Mon, Oct 23, 2017 at 05:33:15PM +0200, Phil Sutter wrote:
> 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.

Series applied.

I have to merged this patch below to 1/4.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 367 bytes --]

diff --git a/configure.ac b/configure.ac
index b01c5bdef0eb..099a4a5e81ec 100644
--- a/configure.ac
+++ b/configure.ac
@@ -140,6 +140,7 @@ AC_CONFIG_FILES([					\
 		Makefile				\
 		src/Makefile				\
 		include/Makefile			\
+		include/nftables/Makefile		\
 		include/linux/Makefile			\
 		include/linux/netfilter/Makefile	\
 		include/linux/netfilter_arp/Makefile	\

Re: [nft PATCH v2 1/4] libnftables: Move library stuff out of main.c

[Pablo Neira Ayuso]

Hi Phil,

Just follow up comments, several things that I think we can polish,
see below.

On Mon, Oct 23, 2017 at 05:33:16PM +0200, Phil Sutter wrote:
[...]
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> new file mode 100644
> index 0000000000000..44d3e95d399e6
> --- /dev/null
> +++ b/include/nftables/nftables.h
> @@ -0,0 +1,58 @@
> +/*
> + * 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
> +
> +#define _GNU_SOURCE
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +
> +struct nft_ctx;
> +
> +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,
> +};

Just pushed out a patch to prepend NFT_ prefix.

> +/**
> + * 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,
> +};

I think library is currently aborting in case of no-netlink and
no-memory, so these two error codes are not useful.

We would need to change codebase to propagate errors up to the
callers.

Regarding error code, I would go for -1 in case of error instead and 0
in case of success. If failure happens, then set errno with reason, so
we can get rid of these exit codes in a follow up patch?

Re: [nft PATCH v2 2/4] libnftables: Introduce nft_ctx_flush_cache()

[Pablo Neira Ayuso]

On Mon, Oct 23, 2017 at 05:33:17PM +0200, Phil Sutter wrote:
> This allows an application to explicitly flush caches associated with a
> given nft context, as seen in cli_complete().
> 
> 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.
> 
> Note that there is no need for explicit cache update routine since cache
> is populated during command execution depending on whether it is needed
> or not.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/nftables/nftables.h | 1 +
>  src/cli.c                   | 3 +--
>  src/libnftables.c           | 9 +++++++--
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> index 44d3e95d399e6..1207f10cd2457 100644
> --- a/include/nftables/nftables.h
> +++ b/include/nftables/nftables.h
> @@ -51,6 +51,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);

I would prefer we rename this to nft_ctx_flush_cache().

Now, let's make an exercise of abstraction: You're not Phil Sutter,
you're developer Vlamidir.

Vladimir wants to make a third party application based on libnftables
that is going to rock the world.

He knows nothing about internals, but he looks at the API and say:
"Hey, I can flush the cache, what is this cache concept?".

I'm telling this story because we're leaking an internal detail
implementation. So I would just rename this to nft_ctx_reset(). This
magic call just cleans up the context for you.

Vladimir will be just need to know that he needs to reset context if
he want to make incremental updates based on current.

I agree with you that we cannot allocate 'cache' from nft_ctx_alloc(),
it's good you reminded me this :-).

But I would say, we can just hide these details behind the curtain.

Let me know, thanks

P.S: I still has pushed out your patchset, it's just that I'm
providing feedback so we speed up libnftables release :-).

Re: [nft PATCH v2 0/4] libnftables preparations

[Phil Sutter]

On Tue, Oct 24, 2017 at 05:20:33PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2017 at 05:33:15PM +0200, Phil Sutter wrote:
> > 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.
> 
> Series applied.

Cool, thanks!

> I have to merged this patch below to 1/4.

Oops. :)

Cheers, Phil

Re: [nft PATCH v2 1/4] libnftables: Move library stuff out of main.c

[Phil Sutter]

Hi Pablo,

On Tue, Oct 24, 2017 at 05:48:00PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2017 at 05:33:16PM +0200, Phil Sutter wrote:
[...]
> > +/**
> > + * 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,
> > +};
> 
> I think library is currently aborting in case of no-netlink and
> no-memory, so these two error codes are not useful.
> 
> We would need to change codebase to propagate errors up to the
> callers.

Ah yes, indeed. I guess for a library, calling exit() is a no-go. :)

> Regarding error code, I would go for -1 in case of error instead and 0
> in case of success. If failure happens, then set errno with reason, so
> we can get rid of these exit codes in a follow up patch?

Sounds good, I'll send a follow-up.

Thanks, Phil

Re: [nft PATCH v2 2/4] libnftables: Introduce nft_ctx_flush_cache()

[Phil Sutter]

Hi,

On Tue, Oct 24, 2017 at 05:52:59PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2017 at 05:33:17PM +0200, Phil Sutter wrote:
[...]
> > diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> > index 44d3e95d399e6..1207f10cd2457 100644
> > --- a/include/nftables/nftables.h
> > +++ b/include/nftables/nftables.h
> > @@ -51,6 +51,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);
> 
> I would prefer we rename this to nft_ctx_flush_cache().
> 
> Now, let's make an exercise of abstraction: You're not Phil Sutter,
> you're developer Vlamidir.
> 
> Vladimir wants to make a third party application based on libnftables
> that is going to rock the world.
> 
> He knows nothing about internals, but he looks at the API and say:
> "Hey, I can flush the cache, what is this cache concept?".
> 
> I'm telling this story because we're leaking an internal detail
> implementation. So I would just rename this to nft_ctx_reset(). This
> magic call just cleans up the context for you.

Hmm. Without further information, I would guess for nft_ctx_reset() to
reset all the context data which was previously modified using those
setters I defined. A possible cache to reset didn't come to mind since I
wasn't aware that there is a cache at all!

> Vladimir will be just need to know that he needs to reset context if
> he want to make incremental updates based on current.

I wonder whether we need to reset the cache at all: We could make
cache_update() ignore cache->initialized and instead check whether
nft_genid did change after calling netlink_genid_get() - if not, cache
is up to date, otherwise call cache_init(). When we discussed possible
performance implications of cache updates, I suggested just that as a
first counter-measure.

Could this work? Or am I missing something?

Cheers, Phil

Re: [nft PATCH v2 2/4] libnftables: Introduce nft_ctx_flush_cache()

[Pablo Neira Ayuso]

Hi Phil,

On Tue, Oct 24, 2017 at 07:40:21PM +0200, Phil Sutter wrote:
[...]
> I wonder whether we need to reset the cache at all:

Good point.

> We could make cache_update() ignore cache->initialized and instead
> check whether nft_genid did change after calling netlink_genid_get()
> - if not, cache is up to date, otherwise call cache_init(). When we
> discussed possible performance implications of cache updates, I
> suggested just that as a first counter-measure.
> 
> Could this work? Or am I missing something?

For the simple API, I think it makes sense to remove this
nft_ctx_flush_cache() and just perform inconditional cache refresh on
every nft_cmd_*() call by now.

We can revisit later on to do incremental cache updates based on
generation ID as you said. If we keep track of incremental updates,
then we will need event handling. But for now I will go simple.

Thanks!

[nft PATCH] libnftables: Get rid of explicit cache flushes

[Phil Sutter]

In the past, CLI as a potentially long running process had to make sure
it kept it's cache up to date with kernel's rule set. A simple test case
is this:

| shell a		|	shell b
|			|	# nft -i
| # nft add table ip t	|
|			|	nft> list ruleset
|			|	table ip t {
|			|	}
| # nft flush ruleset	|
|			|	nft> list ruleset
|			|	nft>

In order to make sure interactive CLI wouldn't incorrectly list the
table again in the second 'list' command, it immediately flushed it's
cache after every command execution.

This patch eliminates the need for that by making cache updates depend
on kernel's generation ID: A cache update stores the current rule set's
ID in struct nft_cache, consecutive calls to cache_update() compare that
stored value to the current generation ID received from kernel - if the
stored value is zero (i.e. no previous cache update did happen) or if it
doesn't match the kernel's value (i.e. cache is outdated) the cache is
flushed and fully initialized again.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/mnl.h               |  2 +-
 include/netlink.h           |  2 +-
 include/nftables.h          |  2 +-
 include/nftables/nftables.h |  2 --
 src/cli.c                   |  1 -
 src/libnftables.c           |  9 ++-------
 src/mnl.c                   |  4 +++-
 src/netlink.c               |  4 ++--
 src/rule.c                  | 12 +++++++-----
 9 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 3df71467c1080..84c362a23bd79 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -15,7 +15,7 @@ struct mnl_socket *netlink_open_sock(void);
 void netlink_close_sock(struct mnl_socket *nf_sock);
 
 uint32_t mnl_seqnum_alloc(uint32_t *seqnum);
-void mnl_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum);
+uint16_t mnl_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum);
 
 struct mnl_err {
 	struct list_head	head;
diff --git a/include/netlink.h b/include/netlink.h
index 2ca6f345b6ac9..b30c05f833554 100644
--- a/include/netlink.h
+++ b/include/netlink.h
@@ -191,7 +191,7 @@ extern void netlink_dump_obj(struct nftnl_obj *nlo, struct netlink_ctx *ctx);
 
 extern int netlink_batch_send(struct netlink_ctx *ctx, struct list_head *err_list);
 
-extern void netlink_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum);
+extern uint16_t netlink_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum);
 extern void netlink_restart(struct mnl_socket *nf_sock);
 #define netlink_abi_error()	\
 	__netlink_abi_error(__FILE__, __LINE__, strerror(errno));
diff --git a/include/nftables.h b/include/nftables.h
index 97a0436693cfa..d69079fe03e3a 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -16,7 +16,7 @@ struct output_ctx {
 };
 
 struct nft_cache {
-	bool			initialized;
+	uint16_t		genid;
 	struct list_head	list;
 	uint32_t		seqnum;
 };
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 449f9e4ee879c..4211be76e6e40 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -70,8 +70,6 @@ FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
 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);
-
 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 37351f2f8b04f..ec4d2a6f2046d 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -128,7 +128,6 @@ static void cli_complete(char *line)
 
 	nft_run_cmd_from_buffer(cli_nft, line, len + 2);
 	xfree(line);
-	nft_ctx_flush_cache(cli_nft);
 }
 
 static char **cli_completion(const char *text, int start, int end)
diff --git a/src/libnftables.c b/src/libnftables.c
index 5ef5532c7f075..0d04ec21d57d3 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -174,18 +174,13 @@ 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);
 
-	nft_ctx_flush_cache(ctx);
+	iface_cache_release();
+	cache_release(&ctx->cache);
 	nft_ctx_clear_include_paths(ctx);
 	xfree(ctx);
 	nft_exit();
diff --git a/src/mnl.c b/src/mnl.c
index 8db2a1847ec51..3be6ebaf1f682 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -94,7 +94,7 @@ static int genid_cb(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-void mnl_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum)
+uint16_t mnl_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct mnl_ctx ctx = {
@@ -106,6 +106,8 @@ void mnl_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum)
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_GETGEN, AF_UNSPEC, 0, seqnum);
 	/* Skip error checking, old kernels sets res_id field to zero. */
 	nft_mnl_talk(&ctx, nlh, nlh->nlmsg_len, genid_cb, NULL);
+
+	return nft_genid;
 }
 
 static int check_genid(const struct nlmsghdr *nlh)
diff --git a/src/netlink.c b/src/netlink.c
index abc22504f877a..845eeeffd7387 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -75,9 +75,9 @@ void netlink_restart(struct mnl_socket *nf_sock)
 	nf_sock = netlink_open_sock();
 }
 
-void netlink_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum)
+uint16_t netlink_genid_get(struct mnl_socket *nf_sock, uint32_t seqnum)
 {
-	mnl_genid_get(nf_sock, seqnum);
+	return mnl_genid_get(nf_sock, seqnum);
 }
 
 void __noreturn __netlink_abi_error(const char *file, int line,
diff --git a/src/rule.c b/src/rule.c
index 948478c96bda2..6a322167b8265 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -153,12 +153,14 @@ int cache_update(struct mnl_socket *nf_sock, struct nft_cache *cache,
 		 enum cmd_ops cmd, struct list_head *msgs, bool debug,
 		 struct output_ctx *octx)
 {
+	uint16_t genid;
 	int ret;
 
-	if (cache->initialized)
-		return 0;
 replay:
-	netlink_genid_get(nf_sock, cache->seqnum++);
+	genid = netlink_genid_get(nf_sock, cache->seqnum++);
+	if (genid && genid == cache->genid)
+		return 0;
+	cache_release(cache);
 	ret = cache_init(nf_sock, cache, cmd, msgs, debug, octx);
 	if (ret < 0) {
 		cache_release(cache);
@@ -168,7 +170,7 @@ replay:
 		}
 		return -1;
 	}
-	cache->initialized = true;
+	cache->genid = genid;
 	return 0;
 }
 
@@ -185,7 +187,7 @@ void cache_flush(struct list_head *table_list)
 void cache_release(struct nft_cache *cache)
 {
 	cache_flush(&cache->list);
-	cache->initialized = false;
+	cache->genid = 0;
 }
 
 /* internal ID to uniquely identify a set in the batch */
-- 
2.13.1

Re: [nft PATCH] libnftables: Get rid of explicit cache flushes

[Pablo Neira Ayuso]

On Wed, Oct 25, 2017 at 01:40:29PM +0200, Phil Sutter wrote:
> In the past, CLI as a potentially long running process had to make sure
> it kept it's cache up to date with kernel's rule set. A simple test case
> is this:
> 
> | shell a		|	shell b
> |			|	# nft -i
> | # nft add table ip t	|
> |			|	nft> list ruleset
> |			|	table ip t {
> |			|	}
> | # nft flush ruleset	|
> |			|	nft> list ruleset
> |			|	nft>
> 
> In order to make sure interactive CLI wouldn't incorrectly list the
> table again in the second 'list' command, it immediately flushed it's
> cache after every command execution.
> 
> This patch eliminates the need for that by making cache updates depend
> on kernel's generation ID: A cache update stores the current rule set's
> ID in struct nft_cache, consecutive calls to cache_update() compare that
> stored value to the current generation ID received from kernel - if the
> stored value is zero (i.e. no previous cache update did happen) or if it
> doesn't match the kernel's value (i.e. cache is outdated) the cache is
> flushed and fully initialized again.

Applied, thanks Phil.

[nft PATCH] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

Apart from SUCCESS/FAILURE, these codes were not used by library
functions simply because NOMEM and NONL conditions lead to calling
exit() instead of propagating the error condition back up the call
stack.

Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
Usually errno will then contain more details about what happened and/or
there are messages in erec.

Calls to exit() in main() are adjusted to stay compatible.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h          |  7 +++++++
 include/nftables/nftables.h | 10 ----------
 src/libnftables.c           | 10 +++++-----
 src/main.c                  | 16 ++++++++--------
 4 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index d69079fe03e3a..3bfa33e5cb336 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -35,6 +35,13 @@ struct nft_ctx {
 	uint32_t		flags;
 };
 
+enum nftables_exit_codes {
+	NFT_EXIT_SUCCESS	= 0,
+	NFT_EXIT_FAILURE	= 1,
+	NFT_EXIT_NOMEM		= 2,
+	NFT_EXIT_NONL		= 3,
+};
+
 struct input_descriptor;
 struct location {
 	const struct input_descriptor		*indesc;
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 4211be76e6e40..8e59f2b2a59ab 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -38,16 +38,6 @@ enum nft_numeric_level {
  */
 #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);
 
diff --git a/src/libnftables.c b/src/libnftables.c
index dc6a5fdf32640..e8fa6742f7d17 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -272,7 +272,7 @@ static const struct input_descriptor indesc_cmdline = {
 
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 {
-	int rc = NFT_EXIT_SUCCESS;
+	int rc = 0;
 	struct parser_state state;
 	LIST_HEAD(msgs);
 	void *scanner;
@@ -284,7 +284,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 	scanner_push_buffer(scanner, &indesc_cmdline, buf);
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
@@ -306,18 +306,18 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
 			  nft->debug_mask, &nft->output);
 	if (rc < 0)
-		return NFT_EXIT_FAILURE;
+		return -1;
 
 	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;
+		rc = -1;
 		goto err;
 	}
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 err:
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
diff --git a/src/main.c b/src/main.c
index 529bedffc2e3b..b4648d7008d04 100644
--- a/src/main.c
+++ b/src/main.c
@@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
 		switch (val) {
 		case OPT_HELP:
 			show_help(argv[0]);
-			exit(NFT_EXIT_SUCCESS);
+			exit(0);
 		case OPT_VERSION:
 			printf("%s v%s (%s)\n",
 			       PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME);
-			exit(NFT_EXIT_SUCCESS);
+			exit(0);
 		case OPT_CHECK:
 			nft_ctx_set_dry_run(nft, true);
 			break;
@@ -202,7 +202,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr,
 					"Failed to add include path '%s'\n",
 					optarg);
-				exit(NFT_EXIT_FAILURE);
+				exit(1);
 			}
 			break;
 		case OPT_NUMERIC:
@@ -211,7 +211,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr, "Too many numeric options "
 						"used, max. %u\n",
 					NFT_NUMERIC_ALL);
-				exit(NFT_EXIT_FAILURE);
+				exit(1);
 			}
 			nft_ctx_output_set_numeric(nft, numeric + 1);
 			break;
@@ -241,7 +241,7 @@ int main(int argc, char * const *argv)
 				if (i == array_size(debug_param)) {
 					fprintf(stderr, "invalid debug parameter `%s'\n",
 						optarg);
-					exit(NFT_EXIT_FAILURE);
+					exit(1);
 				}
 
 				if (end == NULL)
@@ -257,7 +257,7 @@ int main(int argc, char * const *argv)
 			nft_ctx_output_set_echo(nft, true);
 			break;
 		case OPT_INVALID:
-			exit(NFT_EXIT_FAILURE);
+			exit(1);
 		}
 	}
 
@@ -279,12 +279,12 @@ int main(int argc, char * const *argv)
 		if (cli_init(nft) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
-			exit(NFT_EXIT_FAILURE);
+			exit(1);
 		}
 		return 0;
 	} else {
 		fprintf(stderr, "%s: no command specified\n", argv[0]);
-		exit(NFT_EXIT_FAILURE);
+		exit(1);
 	}
 
 	xfree(buf);
-- 
2.13.1

Re: [nft PATCH] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

On Thu, Nov 09, 2017 at 02:25:26PM +0100, Phil Sutter wrote:
> Apart from SUCCESS/FAILURE, these codes were not used by library
> functions simply because NOMEM and NONL conditions lead to calling
> exit() instead of propagating the error condition back up the call
> stack.
> 
> Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
> Usually errno will then contain more details about what happened and/or
> there are messages in erec.
> 
> Calls to exit() in main() are adjusted to stay compatible.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Please disregard this patch, it accidentally breaks expected fail cases
in tests/shell because main() then returns -1 instead of 1.

I'll prepare a v2.

Sorry, Phil

[nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

Apart from SUCCESS/FAILURE, these codes were not used by library
functions simply because NOMEM and NONL conditions lead to calling
exit() instead of propagating the error condition back up the call
stack.

Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
Usually errno will then contain more details about what happened and/or
there are messages in erec.

Calls to exit() in main() are adjusted to stay compatible.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Fixed return code of main() if nft_run_cmd_from_*() fails, this broke
  tests/shell.
---
 include/nftables.h          |  7 +++++++
 include/nftables/nftables.h | 10 ----------
 src/libnftables.c           | 10 +++++-----
 src/main.c                  | 20 ++++++++++----------
 4 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index d69079fe03e3a..3bfa33e5cb336 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -35,6 +35,13 @@ struct nft_ctx {
 	uint32_t		flags;
 };
 
+enum nftables_exit_codes {
+	NFT_EXIT_SUCCESS	= 0,
+	NFT_EXIT_FAILURE	= 1,
+	NFT_EXIT_NOMEM		= 2,
+	NFT_EXIT_NONL		= 3,
+};
+
 struct input_descriptor;
 struct location {
 	const struct input_descriptor		*indesc;
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 4211be76e6e40..8e59f2b2a59ab 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -38,16 +38,6 @@ enum nft_numeric_level {
  */
 #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);
 
diff --git a/src/libnftables.c b/src/libnftables.c
index dc6a5fdf32640..e8fa6742f7d17 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -272,7 +272,7 @@ static const struct input_descriptor indesc_cmdline = {
 
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 {
-	int rc = NFT_EXIT_SUCCESS;
+	int rc = 0;
 	struct parser_state state;
 	LIST_HEAD(msgs);
 	void *scanner;
@@ -284,7 +284,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 	scanner_push_buffer(scanner, &indesc_cmdline, buf);
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
@@ -306,18 +306,18 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
 			  nft->debug_mask, &nft->output);
 	if (rc < 0)
-		return NFT_EXIT_FAILURE;
+		return -1;
 
 	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;
+		rc = -1;
 		goto err;
 	}
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 err:
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
diff --git a/src/main.c b/src/main.c
index 529bedffc2e3b..8d03f8989b1fc 100644
--- a/src/main.c
+++ b/src/main.c
@@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
 		switch (val) {
 		case OPT_HELP:
 			show_help(argv[0]);
-			exit(NFT_EXIT_SUCCESS);
+			exit(0);
 		case OPT_VERSION:
 			printf("%s v%s (%s)\n",
 			       PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME);
-			exit(NFT_EXIT_SUCCESS);
+			exit(0);
 		case OPT_CHECK:
 			nft_ctx_set_dry_run(nft, true);
 			break;
@@ -202,7 +202,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr,
 					"Failed to add include path '%s'\n",
 					optarg);
-				exit(NFT_EXIT_FAILURE);
+				exit(1);
 			}
 			break;
 		case OPT_NUMERIC:
@@ -211,7 +211,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr, "Too many numeric options "
 						"used, max. %u\n",
 					NFT_NUMERIC_ALL);
-				exit(NFT_EXIT_FAILURE);
+				exit(1);
 			}
 			nft_ctx_output_set_numeric(nft, numeric + 1);
 			break;
@@ -241,7 +241,7 @@ int main(int argc, char * const *argv)
 				if (i == array_size(debug_param)) {
 					fprintf(stderr, "invalid debug parameter `%s'\n",
 						optarg);
-					exit(NFT_EXIT_FAILURE);
+					exit(1);
 				}
 
 				if (end == NULL)
@@ -257,7 +257,7 @@ int main(int argc, char * const *argv)
 			nft_ctx_output_set_echo(nft, true);
 			break;
 		case OPT_INVALID:
-			exit(NFT_EXIT_FAILURE);
+			exit(1);
 		}
 	}
 
@@ -272,19 +272,19 @@ int main(int argc, char * const *argv)
 				strcat(buf, " ");
 		}
 		strcat(buf, "\n");
-		rc = nft_run_cmd_from_buffer(nft, buf, len + 2);
+		rc = !!nft_run_cmd_from_buffer(nft, buf, len + 2);
 	} else if (filename != NULL) {
-		rc = nft_run_cmd_from_filename(nft, filename);
+		rc = !!nft_run_cmd_from_filename(nft, filename);
 	} else if (interactive) {
 		if (cli_init(nft) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
-			exit(NFT_EXIT_FAILURE);
+			exit(1);
 		}
 		return 0;
 	} else {
 		fprintf(stderr, "%s: no command specified\n", argv[0]);
-		exit(NFT_EXIT_FAILURE);
+		exit(1);
 	}
 
 	xfree(buf);
-- 
2.13.1

Re: [nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Pablo Neira Ayuso]

On Fri, Nov 10, 2017 at 12:27:15PM +0100, Phil Sutter wrote:
> diff --git a/src/main.c b/src/main.c
> index 529bedffc2e3b..8d03f8989b1fc 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
>  		switch (val) {
>  		case OPT_HELP:
>  			show_help(argv[0]);
> -			exit(NFT_EXIT_SUCCESS);
> +			exit(0);

Better use the standard EXIT_FAILURE and EXIT_SUCCESS here? Instead of
hardcoded 0 and 1 values.

Thanks!

Re: [nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

On Mon, Nov 13, 2017 at 01:31:00PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 10, 2017 at 12:27:15PM +0100, Phil Sutter wrote:
> > diff --git a/src/main.c b/src/main.c
> > index 529bedffc2e3b..8d03f8989b1fc 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
> >  		switch (val) {
> >  		case OPT_HELP:
> >  			show_help(argv[0]);
> > -			exit(NFT_EXIT_SUCCESS);
> > +			exit(0);
> 
> Better use the standard EXIT_FAILURE and EXIT_SUCCESS here? Instead of
> hardcoded 0 and 1 values.

ACK, I'll prepare v3.

Thanks, Phil

Re: [nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

On Mon, Nov 13, 2017 at 01:31:00PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 10, 2017 at 12:27:15PM +0100, Phil Sutter wrote:
> > diff --git a/src/main.c b/src/main.c
> > index 529bedffc2e3b..8d03f8989b1fc 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
> >  		switch (val) {
> >  		case OPT_HELP:
> >  			show_help(argv[0]);
> > -			exit(NFT_EXIT_SUCCESS);
> > +			exit(0);
> 
> Better use the standard EXIT_FAILURE and EXIT_SUCCESS here? Instead of
> hardcoded 0 and 1 values.

While at it, should I convert nft_run_cmd_from_*() to return those
macros as well? This would of course mean changing failure case from -1
to 1, but would streamline calls from nft.c. What do you think?

Cheers, Phil

Re: [nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Pablo Neira Ayuso]

On Mon, Nov 13, 2017 at 02:49:04PM +0100, Phil Sutter wrote:
> On Mon, Nov 13, 2017 at 01:31:00PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Nov 10, 2017 at 12:27:15PM +0100, Phil Sutter wrote:
> > > diff --git a/src/main.c b/src/main.c
> > > index 529bedffc2e3b..8d03f8989b1fc 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
> > >  		switch (val) {
> > >  		case OPT_HELP:
> > >  			show_help(argv[0]);
> > > -			exit(NFT_EXIT_SUCCESS);
> > > +			exit(0);
> > 
> > Better use the standard EXIT_FAILURE and EXIT_SUCCESS here? Instead of
> > hardcoded 0 and 1 values.
> 
> While at it, should I convert nft_run_cmd_from_*() to return those
> macros as well? This would of course mean changing failure case from -1
> to 1, but would streamline calls from nft.c. What do you think?

I prefer to keep EXIT_* in main() only.

To me, -1 is the standard way to return an error in functions. So
that's fine to me to keep it around.

So my suggestion is, you focus on features and fixes at this stage,
cleanups may come later on, so we avoid too much code churning, ie.
things that we may need to undo later on when extending things.

Cleanups can follow up once dust settles down a bit.

Let me know, thanks.

Re: [nft PATCH v2] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

On Mon, Nov 13, 2017 at 02:53:50PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Nov 13, 2017 at 02:49:04PM +0100, Phil Sutter wrote:
> > On Mon, Nov 13, 2017 at 01:31:00PM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Nov 10, 2017 at 12:27:15PM +0100, Phil Sutter wrote:
> > > > diff --git a/src/main.c b/src/main.c
> > > > index 529bedffc2e3b..8d03f8989b1fc 100644
> > > > --- a/src/main.c
> > > > +++ b/src/main.c
> > > > @@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
> > > >  		switch (val) {
> > > >  		case OPT_HELP:
> > > >  			show_help(argv[0]);
> > > > -			exit(NFT_EXIT_SUCCESS);
> > > > +			exit(0);
> > > 
> > > Better use the standard EXIT_FAILURE and EXIT_SUCCESS here? Instead of
> > > hardcoded 0 and 1 values.
> > 
> > While at it, should I convert nft_run_cmd_from_*() to return those
> > macros as well? This would of course mean changing failure case from -1
> > to 1, but would streamline calls from nft.c. What do you think?
> 
> I prefer to keep EXIT_* in main() only.
> 
> To me, -1 is the standard way to return an error in functions. So
> that's fine to me to keep it around.
> 
> So my suggestion is, you focus on features and fixes at this stage,
> cleanups may come later on, so we avoid too much code churning, ie.
> things that we may need to undo later on when extending things.
> 
> Cleanups can follow up once dust settles down a bit.
> 
> Let me know, thanks.

OK, fine with me! Thanks for your quick feedback.

Cheers, Phil

[nft PATCH v3] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

Apart from SUCCESS/FAILURE, these codes were not used by library
functions simply because NOMEM and NONL conditions lead to calling
exit() instead of propagating the error condition back up the call
stack.

Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
Usually errno will then contain more details about what happened and/or
there are messages in erec.

Calls to exit()/return in main() are adjusted to stay compatible.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Fixed return code of main() if nft_run_cmd_from_*() fails, this broke
  tests/shell.

Changes since v2:
- Use standard macros EXIT_SUCCESS and EXIT_FAILURE when
  exiting/returning from nft main() function.
---
 include/nftables.h          |  7 +++++++
 include/nftables/nftables.h | 10 ----------
 src/libnftables.c           | 10 +++++-----
 src/main.c                  | 22 +++++++++++-----------
 4 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index d69079fe03e3a..3bfa33e5cb336 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -35,6 +35,13 @@ struct nft_ctx {
 	uint32_t		flags;
 };
 
+enum nftables_exit_codes {
+	NFT_EXIT_SUCCESS	= 0,
+	NFT_EXIT_FAILURE	= 1,
+	NFT_EXIT_NOMEM		= 2,
+	NFT_EXIT_NONL		= 3,
+};
+
 struct input_descriptor;
 struct location {
 	const struct input_descriptor		*indesc;
diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
index 4211be76e6e40..8e59f2b2a59ab 100644
--- a/include/nftables/nftables.h
+++ b/include/nftables/nftables.h
@@ -38,16 +38,6 @@ enum nft_numeric_level {
  */
 #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);
 
diff --git a/src/libnftables.c b/src/libnftables.c
index dc6a5fdf32640..e8fa6742f7d17 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -272,7 +272,7 @@ static const struct input_descriptor indesc_cmdline = {
 
 int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 {
-	int rc = NFT_EXIT_SUCCESS;
+	int rc = 0;
 	struct parser_state state;
 	LIST_HEAD(msgs);
 	void *scanner;
@@ -284,7 +284,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char *buf, size_t buflen)
 	scanner_push_buffer(scanner, &indesc_cmdline, buf);
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
@@ -306,18 +306,18 @@ int nft_run_cmd_from_filename(struct nft_ctx *nft, const char *filename)
 	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
 			  nft->debug_mask, &nft->output);
 	if (rc < 0)
-		return NFT_EXIT_FAILURE;
+		return -1;
 
 	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;
+		rc = -1;
 		goto err;
 	}
 
 	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
-		rc = NFT_EXIT_FAILURE;
+		rc = -1;
 err:
 	fp = nft_ctx_set_output(nft, stderr);
 	erec_print_list(&nft->output, &msgs, nft->debug_mask);
diff --git a/src/main.c b/src/main.c
index 529bedffc2e3b..ff7878c94ccb3 100644
--- a/src/main.c
+++ b/src/main.c
@@ -183,11 +183,11 @@ int main(int argc, char * const *argv)
 		switch (val) {
 		case OPT_HELP:
 			show_help(argv[0]);
-			exit(NFT_EXIT_SUCCESS);
+			exit(EXIT_SUCCESS);
 		case OPT_VERSION:
 			printf("%s v%s (%s)\n",
 			       PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME);
-			exit(NFT_EXIT_SUCCESS);
+			exit(EXIT_SUCCESS);
 		case OPT_CHECK:
 			nft_ctx_set_dry_run(nft, true);
 			break;
@@ -202,7 +202,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr,
 					"Failed to add include path '%s'\n",
 					optarg);
-				exit(NFT_EXIT_FAILURE);
+				exit(EXIT_FAILURE);
 			}
 			break;
 		case OPT_NUMERIC:
@@ -211,7 +211,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr, "Too many numeric options "
 						"used, max. %u\n",
 					NFT_NUMERIC_ALL);
-				exit(NFT_EXIT_FAILURE);
+				exit(EXIT_FAILURE);
 			}
 			nft_ctx_output_set_numeric(nft, numeric + 1);
 			break;
@@ -241,7 +241,7 @@ int main(int argc, char * const *argv)
 				if (i == array_size(debug_param)) {
 					fprintf(stderr, "invalid debug parameter `%s'\n",
 						optarg);
-					exit(NFT_EXIT_FAILURE);
+					exit(EXIT_FAILURE);
 				}
 
 				if (end == NULL)
@@ -257,7 +257,7 @@ int main(int argc, char * const *argv)
 			nft_ctx_output_set_echo(nft, true);
 			break;
 		case OPT_INVALID:
-			exit(NFT_EXIT_FAILURE);
+			exit(EXIT_FAILURE);
 		}
 	}
 
@@ -272,19 +272,19 @@ int main(int argc, char * const *argv)
 				strcat(buf, " ");
 		}
 		strcat(buf, "\n");
-		rc = nft_run_cmd_from_buffer(nft, buf, len + 2);
+		rc = !!nft_run_cmd_from_buffer(nft, buf, len + 2);
 	} else if (filename != NULL) {
-		rc = nft_run_cmd_from_filename(nft, filename);
+		rc = !!nft_run_cmd_from_filename(nft, filename);
 	} else if (interactive) {
 		if (cli_init(nft) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
-			exit(NFT_EXIT_FAILURE);
+			exit(EXIT_FAILURE);
 		}
-		return 0;
+		return EXIT_SUCCESS;
 	} else {
 		fprintf(stderr, "%s: no command specified\n", argv[0]);
-		exit(NFT_EXIT_FAILURE);
+		exit(EXIT_FAILURE);
 	}
 
 	xfree(buf);
-- 
2.13.1

Re: [nft PATCH v3] libnftables: Unexport enum nftables_exit_codes

[Pablo Neira Ayuso]

On Mon, Nov 13, 2017 at 03:08:16PM +0100, Phil Sutter wrote:
> Apart from SUCCESS/FAILURE, these codes were not used by library
> functions simply because NOMEM and NONL conditions lead to calling
> exit() instead of propagating the error condition back up the call
> stack.
> 
> Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
> Usually errno will then contain more details about what happened and/or
> there are messages in erec.
> 
> Calls to exit()/return in main() are adjusted to stay compatible.

Also applied, thanks.

BTW, I think you mentioned you planned to change all
memory_allocation_error() to pass up the error to the client
application.

Let me know, if you don't have time for this, no worries if too busy.

Thanks!

Re: [nft PATCH v3] libnftables: Unexport enum nftables_exit_codes

[Phil Sutter]

Hi Pablo,

On Thu, Nov 16, 2017 at 02:33:32PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Nov 13, 2017 at 03:08:16PM +0100, Phil Sutter wrote:
> > Apart from SUCCESS/FAILURE, these codes were not used by library
> > functions simply because NOMEM and NONL conditions lead to calling
> > exit() instead of propagating the error condition back up the call
> > stack.
> > 
> > Instead, make nft_run_cmd_from_*() return either 0 or -1 on error.
> > Usually errno will then contain more details about what happened and/or
> > there are messages in erec.
> > 
> > Calls to exit()/return in main() are adjusted to stay compatible.
> 
> Also applied, thanks.
> 
> BTW, I think you mentioned you planned to change all
> memory_allocation_error() to pass up the error to the client
> application.
> 
> Let me know, if you don't have time for this, no worries if too busy.

I looked into it once, but didn't pursue much further. This requires
some effort, since code everywhere just assumes (e.g.) memory allocation
to succeed so there is no error path at all in many places.

OTOH, I wasn't sure whether adding this is feasible at all - if memory
allocation fails, we're usually in big trouble and error propagation
might not work anymore as well (e.g. allocation of erec items). Sure,
bugs like 'malloc(-1)' would be handled properly, of course.

Not sure about netlink errors: Ideally, the library would check this
early (e.g. during context allocation), but of course syscalls like
socket() could still fail later.

Cheers, Phil