Re: [PATCH 1/2] netfilter: ipset: Add hash:net,net module to kernel.

[Oliver <oliver@8.c.9.b.0.7.4.0.1.0.0.2.ip6.arpa>]

On Thursday 19 September 2013 11:30:50 Jozsef Kadlecsik wrote:
> On Thu, 19 Sep 2013, Oliver wrote:
> > > On Tue, 17 Sep 2013, Oliver wrote:
> > <snip>
> > 
> > > > @@ -461,6 +462,9 @@ mtype_expire(struct ip_set *set, struct htype *h,
> > > > u8
> > > > nets_length, size_t dsize)>
> > > > 
> > > >  	struct mtype_elem *data;
> > > >  	u32 i;
> > > >  	int j;
> > > > 
> > > > +#if IPSET_NET_COUNT > 1
> > > > +	u8 k;
> > > > +#endif
> > > 
> > > Please get rid of all these #if .. [#else ...] #endif constructions,
> > > except in mtype_test_cidrs. The compiler optimizes away the for loop
> > > when
> > > IPSET_NET_COUNT == 1.
> > 
> > Yep, there's a couple of other places where I don't currently see a way
> > around it, but anywhere that it doesn't pose a logical problem to
> > existing types, I've removed it.
> 
> Where do you think it's required, except in mtype_test_cidrs?

Well actually, I've come to the realisation that it's actually required pretty 
much everywhere that I placed it because of the simple reason that in the 
existing hash types, data->cidr is not a u8 array, but it is in hash:net,net 
obviously.

So now either we can convert all the existing hash types so that the cidr 
member of their struct is a single-element array, or we keep the preprocessor 
conditions, your choice.

> 
> > > > +	if (unlikely(!tb[IPSET_ATTR_IP] || !tb[IPSET_ATTR_IP2] ||
> > > > +		     !ip_set_optattr_netorder(tb, IPSET_ATTR_TIMEOUT) ||
> > > > +		     !ip_set_optattr_netorder(tb, IPSET_ATTR_CADT_FLAGS) ||
> > > > +		     !ip_set_optattr_netorder(tb, IPSET_ATTR_PACKETS) ||
> > > > +		     !ip_set_optattr_netorder(tb, IPSET_ATTR_BYTES)))
> > > > +		return -IPSET_ERR_PROTOCOL;
> > > > +	if (unlikely(tb[IPSET_ATTR_IP_TO]))
> > > > +		return -IPSET_ERR_HASH_RANGE_UNSUPPORTED;
> > > 
> > > Check tb[IPSET_ATTR_IP2_TO] in the condition above too.
> > 
> > This is IPv6, I thought ranges weren't even legitimate here, also, if
> > this is wrong, hash:net must be too... Or does IP2_TO not mean what I
> > think it means?
> 
> IPSET_ATTR_IP2_TO carries the endpoint of the range of the second IP type
> of element and ranges are not allowed in either IP type of element part
> for IPv6. It is not valid for hash:net at all, but messages for
> hash:net,net types may carry it. ipset itself does not allow the syntax,
> but the library is free to use for anyone and therefore the kernel must
> handle the case.

OK, I see what you mean now, for some reason I didn't see the IPSET_ATTR_IP_TO 
check, obviously TO2 is needed, I will fix that before sending v2.

Thanks,
Oliver.