Re: json_cmd_assoc and cmd

[Jeremy Sowden <jeremy@azazel.net>]

[-- 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 --]