Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

route: expose nexthop id attribute #386

Closed
wants to merge 1 commit into from

Conversation

KanjiMonster
Copy link
Contributor

Routes may reference a nexthop (group) via the new nexthop API by its ID, so add accessors for setting and getting it.

Referencing a nexthop is mutually exclusive to specifiying nexthops in the route, so make sure we do not do that when creating netlink messages (which may exist both, since netlink messages from the kernel contain both unless 'nexthop_compat_mode' is disabled).

$ ip -6 r
2001:db8:3::/64 nhid 20 metric 1024 pref medium
	nexthop via 2001:db8:1::2 dev v0 weight 1
	nexthop via 2001:db8:2::2 dev v1 weight 1

Before:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

After:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast nhid 20 via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

@KanjiMonster
Copy link
Contributor Author

I didn't try to link the nexthop and route caches for now, to keep the changes simple.

Copy link
Owner

@thom311 thom311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good patch! Minor commonts.

@@ -284,6 +289,9 @@ static void route_dump_details(struct nl_object *a, struct nl_dump_params *p)
r->rt_ttl_propagate ? "enabled" : "disabled");
}

if (r->ce_mask & ROUTE_ATTR_NHID && r->rt_nhid != 0)
nl_dump(p, "nhid %d ", r->rt_nhid);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%u

that is appropriate for the type of r->rt_nhid, but it's also what iproute2 does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, and yes, %u sounds more appropriate.

lib/route/route_obj.c Outdated Show resolved Hide resolved
lib/route/route_obj.c Show resolved Hide resolved
lib/route/route_obj.c Outdated Show resolved Hide resolved
@thom311
Copy link
Owner

thom311 commented May 17, 2024

There is still a if (r->ce_mask & ROUTE_ATTR_NHID && r->rt_nhid != 0). It should only check the flags (granted, the conditions are true always at the same time).

Rest lgtm.

Routes may reference a nexthop (group) via the new nexthop API by its
ID, so add accessors for setting and getting it.

Referencing a nexthop is mutually exclusive to specifiying nexthops in
the route, so make sure we do not do that when creating netlink
messages (which may exist both, since netlink messages from the kernel
contain both unless 'nexthop_compat_mode' is disabled).

$ ip -6 r
2001:db8:3::/64 nhid 20 metric 1024 pref medium
	nexthop via 2001:db8:1::2 dev v0 weight 1
	nexthop via 2001:db8:2::2 dev v1 weight 1

Before:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

After:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast nhid 20 via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

Signed-off-by: Jonas Gorski <[email protected]>
@KanjiMonster
Copy link
Contributor Author

There is still a if (r->ce_mask & ROUTE_ATTR_NHID && r->rt_nhid != 0). It should only check the flags (granted, the conditions are true always at the same time).

Rest lgtm.

Oops, fixed.

thom311 pushed a commit that referenced this pull request May 17, 2024
Routes may reference a nexthop (group) via the new nexthop API by its
ID, so add accessors for setting and getting it.

Referencing a nexthop is mutually exclusive to specifiying nexthops in
the route, so make sure we do not do that when creating netlink
messages (which may exist both, since netlink messages from the kernel
contain both unless 'nexthop_compat_mode' is disabled).

$ ip -6 r
2001:db8:3::/64 nhid 20 metric 1024 pref medium
	nexthop via 2001:db8:1::2 dev v0 weight 1
	nexthop via 2001:db8:2::2 dev v1 weight 1

Before:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

After:

$ nl-route-list
inet6 2001:db8:3::/64 table main type unicast nhid 20 via 2001:db8:1::2 dev v0 via 2001:db8:2::2 dev v1

Signed-off-by: Jonas Gorski <[email protected]>

#386
@thom311
Copy link
Owner

thom311 commented May 17, 2024

merged as 3e08063

Thank you!!

@thom311 thom311 closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants