search

thom311 / libnl

Netlink Library Suite
GNU Lesser General Public License v2.1
423 stars 311 forks source link

support for nexthops #331

Closed zstas closed 2 years ago

zstas commented 2 years ago

Hello,

There's support for the next-hops in the kernel for a while. I've implemented the support for libnl and want to send the PR, but I have some questions before:

  1. right now there's a next-hop entity for a rtnl_route. it's called rtnl_nexthop. IMO it's better to give a new name (for instance rtnl_route_nh) and for generic next-hops use rtnl_nexthop. Another option is to give a different name for new next-hops (like rtnl_nexthop2). what do you think is better? I think the 1st approach is good but it'd break API.
  2. is full support for a new type requested? I've implemented only the parsing/dumping, but not adding/removing next-hops. shall I do that as well?
thom311 commented 2 years ago

IMO it's better to give a new name (for instance rtnl_route_nh) and for generic next-hops use rtnl_nexthop.

The existing API must not change, so this is not possible.

This leaves only to find another name... "struct rtnl_nh" might work.

is full support for a new type requested?

If the new code is self-contained and somewhat useful without the additional add/remove API, then it's preferable to merge it. That makes it possible to improve it in the future.

Patch welcome :) Thanks.

zstas commented 2 years ago

got it! thanks for the quick response

thom311 commented 2 years ago

naming is hard, but important. I don't have a good suggestion. I don't like rtnl_nexthop2 very much. I tend to rtnl_nh.

Suggestion welcome.

In any case, it might be best to agree on the naming first, before doing a larger patch/renaming...

zstas commented 2 years ago

yes, forgot to mention that I like your suggestion (almost anything would be better than rtnl_nexthop2) and will change the name (also rename headers and sources) before sending the patch for a review.