Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four <mr.dash.four@googlemail.com>]

>> +	IPSET_DIM_IFACE = 7, 
>>  };
> 
> It's not a dimension, please give it some other name,
> like IPSET_IFACE_INOUT_FLAG.
A bit like you then. Unless you think that including IPSET_DUMP_LAST in the "features" enum list describes an actual feature. Point taken though, will change it to something more appropriate.

>> +struct ip_set_req_get_features {
>> +	unsigned int op;
>> +	unsigned int version;
>> +	__u8 features;
>> +	union ip_set_name_index set;
>> +};
>> +
> 
> In spite of stuffing a u8 sized value into features, please make it an
> unsigned int. We don't spare anything with __u8 here.
We do. In ip_set.h, "features" is defines as u8 (a byte), so I am not "stuffing" anything up - I just use the same type of variable. Besides, you are well-aware that I store this in info->flags, which is also __u8 (a byte). If I use unsigned int (a word), then I have to do extra jiggling to get rid of the bits I don't need. 

Not to mention, that "features" as defined above currently contains a single bit (the IFACE feature), so to me, given the potential hazards I just described, it wasn't warranting unsigned int, unless you plan on expanding the "features" variable and use the same struct above.


> 
>>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
>>  struct ip_set_req_version {
>>  	unsigned int op;
>> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> index d7eaf10..1d8d754 100644
>> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> @@ -348,6 +348,10 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
>>  	ipset_adtfn adtfn = set->variant->adt[adt];
>>  	struct ipmac data;
>>  
>> +	/* in|out not allowed in this set type, only src|dst */
>> +	if (opt->flags & IPSET_DIM_IFACE_INOUT)
>> +		return -EINVAL;
>> +
> 
> Drop this unnecessary checking: ipset doesn't care about iptables syntax.
This was a left-over from v1 of the patch and served to restrict matching when 'in' or 'out' is used in a list:set, or, when 'in' or 'out' was entered in 2+ dimensional sets as I didn't have the means to see what type of set I am dealing with in iptables. 

Now that the latter is being resolved via the get_set_byname_with_features function, I would still like to keep this and relax the restriction for 'in' and 'out' on the list:set as I am yet to get a reasonable and adequate explanation as to why that should not be allowed (the use of 'in' and 'out' in list:set)?

It gives me, as a user, a choice when I have a list:set with many different set types in it to apply that restriction (checking on in/out interfaces only) and use it when needed, instead of being restricted to just src/dst and have ipset reiterate through all members of each set - unnecessarily, when in practice I would have liked a match on the iface-type sets only. 

It also forces me to use two separate list:sets with duplicate members: one for the occasion when I want to use src/dst and match against all members of the set, and another, duplicate copy, which contains just the iface sets so that I could get a match just on them. Wasteful!

If I have a choice to use in/out, in addition to src/dst, I can deploy a single set (saves on memory and scanning is faster) and use this - it is much more elegant. 

The use of in/out is not in any way restrictive as whoever deploys it, clearly has a choice (I'd argue that the use of just src/dst is more restrictive as it doesn't give me that choice). That use of in/out can't be "confusing" either as the definition of it and what it matches against is pretty clear for everyone.

Just a couple of days ago you wanted to introduce in/out direction parameters everywhere - in every set and in every direction parameter. I would argue that was more confusing and hardly appropriate, if anything.

>> +static ip_set_id_t
>> +find_set_features(const char *name, __u8 *features)
>> +{
>> +	ip_set_id_t i, index = IPSET_INVALID_ID;
>> +	const struct ip_set *set;
>> +
>> +	for (i = 0; index == IPSET_INVALID_ID && i < ip_set_max; i++) {
>> +		set = ip_set_list[i];
>> +		if (set != NULL && STREQ(set->name, name)) {
>> +			index = i;
>> +			/* In theory, we could return the entire set of features
>> +			 * for a given set, though, for now, we only return 
>> +			 * the IPSET_TYPE_IFACE bit
>> +			 */
>> +			*features = set->type->features & IPSET_TYPE_IFACE;
> 
> I believe it's better to return the entire feature, not just 
> IPSET_TYPE_IFACE.
Agreed, though with the added caveat about versioning and the use of new parse/print etc functions - see my comments on this in the other email (about the iptables patch).

>> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
>> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>>  				 &data.port, &data.proto))
>>  		return -EINVAL;
> 
> Drop this and all other checkings below:
See above - I'd like to keep it and relax the restrictions placed upon the list:set.