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

[Thomas Haller]

On Mon, 2017-08-14 at 08:07 -0600, David Ahern wrote:
> On 8/14/17 3:24 AM, Thomas Haller wrote:
> > > --- a/lib/route/route_obj.c
> > > +++ b/lib/route/route_obj.c

> > > @@ -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.

oo_id_attrs is internal, but the effects are very much visible to the
user, as it determines how nl_object_identical() behaves.


> > 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.

I don't think that is correct. 

For oo_compare() we pass @attrs (== @req_attrs == @oo_id_attrs).
Implementations like route_compare() use 
  #define ROUTE_DIFF(ATTR, EXPR) ATTR_DIFF(attrs, ROUTE_ATTR_##ATTR, a, b, EXPR)
which only compares two properties if they are mentioned by
attrs/req_attrs.

Removing ROUTE_ATTR_PRIO from @oo_id_attrs means, that we no longer
consider the priority when comparing two routes. That is a change in
behavior for AF_INET/AF_INET6 as now two routes compare identical that
only differ by priority.

(one might argue, that nl_object_identical() is anyway broken for
routes. But this change seems going to the wrong direction).



I see two options:

 - for MPLS routes, pretend ROUTE_ATTR_PRIO is always set to a fake 
   value of zero.
 - implement oo_id_attrs_get() for routes instead.


best,
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20170817/5ac41b1a/attachment.sig>