json_cmd_assoc and cmd

json_cmd_assoc and cmd

[Pablo Neira Ayuso]

Hi Phil,

Why json_cmd_assoc is not placed in struct cmd instead? I mean, just
store the json_t *json in cmd?

This code could be simplified if you store this information in the
command object.

Thanks.

Re: json_cmd_assoc and cmd

[Phil Sutter]

Hi Pablo,

On Tue, Jul 16, 2019 at 08:31:01PM +0200, Pablo Neira Ayuso wrote:
> Why json_cmd_assoc is not placed in struct cmd instead? I mean, just
> store the json_t *json in cmd?

The global list (json_cmd_list) is used in json_events_cb(). Unless I
miss something, the cmd list is not available from struct
netlink_mon_handler.

Maybe I could move struct cmds list head into struct nft_ctx?

Thanks, Phil

Re: json_cmd_assoc and cmd

[Pablo Neira Ayuso]

On Tue, Jul 16, 2019 at 09:02:24PM +0200, Phil Sutter wrote:
> Hi Pablo,
>
> On Tue, Jul 16, 2019 at 08:31:01PM +0200, Pablo Neira Ayuso wrote:
> > Why json_cmd_assoc is not placed in struct cmd instead? I mean, just
> > store the json_t *json in cmd?
>
> The global list (json_cmd_list) is used in json_events_cb(). Unless I
> miss something, the cmd list is not available from struct
> netlink_mon_handler.

I see, thanks for explaining.

> Maybe I could move struct cmds list head into struct nft_ctx?

Probably place this in netlink_ctx?

We could also store num_cmds there too.

BTW, not directly related to this, but isn't this strange?

        list_for_each_entry(cmd, cmds, list) {
                memset(&ctx, 0, sizeof(ctx));
                ctx.msgs = msgs;
                ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
                ctx.batch = batch;
                ctx.nft = nft;
                init_list_head(&ctx.list);
                ret = do_command(&ctx, cmd);
                ...

ctx is reset over and over again. Then, recycled here:

                ret = mnl_batch_talk(&ctx, &err_list, num_cmds);

I wonder if we can get this better.

Re: json_cmd_assoc and cmd

[Jeremy Sowden]

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

On 2019-07-16, at 21:39:03 +0200, Pablo Neira Ayuso wrote:
> BTW, not directly related to this, but isn't this strange?
>
>         list_for_each_entry(cmd, cmds, list) {
>                 memset(&ctx, 0, sizeof(ctx));
>                 ctx.msgs = msgs;
>                 ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
>                 ctx.batch = batch;
>                 ctx.nft = nft;
>                 init_list_head(&ctx.list);
>                 ret = do_command(&ctx, cmd);
>                 ...
>
> ctx is reset over and over again. Then, recycled here:
>
>                 ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
>
> I wonder if we can get this better.

Something like this?

        ...
	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };
        ...

	ctx.batch = batch = mnl_batch_init();
	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
	list_for_each_entry(cmd, cmds, list) {
		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
		init_list_head(&ctx.list);
		ret = do_command(&ctx, cmd);
		...
	}

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: json_cmd_assoc and cmd

[Phil Sutter]

Hi,

On Thu, Jul 18, 2019 at 01:37:04PM +0100, Jeremy Sowden wrote:
> On 2019-07-16, at 21:39:03 +0200, Pablo Neira Ayuso wrote:
> > BTW, not directly related to this, but isn't this strange?
> >
> >         list_for_each_entry(cmd, cmds, list) {
> >                 memset(&ctx, 0, sizeof(ctx));
> >                 ctx.msgs = msgs;
> >                 ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> >                 ctx.batch = batch;
> >                 ctx.nft = nft;
> >                 init_list_head(&ctx.list);
> >                 ret = do_command(&ctx, cmd);
> >                 ...
> >
> > ctx is reset over and over again. Then, recycled here:
> >
> >                 ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
> >
> > I wonder if we can get this better.
> 
> Something like this?
> 
>         ...
> 	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };
>         ...
> 
> 	ctx.batch = batch = mnl_batch_init();
> 	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
> 	list_for_each_entry(cmd, cmds, list) {
> 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> 		init_list_head(&ctx.list);
> 		ret = do_command(&ctx, cmd);
> 		...
> 	}

Yes, that at least simplifies the foreach loop a bit. I wonder though
if we could eliminate struct netlink_ctx altogether by moving pointers
into struct nft_ctx.

Pablo, do you think that's feasible?

Cheers, Phil

Re: json_cmd_assoc and cmd

[Pablo Neira Ayuso]

On Thu, Jul 18, 2019 at 01:37:04PM +0100, Jeremy Sowden wrote:
> On 2019-07-16, at 21:39:03 +0200, Pablo Neira Ayuso wrote:
> > BTW, not directly related to this, but isn't this strange?
> >
> >         list_for_each_entry(cmd, cmds, list) {
> >                 memset(&ctx, 0, sizeof(ctx));
> >                 ctx.msgs = msgs;
> >                 ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> >                 ctx.batch = batch;
> >                 ctx.nft = nft;
> >                 init_list_head(&ctx.list);
> >                 ret = do_command(&ctx, cmd);
> >                 ...
> >
> > ctx is reset over and over again. Then, recycled here:
> >
> >                 ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
> >
> > I wonder if we can get this better.
> 
> Something like this?

Yes, something like that would get things in better shape I think,
more comments below.

> 	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };
>         ...
> 
> 	ctx.batch = batch = mnl_batch_init();
> 	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
> 	list_for_each_entry(cmd, cmds, list) {
> 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> 		init_list_head(&ctx.list);

I think we don't need to re-initialize this list over and over again
(from what I see when doing: git grep "ctx->list").

This always does list_splice_tail_init() to attach the object list
where they belong.

You can probably add something like:

        if (!list_empty(&ctx->list))
                BUG("command list is not empty\n");

I would make a patch and run tests/shell and tests/py to check if what
I'm suggesting this fine :-)

> 		ret = do_command(&ctx, cmd);
> 		...

Thanks!

Re: json_cmd_assoc and cmd

[Jeremy Sowden]

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

On 2019-07-18, at 16:57:22 +0200, Pablo Neira Ayuso wrote:
> On Thu, Jul 18, 2019 at 01:37:04PM +0100, Jeremy Sowden wrote:
> > On 2019-07-16, at 21:39:03 +0200, Pablo Neira Ayuso wrote:
> > > BTW, not directly related to this, but isn't this strange?
> > >
> > >         list_for_each_entry(cmd, cmds, list) {
> > >                 memset(&ctx, 0, sizeof(ctx));
> > >                 ctx.msgs = msgs;
> > >                 ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> > >                 ctx.batch = batch;
> > >                 ctx.nft = nft;
> > >                 init_list_head(&ctx.list);
> > >                 ret = do_command(&ctx, cmd);
> > >                 ...
> > >
> > > ctx is reset over and over again. Then, recycled here:
> > >
> > >                 ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
> > >
> > > I wonder if we can get this better.
> >
> > Something like this?
>
> Yes, something like that would get things in better shape I think,
> more comments below.
>
> > 	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };
> >         ...
> >
> > 	ctx.batch = batch = mnl_batch_init();
> > 	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
> > 	list_for_each_entry(cmd, cmds, list) {
> > 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
> > 		init_list_head(&ctx.list);
>
> I think we don't need to re-initialize this list over and over again
> (from what I see when doing: git grep "ctx->list").
>
> This always does list_splice_tail_init() to attach the object list
> where they belong.

Right.  Got it.

> You can probably add something like:
>
>         if (!list_empty(&ctx->list))
>                 BUG("command list is not empty\n");
>
> I would make a patch and run tests/shell and tests/py to check if what
> I'm suggesting this fine :-)

I've compiled the changes I outlined above and run `make check`.  Will
add the list changes and submit a patch once I've tested them.

> > 		ret = do_command(&ctx, cmd);
> > 		...

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH nft] libnftables: got rid of repeated initialization of netlink_ctx variable in loop.

[Jeremy Sowden]

Most members in the context doesn't change, so there is no need to
memset it and reassign most of its members on every iteration.  Moved
that code out of the loop.

Fixes: 49900d448ac9 ("libnftables: Move library stuff out of main.c")
Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/libnftables.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 2f77a7709e2c..834ea661a146 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -23,7 +23,7 @@ static int nft_netlink(struct nft_ctx *nft,
 {
 	uint32_t batch_seqnum, seqnum = 0, num_cmds = 0;
 	struct nftnl_batch *batch;
-	struct netlink_ctx ctx;
+	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };
 	struct cmd *cmd;
 	struct mnl_err *err, *tmp;
 	LIST_HEAD(err_list);
@@ -32,16 +32,13 @@ static int nft_netlink(struct nft_ctx *nft,
 	if (list_empty(cmds))
 		return 0;
 
-	batch = mnl_batch_init();
+	init_list_head(&ctx.list);
+
+	ctx.batch = batch = mnl_batch_init();
 
 	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
 	list_for_each_entry(cmd, cmds, list) {
-		memset(&ctx, 0, sizeof(ctx));
-		ctx.msgs = msgs;
 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
-		ctx.batch = batch;
-		ctx.nft = nft;
-		init_list_head(&ctx.list);
 		ret = do_command(&ctx, cmd);
 		if (ret < 0) {
 			netlink_io_error(&ctx, &cmd->location,
-- 
2.20.1

Re: [PATCH nft] libnftables: got rid of repeated initialization of netlink_ctx variable in loop.

[Phil Sutter]

Hi,

On Thu, Jul 18, 2019 at 10:05:52PM +0100, Jeremy Sowden wrote:
> Most members in the context doesn't change, so there is no need to
> memset it and reassign most of its members on every iteration.  Moved
> that code out of the loop.
> 
> Fixes: 49900d448ac9 ("libnftables: Move library stuff out of main.c")

This commit merely moved the code from src/main.c into the current
location. I would rather point at a72315d2bad47 ("src: add rule batching
support"), which introduced the loop (and already contains some of the
pointless assignments). Note that commit is from 2013. :)

> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  src/libnftables.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 2f77a7709e2c..834ea661a146 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -23,7 +23,7 @@ static int nft_netlink(struct nft_ctx *nft,
>  {
>  	uint32_t batch_seqnum, seqnum = 0, num_cmds = 0;
>  	struct nftnl_batch *batch;
> -	struct netlink_ctx ctx;
> +	struct netlink_ctx ctx = { .msgs = msgs, .nft = nft };

Use '.list = LIST_HEAD_INIT(ctx.list),' here. Refer to cache_update() in
src/rule.c for inspiration. :)

>  	struct cmd *cmd;
>  	struct mnl_err *err, *tmp;
>  	LIST_HEAD(err_list);
> @@ -32,16 +32,13 @@ static int nft_netlink(struct nft_ctx *nft,
>  	if (list_empty(cmds))
>  		return 0;
>  
> -	batch = mnl_batch_init();
> +	init_list_head(&ctx.list);

Drop this.

> +
> +	ctx.batch = batch = mnl_batch_init();

Move ctx.batch init into declaration as well. Drop dedicated 'batch'
variable and replace by 'ctx.batch'.

Thanks, Phil

[PATCH nft v2 0/2] netlink_ctx initialization fixes.

[Jeremy Sowden]

A couple of patches to tidy up initialization of a pair of netlink_ctx
variables.

Since v1 (based on Phil's feedback -- thanks, Phil):

 * Eliminated batch local variable from first patch.
 * Do initialization of .list and .batch into struct initializer from
   first patch.
 * Updated commit in "Fixes:" tag in first patch.
 * Added second patch.

Jeremy Sowden (2):
  libnftables: got rid of repeated initialization of netlink_ctx
    variable in loop.
  rule: removed duplicate member initializer.

 src/libnftables.c | 23 ++++++++++-------------
 src/rule.c        |  1 -
 2 files changed, 10 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH nft v2 1/2] libnftables: got rid of repeated initialization of netlink_ctx variable in loop.

[Jeremy Sowden]

Most members in the context doesn't change, so there is no need to
memset it and reassign them on every iteration.  Moved that code out of
the loop.

Fixes: a72315d2bad4 ("src: add rule batching support")
Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/libnftables.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/src/libnftables.c b/src/libnftables.c
index 2f77a7709e2c..4a139c58b2b3 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -22,8 +22,12 @@ static int nft_netlink(struct nft_ctx *nft,
 		       struct mnl_socket *nf_sock)
 {
 	uint32_t batch_seqnum, seqnum = 0, num_cmds = 0;
-	struct nftnl_batch *batch;
-	struct netlink_ctx ctx;
+	struct netlink_ctx ctx = {
+		.nft  = nft,
+		.msgs = msgs,
+		.list = LIST_HEAD_INIT(ctx.list),
+		.batch = mnl_batch_init(),
+	};
 	struct cmd *cmd;
 	struct mnl_err *err, *tmp;
 	LIST_HEAD(err_list);
@@ -32,16 +36,9 @@ static int nft_netlink(struct nft_ctx *nft,
 	if (list_empty(cmds))
 		return 0;
 
-	batch = mnl_batch_init();
-
-	batch_seqnum = mnl_batch_begin(batch, mnl_seqnum_alloc(&seqnum));
+	batch_seqnum = mnl_batch_begin(ctx.batch, mnl_seqnum_alloc(&seqnum));
 	list_for_each_entry(cmd, cmds, list) {
-		memset(&ctx, 0, sizeof(ctx));
-		ctx.msgs = msgs;
 		ctx.seqnum = cmd->seqnum = mnl_seqnum_alloc(&seqnum);
-		ctx.batch = batch;
-		ctx.nft = nft;
-		init_list_head(&ctx.list);
 		ret = do_command(&ctx, cmd);
 		if (ret < 0) {
 			netlink_io_error(&ctx, &cmd->location,
@@ -52,9 +49,9 @@ static int nft_netlink(struct nft_ctx *nft,
 		num_cmds++;
 	}
 	if (!nft->check)
-		mnl_batch_end(batch, mnl_seqnum_alloc(&seqnum));
+		mnl_batch_end(ctx.batch, mnl_seqnum_alloc(&seqnum));
 
-	if (!mnl_batch_ready(batch))
+	if (!mnl_batch_ready(ctx.batch))
 		goto out;
 
 	ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
@@ -83,7 +80,7 @@ static int nft_netlink(struct nft_ctx *nft,
 		}
 	}
 out:
-	mnl_batch_reset(batch);
+	mnl_batch_reset(ctx.batch);
 	return ret;
 }
 
-- 
2.20.1

Re: [PATCH nft v2 1/2] libnftables: got rid of repeated initialization of netlink_ctx variable in loop.

[Phil Sutter]

On Fri, Jul 19, 2019 at 12:10:09PM +0100, Jeremy Sowden wrote:
> Most members in the context doesn't change, so there is no need to
> memset it and reassign them on every iteration.  Moved that code out of
> the loop.
> 
> Fixes: a72315d2bad4 ("src: add rule batching support")
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>

Acked-by: Phil Sutter <phil@nwl.cc>

[PATCH nft v2 2/2] rule: removed duplicate member initializer.

[Jeremy Sowden]

Initialization of a netlink_ctx included two initializers for .nft.
Removed one of them.

Fixes: 2dc07bcd7eaa ("src: pass struct nft_ctx through struct netlink_ctx")
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 src/rule.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index b957b4571249..0ebe91e79a03 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -240,7 +240,6 @@ int cache_update(struct nft_ctx *nft, unsigned int flags, struct list_head *msgs
 		.list		= LIST_HEAD_INIT(ctx.list),
 		.nft		= nft,
 		.msgs		= msgs,
-		.nft		= nft,
 	};
 	struct nft_cache *cache = &nft->cache;
 	uint32_t genid, genid_stop;
-- 
2.20.1

Re: [PATCH nft v2 2/2] rule: removed duplicate member initializer.

[Phil Sutter]

On Fri, Jul 19, 2019 at 12:10:10PM +0100, Jeremy Sowden wrote:
> Initialization of a netlink_ctx included two initializers for .nft.
> Removed one of them.

Oh, good catch! The fact that I stared ad this code ten minutes
ago and didn't see it speaks for itself. :)

> Fixes: 2dc07bcd7eaa ("src: pass struct nft_ctx through struct netlink_ctx")
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>

Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH nft v2 0/2] netlink_ctx initialization fixes.

[Pablo Neira Ayuso]

On Fri, Jul 19, 2019 at 12:10:08PM +0100, Jeremy Sowden wrote:
> A couple of patches to tidy up initialization of a pair of netlink_ctx
> variables.

Applied, thanks.