[PATCH nft 1/3] src: move nf_sock into nft_ctx structure

[PATCH nft 1/3] src: move nf_sock into nft_ctx structure

[Pablo Neira Ayuso]

The idea is to provide a simplistic API for non-netlink wise people.
Add a field in struct nft_ctx to store the socket.

The advanced API that we're planning will just simply leave this unset,
since netlink IO will be exposed.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
@Eric: Still pending the specific interface to print. As I said, I would prefer
       functions with stricting typing. I think after this you have the simple
       API that you need.

 include/nftables.h |  3 +++
 src/main.c         | 26 +++++++++++---------------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index 256b06ee33fc..5035567a75fd 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -38,7 +38,10 @@ struct nft_cache {
 	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;
diff --git a/src/main.c b/src/main.c
index eecd430a2e3f..a891832ec5d6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -305,7 +305,6 @@ static void nft_ctx_free(const struct nft_ctx *ctx)
 }
 
 static int nft_run_cmd_from_buffer(struct nft_ctx *nft,
-				   struct mnl_socket *nf_sock,
 				   char *buf, size_t buflen)
 {
 	int rc = NFT_EXIT_SUCCESS;
@@ -313,11 +312,11 @@ static int nft_run_cmd_from_buffer(struct nft_ctx *nft,
 	LIST_HEAD(msgs);
 	void *scanner;
 
-	parser_init(nf_sock, &nft->cache, &state, &msgs, nft->debug_mask);
+	parser_init(nft->nf_sock, &nft->cache, &state, &msgs, nft->debug_mask);
 	scanner = scanner_init(&state);
 	scanner_push_buffer(scanner, &indesc_cmdline, buf);
 
-	if (nft_run(nft, nf_sock, scanner, &state, &msgs) != 0)
+	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
 		rc = NFT_EXIT_FAILURE;
 
 	erec_print_list(stderr, &msgs, nft->debug_mask);
@@ -326,26 +325,24 @@ static int nft_run_cmd_from_buffer(struct nft_ctx *nft,
 	return rc;
 }
 
-static int nft_run_cmd_from_filename(struct nft_ctx *nft,
-				     struct mnl_socket *nf_sock,
-				     const char *filename)
+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;
 
-	rc = cache_update(nf_sock, &nft->cache, CMD_INVALID, &msgs,
+	rc = cache_update(nft->nf_sock, &nft->cache, CMD_INVALID, &msgs,
 			  nft->debug_mask);
 	if (rc < 0)
 		return NFT_EXIT_FAILURE;
 
-	parser_init(nf_sock, &nft->cache, &state, &msgs, nft->debug_mask);
+	parser_init(nft->nf_sock, &nft->cache, &state, &msgs, nft->debug_mask);
 	scanner = scanner_init(&state);
 	if (scanner_read_file(scanner, filename, &internal_location) < 0)
 		goto err;
 
-	if (nft_run(nft, nf_sock, scanner, &state, &msgs) != 0)
+	if (nft_run(nft, nft->nf_sock, scanner, &state, &msgs) != 0)
 		rc = NFT_EXIT_FAILURE;
 err:
 	erec_print_list(stderr, &msgs, nft->debug_mask);
@@ -359,13 +356,12 @@ int main(int argc, char * const *argv)
 	char *buf = NULL, *filename = NULL;
 	unsigned int len;
 	bool interactive = false;
-	struct mnl_socket *nf_sock;
 	struct parser_state state;
 	int i, val, rc;
 
 	nft = nft_ctx_new();
 
-	nf_sock = netlink_open_sock();
+	nft->nf_sock = netlink_open_sock();
 	while (1) {
 		val = getopt_long(argc, argv, OPTSTRING, options, NULL);
 		if (val == -1)
@@ -460,11 +456,11 @@ int main(int argc, char * const *argv)
 				strcat(buf, " ");
 		}
 		strcat(buf, "\n");
-		rc = nft_run_cmd_from_buffer(nft, nf_sock, buf, len + 2);
+		rc = nft_run_cmd_from_buffer(nft, buf, len + 2);
 	} else if (filename != NULL) {
-		rc = nft_run_cmd_from_filename(nft, nf_sock, filename);
+		rc = nft_run_cmd_from_filename(nft, filename);
 	} else if (interactive) {
-		if (cli_init(nft, nf_sock, &state) < 0) {
+		if (cli_init(nft, nft->nf_sock, &state) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
 			exit(NFT_EXIT_FAILURE);
@@ -476,7 +472,7 @@ int main(int argc, char * const *argv)
 	}
 
 	xfree(buf);
-	netlink_close_sock(nf_sock);
+	netlink_close_sock(nft->nf_sock);
 	nft_ctx_free(nft);
 
 	return rc;
-- 
2.1.4

[PATCH nft 2/3] netlink: remove nfsock_open()

[Pablo Neira Ayuso]

Just merge this code to netlink_open_sock().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 90f8486581fe..b6336e836e88 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -48,22 +48,16 @@ const struct location netlink_location = {
 	.indesc	= &indesc_netlink,
 };
 
-static struct mnl_socket *nfsock_open(void)
-{
-	struct mnl_socket *s;
-
-	s = mnl_socket_open(NETLINK_NETFILTER);
-	if (s == NULL)
-		netlink_init_error();
-	return s;
-}
-
 struct mnl_socket *netlink_open_sock(void)
 {
 	struct mnl_socket *nf_sock;
 
-	nf_sock = nfsock_open();
+	nf_sock = mnl_socket_open(NETLINK_NETFILTER);
+	if (nf_sock == NULL)
+		netlink_init_error();
+
 	fcntl(mnl_socket_get_fd(nf_sock), F_SETFL, O_NONBLOCK);
+
 	return nf_sock;
 }
 
-- 
2.1.4

[PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Pablo Neira Ayuso]

Add these two new functions to set up netlink sockets in the global
context structure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c
index a891832ec5d6..fce9bfeca100 100644
--- a/src/main.c
+++ b/src/main.c
@@ -298,12 +298,20 @@ static struct nft_ctx *nft_ctx_new(void)
 
 static void nft_ctx_free(const struct nft_ctx *ctx)
 {
+	if (ctx->nf_sock)
+		netlink_close_sock(ctx->nf_sock);
+
 	iface_cache_release();
 	cache_release(&nft->cache);
 	xfree(ctx);
 	nft_exit();
 }
 
+static void nft_ctx_netlink_init(struct nft_ctx *ctx)
+{
+	ctx->nf_sock = netlink_open_sock();
+}
+
 static int nft_run_cmd_from_buffer(struct nft_ctx *nft,
 				   char *buf, size_t buflen)
 {
@@ -361,7 +369,8 @@ int main(int argc, char * const *argv)
 
 	nft = nft_ctx_new();
 
-	nft->nf_sock = netlink_open_sock();
+	nft_ctx_netlink_init(nft);
+
 	while (1) {
 		val = getopt_long(argc, argv, OPTSTRING, options, NULL);
 		if (val == -1)
@@ -472,7 +481,6 @@ int main(int argc, char * const *argv)
 	}
 
 	xfree(buf);
-	netlink_close_sock(nft->nf_sock);
 	nft_ctx_free(nft);
 
 	return rc;
-- 
2.1.4

Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Pablo Neira Ayuso]

On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> Add these two new functions to set up netlink sockets in the global
> context structure.

We can alternatively call this nft_ctx_netlink_auto() if prefer.

I'm just trying to skip the type/flag field for nft_ctx_alloc().

Does this look acceptable to you to have this extra API to request
libnftables to deal with IO details too?

Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Phil Sutter]

Hi Pablo,

On Fri, Sep 01, 2017 at 12:17:33PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> > Add these two new functions to set up netlink sockets in the global
> > context structure.
> 
> We can alternatively call this nft_ctx_netlink_auto() if prefer.
> 
> I'm just trying to skip the type/flag field for nft_ctx_alloc().
> 
> Does this look acceptable to you to have this extra API to request
> libnftables to deal with IO details too?

I think we could do it in a simpler way:

| /* create an mnl netlink socket object */
| struct mnl_socket *netlink_open_sock(void);
| 
| /* create nft context, optionally passing mnl socket object returned
|  * from netlink_open_sock()
|  * Calling nft_ctx_new(NULL) is equivalent to calling
|  * nft_ctx_new(netlink_open_sock())
|  */
| static struct nft_ctx *nft_ctx_new(struct mnl_socket *nf_sock);

This way we allow the application to control mnl_socket object, provide
a simple API for applications which don't need that and at the same time
always have ctx->nf_sock point to the socket so we can further simplify
things.

What do you think?

Cheers, Phil

Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Sep 01, 2017 at 12:50:49PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Fri, Sep 01, 2017 at 12:17:33PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> > > Add these two new functions to set up netlink sockets in the global
> > > context structure.
> > 
> > We can alternatively call this nft_ctx_netlink_auto() if prefer.
> > 
> > I'm just trying to skip the type/flag field for nft_ctx_alloc().
> > 
> > Does this look acceptable to you to have this extra API to request
> > libnftables to deal with IO details too?
> 
> I think we could do it in a simpler way:
> 
> | /* create an mnl netlink socket object */
> | struct mnl_socket *netlink_open_sock(void);
> | 
> | /* create nft context, optionally passing mnl socket object returned
> |  * from netlink_open_sock()
> |  * Calling nft_ctx_new(NULL) is equivalent to calling
> |  * nft_ctx_new(netlink_open_sock())
> |  */
> | static struct nft_ctx *nft_ctx_new(struct mnl_socket *nf_sock);
> 
> This way we allow the application to control mnl_socket object, provide
> a simple API for applications which don't need that and at the same time
> always have ctx->nf_sock point to the socket so we can further simplify
> things.
> 
> What do you think?

I don't want to expose the mnl_socket for the simple API.

Once we expose the socket, we're allowing people to modify behaviour
via setsockopt() toggles. Then, we'll have to tell people not to
modify socket options for the simple API, or it can be worst: someone
will try to fix this by adding lots of branches to deal with every
socket option combination in libnftables.

So this is an intentional design decision here.

For people willing to do their own netlink IO, we should push them to
use the advanced API.

Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Phil Sutter]

Hi Pablo,

On Fri, Sep 01, 2017 at 12:58:59PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Sep 01, 2017 at 12:50:49PM +0200, Phil Sutter wrote:
> > On Fri, Sep 01, 2017 at 12:17:33PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> > > > Add these two new functions to set up netlink sockets in the global
> > > > context structure.
> > > 
> > > We can alternatively call this nft_ctx_netlink_auto() if prefer.
> > > 
> > > I'm just trying to skip the type/flag field for nft_ctx_alloc().
> > > 
> > > Does this look acceptable to you to have this extra API to request
> > > libnftables to deal with IO details too?
> > 
> > I think we could do it in a simpler way:
> > 
> > | /* create an mnl netlink socket object */
> > | struct mnl_socket *netlink_open_sock(void);
> > | 
> > | /* create nft context, optionally passing mnl socket object returned
> > |  * from netlink_open_sock()
> > |  * Calling nft_ctx_new(NULL) is equivalent to calling
> > |  * nft_ctx_new(netlink_open_sock())
> > |  */
> > | static struct nft_ctx *nft_ctx_new(struct mnl_socket *nf_sock);
> > 
> > This way we allow the application to control mnl_socket object, provide
> > a simple API for applications which don't need that and at the same time
> > always have ctx->nf_sock point to the socket so we can further simplify
> > things.
> > 
> > What do you think?
> 
> I don't want to expose the mnl_socket for the simple API.

Well, technically my proposal changes nft_ctx_new() to be a shared
function between simple and advanced API. Simple users passing NULL as
parameter won't have to take care of socket init and therefore don't
have a separate mnl_socket object they could manipulate.

> Once we expose the socket, we're allowing people to modify behaviour
> via setsockopt() toggles. Then, we'll have to tell people not to
> modify socket options for the simple API, or it can be worst: someone
> will try to fix this by adding lots of branches to deal with every
> socket option combination in libnftables.
> 
> So this is an intentional design decision here.
> 
> For people willing to do their own netlink IO, we should push them to
> use the advanced API.

Your patches seem to make the simple API unnecessarily complicated by
forcing users to call nft_ctx_netlink_init(). Why not keep this in
nft_ctx_new() and provide an alternative to struct nft_ctx for advanced
API? This way simple and advanced functions could be easily identified
by the type of their first parameter.

Maybe I'm also overlooking something regarding advanced API - can you
maybe give an example of a set of function declarations it would consist
of?

Thanks, Phil

Re: [PATCH nft 3/3] src: add nft_ctx_netlink_init()

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Sep 01, 2017 at 12:14:07PM +0200, Pablo Neira Ayuso wrote:
> > Add these two new functions to set up netlink sockets in the global
> > context structure.
> 
> We can alternatively call this nft_ctx_netlink_auto() if prefer.

I think its good as-is.

I also agree with not exposing libmnl anywhere in the api.

Re: [PATCH nft 1/3] src: move nf_sock into nft_ctx structure

[Pablo Neira Ayuso]

On Fri, Sep 01, 2017 at 12:14:05PM +0200, Pablo Neira Ayuso wrote:
> The idea is to provide a simplistic API for non-netlink wise people.
> Add a field in struct nft_ctx to store the socket.
> 
> The advanced API that we're planning will just simply leave this unset,
> since netlink IO will be exposed.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> @Eric: Still pending the specific interface to print. As I said, I would prefer
>        functions with stricting typing. I think after this you have the simple
>        API that you need.

I have just pushed out this. Just follow up with patches in case you
propose to change anything. I just wanted to make sure we don't have
to rebase again.