Re: [nft PATCH v4 0/3] Implement --echo option

[Phil Sutter <phil@nwl.cc>]

On Mon, Aug 14, 2017 at 11:26:51AM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 09, 2017 at 01:16:40PM +0200, Phil Sutter wrote:
> > Long description of what it is and how it works in patch 3. Patch 1 is a
> > dependency to patch 2, Patch 3 adds a simple test suite which was
> > helpful during development.
> 
> Applied, but please follow up asap to address a couple of issues:
> 
> mnl.c: In function ‘nft_mnl_talk_cb’:
> mnl.c:82:5: warning: ‘rc’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
>   if (rc)
>      ^

Hmm, that's weird - the warning is correct, but gcc on my system doesn't
complain. Even after explicitly setting -Wmaybe-uninitialized.

[...]
> Apart from this, this struct nft_mnl_talk_cb_data looks... a bit
> convoluted ;)
> 
>         static int
>         nft_mnl_talk(struct mnl_socket *nf_sock, const void *data, unsigned int len,
>                      int (*cb)(const struct nlmsghdr *nlh, void *data), void *cb_data)
>         {
>                 uint32_t portid = mnl_socket_get_portid(nf_sock);
>                 struct nft_mnl_talk_cb_data tcb_data = {
>                         .cb = cb,
>                         .data = cb_data,
>                 };
> 
>         #ifdef DEBUG
>                 if (debug_level & DEBUG_MNL)
>                         mnl_nlmsg_fprintf(stdout, data, len, sizeof(struct nfgenmsg));
>         #endif
> 
>                 if (mnl_socket_sendto(nf_sock, data, len) < 0)
>                         return -1;
> 
>                 return nft_mnl_recv(nf_sock, seq, portid, &nft_mnl_talk_cb, &tcb_data);
>         }
> 
> Why don't you simply pass the callback that you need to nft_mnl_recv()
> instead of adding this extra unnecesary abstraction...

It is not unnecessary: There are several callers passing a callback to
nft_mnl_talk(). I didn't want to mess with all of them but still insert
netlink_echo_callback(). Hence I introduced nft_mnl_talk_cb() which
takes care of the callback passed by callers and ultimately calls the
echo callback.

> Please, follow up with a patchset to address this.

Will do, thanks for the feedback!

Cheers, Phil