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