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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Aug 14, 2017 at 01:36:44PM +0200, Phil Sutter wrote:
> 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.

May be your gcc version.

Anyway, the warning is clearly right.

> [...]
> > 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().

Then, you pass the callback that you need to nft_mnl_talk() as parameter.

> 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.

Why don't you just add context as cb_data so you know you have to do
the netlink_echo_callback() handling?

There must be a better way to do this...

> > Please, follow up with a patchset to address this.
> 
> Will do, thanks for the feedback!

Thanks!