Re: [PATCH v3 1/3] iproute2: add support for setting device groups

[Patrick McHardy <kaber@trash.net>]

On 02.02.2011 10:13, Vlad Dogaru wrote:
> On Wed, Feb 02, 2011 at 09:56:28AM +0100, Patrick McHardy wrote:
>> On 26.01.2011 17:41, Vlad Dogaru wrote:
>>> Use the group keyword to specify what group the device should belong to.
>>> Since the kernel uses numbers internally, mapping of group names to
>>> numbers is defined in /etc/iproute2/group_map. Example usage:
>>>
>>>   ip link set dev eth0 group default
>>>
>>> @@ -297,6 +299,13 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
>>>  			if (get_integer(&mtu, *argv, 0))
>>>  				invarg("Invalid \"mtu\" value\n", *argv);
>>>  			addattr_l(&req->n, sizeof(*req), IFLA_MTU, &mtu, 4);
>>> +		} else if (strcmp(*argv, "group") == 0) {
>>> +			NEXT_ARG();
>>> +			if (group != -1)
>>> +				duparg("group", *argv);
>>> +			if (lookup_map_id(*argv, &group, GROUP_MAP))
>>> +				invarg("Invalid \"group\" value\n", *argv);
>>> +			addattr_l(&req->n, sizeof(*req), IFLA_GROUP, &group, 4);
>>
>> I think it would be preferrable to use a function similar to
>> rt_realm_n2a() that can also handle plain numerical values.
> 
> The a2n() functions are rather complex for this case: they employ
> caching and store a table. I suppose this is because multiple calls to
> them are possible in a single run and the correspondence has to be made
> in both ways (a2n and n2a).
> 
> A network group is only converted to a number at most once for each ip
> process spawned, so storing a table is not really helpful. What could,
> however, help is using get_integer before lookup_map_id. Only if
> get_integer fails would we lookup the symbolic group name.

Actually that's not entirely correct, the caches are (also) maintained
to speed up batch mode, in which case there could also be multiple name
to group mappings.