[RFC PATCH 1/5] Add support for AF_BRIDGE family in link cache

[Roopa Prabhu]

On 11/5/12 5:56 AM, Thomas Graf wrote:
> On 11/05/12 at 05:37am, roopa at cumulusnetworks.com wrote:
>> From: roopa<roopa at cumulusnetworks.com>
>>
>> This patch adds support for AF_BRIDGE family in link cache
>> and adds link family attribute to link object oo_id_attrs.
>> With this the key for link object lookups will be ifindex and family.
>>
>> This allows for base rtnl link objects to co-exist with the AF_BRIDGE
>> link objects in the same cache.
>
> I like the idea of reusing the link cache to cover bridge objects.
> Especially because most of the code is essentially the same.
>
> What we can't do is include the bridging objects by default.
> Especially they look like incomplete duplicates to users that
> don't care about AF_BRIDGE object.
>
> I suggest to require enabling this feature using a flag of some
> sort. Eventually the flag that I propose further down in the email.

>
>
>> Note: I am considering the option of fixing the kernel link cache AF_UNSPEC get
>> at some point if possible. Also the link cache get api's today only look for
>> ifindex match and not family. These will need to be fixed or new api's introduced.
>> I can submit patches to fix this if we choose to take this path.
>>
>> -restart:
>>   	/* Mark all objects so we can see if some of them are obsolete */
>>   	nl_cache_mark_all(cache);
>>
>> -	err = nl_cache_request_full_dump(sk, cache);
>> -	if (err<  0)
>> -		goto errout;
>> -
>> -	err = __cache_pickup(sk, cache,&p);
>> -	if (err == -NLE_DUMP_INTR)
>> -		goto restart;
>> -	else if (err<  0)
>> -		goto errout;
>> +        for (grp = cache->c_ops->co_groups; grp->ag_group; grp++) {
>> +		nl_cache_set_arg1(cache, grp->ag_family);
>> +restart:
>> +		err = nl_cache_request_full_dump(sk, cache);
>> +		if (err<  0)
>> +			goto errout;
>> +
>> +		err = __cache_pickup(sk, cache,&p);
>> +		if (err == -NLE_DUMP_INTR)
>> +			goto restart;
>> +		else if (err<  0)
>> +			goto errout;
>
> We need to add a flag to trigger this behavior. It makes sense for the
> link cache but it does not make sense for the address cache. RTM_GETADDR
> with family == AF_UNSPEC triggers a foreach(family) do dump action
> which is definitely more desirable than trying to emulate that from with
> libnl.
>
> So please add 'co_flags' to struct nl_cache_ops with a new flag
> NL_CACHE_AF_ITER to trigger the above code.

yes, agreed. This sounds good.

>
>> +/**
>> + * (Re)fill a cache with the contents in the kernel for all
>> + * families the cache is associated with
>> + * @arg sk		Netlink socket.
>> + * @arg cache		cache to update
>> + *
>> + * Clears the specified cache and fills it with the current state in
>> + * the kernel.
>> + *
>> + * @return 0 or a negative error code.
>> + */
>> +int nl_cache_refill_assoc_families(struct nl_sock *sk, struct nl_cache *cache)
>
> I believe we can get rid of this if we have the above flag, do you
> agree?
>
Yup definitely. I would prefer making it conditional too. And flags is 
good. This patch series was just to show what all needs to change.

> Alternatively we could add nl_cache_set_flags() and
> nl_cache_unset_flags() to allow modifying the iterate groups feature.
>
> Example:
> 	nl_cache_set_flags(link_cache, NL_CACHE_AF_ITER);
> 	nl_cache_refill(sk, link_cache);

yes this is good too. This will give the user full control over the flag 
like you suggested.
But, if the user really is interested in AF_BRIDGE objects, and if the 
flag is not on by default for the link cache, the refill in 
nl_cache_mngr_add will not contain AF_BRIDGE objects and user will need 
to call an additional refill with the flag to get the AF_BRIDGE objects.
Would have been nice if this additional call could be avoided. Let me 
know if you have any suggestions here. I will see if there is a way to 
avoid this too.


Thanks for the comments thomas! I will respin this patch and submit.