[PATCH libnl 05/11] route: Add support for MPLS address family

[David Ahern]

On 8/14/17 3:24 AM, Thomas Haller wrote:
>> --- a/lib/route/route_obj.c
>> +++ b/lib/route/route_obj.c
>> @@ -646,8 +646,11 @@ uint8_t rtnl_route_get_protocol(struct
>> rtnl_route *route)
>>  
>>  void rtnl_route_set_priority(struct
>> rtnl_route *route, uint32_t prio)
>>  {
>> -	route->rt_prio = prio;
>> -	route->ce_mask |= ROUTE_ATTR_PRIO;
>> +	/* MPLS address family does not allow RTA_PRIORITY to be set
>> */
>> +	if (route->rt_family != AF_MPLS) {
>> +		route->rt_prio = prio;
>> +		route->ce_mask |= ROUTE_ATTR_PRIO;
>> +	}
> 
> it's a bit strange that the setter does nothing for MPLS family.
> 
> There are countless ways how to create invalid/incomplete objects.
> 
> If the user first calls rtnl_route_set_priority() before set_family(),
> he still could set ROUTE_ATTR_PRIO.
> 
> Anyway, I don't mind this too much. Fine as is.

I'll take another look. Might be leftover from attempts to figure out
why route adds were failing EINVAL. The problem was the priority
attribute was always getting set for routes.

The API should set it if the function is called in which the kernel add
can fail. Without the extended error reporting in libnl,

> 
> 
>> @@ -1310,8 +1415,7 @@ struct nl_object_ops route_obj_ops = {
>>  	.oo_update		= route_update,
>>  	.oo_attrs2str		= route_attrs2str,
>>  	.oo_id_attrs		= (ROUTE_ATTR_FAMILY |
>> ROUTE_ATTR_TOS |
>> -				   ROUTE_ATTR_TABLE | ROUTE_ATTR_DST
>> |
>> -				   ROUTE_ATTR_PRIO),
>> +				   ROUTE_ATTR_TABLE |
>> ROUTE_ATTR_DST),
> 
> as you surely are aware, these ID attributes for routes are not how
> kernel handles routes (see your other patchset "route cache: support
> for non-exclusive and append routes").

I am not following you. oo_id_attrs is internal to libnl for comparing
objects. The above change is only dropping ROUTE_ATTR_PRIO from that list.

> 
> It seems dangerous to change the ID attributes of all routes (at this
> point, without careful consideration).
> 
> Maybe we should instead pretend that for MPLS routes, the priority is
> always implicitly set to zero?
> Or maybe better: implement oo_id_attrs_get() instead, which continues
> to give ROUTE_ATTR_PRIO for regular routes, but not for MPLS routes.

>From my understanding of nl_object_identical, the oo_id_attrs_get is the
quick review of whether 2 objects might be identical. Object 'a' should
have those properties and object 'b' should have those properties. If
so, proceed to the oo_compare function which looks at all attributes for
'a' and 'b'.

So dropping ROUTE_ATTR_PRIO only means rt_prio does not have to be set
in 'a' and 'b' to drop to route_compare, though when route_compare does
run it compares that attribute in 'a' and 'b'. I need to confirm (it's
been 2 months) but I recall testing identical routes with different
priorities to make sure it work as expected and caches stayed consistent.