On Tue, Feb 14, 2012 at 04:56:01PM +0100, Jan Engelhardt wrote:
>
> On Tuesday 2012-02-14 11:47, Pablo Neira Ayuso wrote:
> >> include/linux/netfilter/nfnetlink_xtables.h | 20 ++++
> >> net/netfilter/Kconfig | 8 ++
> >> net/netfilter/Makefile | 1 +
> >> net/netfilter/xt2_nfnetlink.c | 153 +++++++++++++++++++++++++++
> >
> >I prefer if you call this nfnetlink_xtables.c following the same
> >naming convention.
>
> (I dropped the '2' meanwhile) Hm I chose so as I planned to have
> at least another file, xt_compat.c. In that regard, it's more like
> what ipset/ has (also not nfnetlink_ipset). Should I pick a
> different header name then?
As said, I'd prefer to stick to nfnetlink_* naming. But I don't mind.
> >> +enum nfxt_msg_type {
> >> + NFXTM_IDENTIFY = 1,
> >
> >I'd suggest: NFXT_MSG_NEW, NFXT_MSG_GET,...and so on.
>
> The identifiers are already long enough IMO. Hence the choice
> to use NFXTM_ (msg), NFXTA_ (attr) and NFXTE_ (error codes).
OK
> >> +static struct nlmsghdr *
> >> +xtnetlink_fill(struct sk_buff *skb, const struct xtnetlink_pktref *old,
> >> + unsigned int flags)
> >> +{ [...]
> >> + if (nlmsg == NULL) {
> >> + nlmsg_cancel(skb, nlmsg);
> >> + return ERR_PTR(-ENOBUFS);
> >
> >my experience is that it's better to use goto to handle errors.
>
> There is just one error case here in this function,
> so goto was nowhere to be desired :)
OK
> >> +static int
> >> +xtnetlink_identify(struct sock *xtnl, struct sk_buff *iskb,
> >> + const struct nlmsghdr *imsg, const struct nlattr *const *ad)
> >> +{
> >> + return netlink_dump_start(xtnl, iskb, imsg, xtnetlink_identify2,
> >> + NULL, 0);
> >
> >you have to check for NLM_F_DUMP. Otherwise, you are requesting one
> >single table. Please, see ctnetlink or nfnetlink_acct for reference.
>
> identify returns auxiliary information about the implementation.
> For tables there is(will be) NFXTM_TABLE_DUMP.
Better use standard netlink flags like NLM_F_DUMP, NLM_F_REPLACE and
so on.
--
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
