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!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
