Restore adding a default route with different metric

[sthibaul]

For instance, when using a ppp link as backup link, one would want to add a default route on the ppp link, in addition to the existing default route.

d0ccb87 ("pppd: Add replacedefaultroute option (#200)") however broke this case: sifdefaultroute was not passing the metric to defaultroute_exists any more. This commit restores this case.

Fixes #357

[jkroonza]

The relevant documentation of the defaultroute options:

   defaultroute
          Add  a  default  route to the system routing tables, using the peer as the gateway,
          when IPCP negotiation is successfully completed.  This entry is  removed  when  the
          PPP  connection  is broken.  This option is privileged if the nodefaultroute option
          has been specified.

   defaultroute-metric
          Define the metric of the defaultroute and only add it if there is no other  default
          route  with the same metric.  With the default value of -1, the route is only added
          if there is no default route at all.

   replacedefaultroute
          This  option  is a flag to the defaultroute option. If defaultroute is set and this
          flag is also set, pppd replaces an existing default  route  with  the  new  default
          route.  This option is privileged.

(An appenddefaultroute option would also be convenient but let's not over-complicate things).

I think we should ignore the IPv6 situation, defaultroute6 technically already violates the standards as I understand them, but it's been stated that providers are messing around, so let's just leave that as is. Also, this could technically be handled more flexibly from ipv6-up. Same for ip-up actually come to think about it.

So there are a few valid combinations of options:

1. defaultroute
2. defaultroute+defaultroute-metric
3. defaultroute+replacedefaultroute
4. defaultroute+defaultroute-metric+replacedefaultroute

So I believe for these combinations the behaviour should be as follows (Ignoring 3 which is problematic):

1. If any default route exist, don't add one, if none exist, add with metric 0.
2. If a default route with the same metric exist, don't add one, else add with relevant metric.
3. If a default route with the same metric exist, remove it (restore on ipv4 down) and re-add via ppp.

A further problem with replacedefaultroute is that it's possible in linux to have multiple exact same routes at the same metric, so technically even 4 is a problem above:

plastiekpoot [09:01:41] ~ # ip ro sh
default via 192.168.42.1 dev wlp2s0 proto dhcp src 192.168.42.21 metric 2002 
...
plastiekpoot [09:02:05] ~ # ip route append default via 192.168.42.1 dev wlp2s0 proto static src 192.168.42.21 metric 2002
plastiekpoot [09:02:10] ~ # ip ro sh
default via 192.168.42.1 dev wlp2s0 proto dhcp src 192.168.42.21 metric 2002 
default via 192.168.42.1 dev wlp2s0 proto static src 192.168.42.21 metric 2002 
...

So with replacedefaultroute - should we remove all matching default routes or only the first matching one?

Which leads into my logic issue with 3 above. If there is only one other default route it's all good and well, we remove it and re-add later, but what if there are multiple such routes?

I guess the replacedefaultroute option is problematic regardless. We should only ever add routes and remove our own, the system administrator should manage priority via metrics.

So really the question is if a matching route with the same metric exist, do we append or not? Does yet another separate option for that even make sense or should we simply append our route regardless - as such - nuke the replacedefaultroute option entirely, and append another default route at the specified metric and move on, thus reduce the defaultroute options to "defaultroute" and "defaultroute-metric" where if defaultroute is given we ALWAYS APPEND a default route at the specified defaultroute-metric (which then defaults to 0).

If the system administrator wants more control than that - have them rely on ip-(pre-)up or net-pre-up and let them deal with it there?

Whilst the existing logic is definitely broken, I'm not convinced that the PR brings things closer to the documented behaviour either, nor am I sure the documented behaviour is well defined.

[Neustradamus]

@sthibaul: Good, @apompee has confirmed on the ticket that it works with your PR :)

[apompee]

Since the defaultroute-metric only works if defaultroute option is set. Wouldn't it also make sense to modify the defaultroute option to accept a metric parameter which defaults to 0 if not set? And then the defaultroute-metric option could be removed.

[sthibaul]

Whilst the existing logic is definitely broken, I'm not convinced that the PR brings things closer to the documented behaviour either

Sure it does: it does fix #357

Indeed, the multi-default routes situation pose question, and it was probably not considered when these options were introduced. But this merge request does fix the (very common) case of an existing default route, and adding another one with a different metric. The case of #357, precisely.

Revamping defaultroute option can be useful, but seems very off-topic for this merge request (which is really about bringing back behavior which was working in the past).