Rework default route handling. #544
Conversation
There was a problem hiding this comment.
There is also this reference to replacedefaultroute in pppd/ipcp.c in ipcp_clear_addrs:
2055 /* If replacedefaultroute, sifdefaultroute will be called soon
2056 * with replacedefaultroute set and that will overwrite the current
2057 * default route. This is the case only when doing demand, otherwise
2058 * during demand, this cifdefaultroute would restore the old default
2059 * route which is not what we want in this case. In the non-demand
2060 * case, we'll delete the default route and restore the old if there
2061 * is one saved by an sifdefaultroute with replacedefaultroute.
2062 */
I reckon this means further code adjustments may be required around there.
|
@paulusmack, @sthibaul: Can you look this @jkroonza PR which solves one ticket :) |
jkroonza
force-pushed
the
pppd-default-rework
branch
from
3fc321e to
1f88d4b
Compare
|
@paulusmack, @sthibaul: Can you look this @jkroonza PR which solves one ticket :) |
|
@paulusmack I would appreciate some feedback on this, especially in the light off the concept #546. Given that I'm wondering if the newly added netlink route/add options should take a prefix in addition. sockaddr_t + socklet_t + some unsigned option. Then a pair of sifroute functions for adding arbitrary routes which can be called from the radius module. Currently we handle static routes external to pppd in ip-up and ipv6 up, which is fine, and that can stay there, but we're going to need plumbing for IPv6 PDs anyway, so might just as well handle Framed-Route and IPv6-Framed-Route radius options too. |
|
Is 0 really a good choice for the default metric? Shouldn't the metric be somewhat reflective of the bandwidth of the network device (which is often lower for PPP links than other types of network)? |
|
That's the current default. And how do you know what the bandwidth on the client side of a ppp link will be? |
| { "noreplacedefaultroute", o_bool, | ||
| &ipcp_wantoptions[0].replace_default_route, | ||
| "Do not replace default route", 0 }, | ||
| #endif |
There was a problem hiding this comment.
In general I prefer not to break existing working setups by removing options, if it is reasonable for the option to become a no-op (or at worst emit a warning). Could these be made no-ops?
There was a problem hiding this comment.
Not sure why I can't reply by the 2^31 issue, the kernel type is unsigned, but sure, as long as we can restrict to positive integer that's fine by me. If you need metrics >=2^31 you can do the relevant patching.
Re no-op, will handle. Normally I'd argue for "let it break so that someone knows something need to change", but in this case since it might well relate to something that the admin needs to gain access to the system ... I'm happy to concede.
There was a problem hiding this comment.
@paulusmack please check if you're happy with the change.
I took the opportunity to move options out of pppd/options.c and into the NCPs where they belong at the same time (at least those that I touched). I trust this will be in-order as it honestly makes more sense to have IPCP options reside in ipcp.c and IPv6CP options in ipv6cp.c.
I did (from the initial PR) include some whitespace only changes into the PR. If that's an issue I can rework to exclude that. Didn't make the same mistake on other files (vim automatically cleans up trailing whitespace and a few others on my system).
I think being able to have multiple default routes in the system makes this easier, actually, in that we can add a default route and remove it (as long as we are sure to remove the one we added) and leave the system as it was before. So that whole comment can probably be deleted. |
1. replacedefaultroute option is no longer functional.
2. default route will now always be appended at defaultroute-metric (both
IPv4 and IPv6) if defaultroute{.6} is given.
Closes: ppp-project#473
Signed-off-by: Jaco Kroon <[email protected]>
jkroonza
force-pushed
the
pppd-default-rework
branch
from
1f88d4b to
99151ea
Compare
Closes: #473