thank you once again for review, few comments bellow, patches can be found: [PATCH 0/1] add hash:ip,mark data type to ipset - http://marc.info/?l=netfilter-devel&m=138728893302103&w=2 [PATCH 1/1] add markmask for hash:ip,mark data type - http://marc.info/?l=netfilter-devel&m=138728893302104&w=2 On 3 December 2013 19:15, Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxxxxxx> wrote: > > > The intended use is similar to the ip:port type, but for protocols which don't use > > a predictable port number. Instead of port number it matches a firewall mark > > determined by a layer 7 filtering program like opendpi, which will be called > > by an earlier iptables rule. > > OK, I see. > > > I wasn't sure if it's best to set mark on individual entry within a set or for > > whole set with or without mask. > > What are your views on it? For now it's set on individual entries without mask. > > I think mark value and mask pair per element would be more useful and > flexible than a mark value alone. The "mark" match uses value[/mask], so > the same should be used here too. > mark[/mask] has limitation that packet marks don't have precedence like ip netmask ( with most specific networks first ), so overlapping marks can't exist in the same set. Constantly parsing them, might introduce performance issues. Therefore, I implemented mark mask per set ( the same way as netmask is implemented in hash:ip ). A simple use case: user is interested in the first 11 bits of mark. User specifies markmask when creating set: ipset create mytest hash:ip,mark markmask 0xffe00000 Thereafter, each time an entry is added to the set, it will perform bitwise AND using markmask value before mark is stored: ipset add mytest 1.1.1.1,0xf0345678 which results to 1.1.1.1,0xf0200000 being stored. However, if user is interested in all 32 bits, user should either specify markmask as 0xffffffff or leave it blank, as markmask defaults to 0xffffffff. I believe this is more clear and useful than markmask per entry. > There's one thing I spotted in your patch, in both *_uadt function: > > + e.mark = htonl(nla_get_u32(tb[IPSET_ATTR_MARK])); > > The value is passed in network order back and forth, so it should be at > both places: > > + e.mark = ntohl(nla_get_u32(tb[IPSET_ATTR_MARK])); > Fixed -- Vytas Dauksa Junior Developer vytas.dauksa@xxxxxxxxxxxxxx Smoothwall Ltd Phone: +44 (0) 8701 999500 www.smoothwall.net -- 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
