Created attachment 504106 [details, diff] proposed patch Hello. Currently netifrc iproute2 module does not support adding routes with types like "throw" or "blackhole". One of the reasons is because the "dev" option is added to the route unconditionally. I faced with the problem of adding such routes several times. And I was forced to do it by postup function which is not so convenient. I propose this patch to change this behaviour. It checks if specific types are set for the route and drops adding device for it. Also it drops using dev option for "route show" in couple of places. This is not needed to find the route and anyway two similar routes with different devices are not allowed, so it probably will behave better when detecting already defined routes. The only open problem with such routes is that they will not be flushed in interface stop. But I think it will not cause big problems, but the convenience is obvious.
There's 2 problems with your patch as-is: 1. "route show" It is possible (but uncommon) to have the same route on two different interfaces, with different metrics (if they have the same metric, the kernel will reject them as you noted for similar routes) -ip ${family} route show ${cmd_nometric} dev "${IFACE}" 2>/dev/null | \ +ip ${family} route show ${cmd_nometric} 2>/dev/null | \ ... -eval $msgfunc \"$(ip $family route show ${cmd_nometric} dev "${IFACE}" 2>&1)\" +eval $msgfunc \"$(ip $family route show ${cmd_nometric} 2>&1)\" I think that dev should be conditionally added here if it's not one of the no-interface route types. 2. loop for cmd_nometric: + throw|unreachable|prohibit|blackhole) cmd="${cmd} ${1}" ;; This line needs to ALSO append cmd_nometric.
> 1. "route show" Yes. I forgot that it calls "show" without metric when dropped the "dev" here. Thank you for noticing this. But there is a problem with "nodev" routes because if we add "dev", they they would not match. On the other side, metric can not be used for matching (I wonder why??). So it looks like there should be more ifs. :) > 2. loop for cmd_nometric: > + throw|unreachable|prohibit|blackhole) cmd="${cmd} ${1}" ;; > This line needs to ALSO append cmd_nometric. I disagree here. For example "ip route show throw 192.168.0.0/24" will not work. We can probably add "type throw" here, but on the other side it will match only throw routes, but there can be anoter matching routes, which will stop this route from adding, so they should be matched too. Probably the only way to overcome this is to run "ip route show" without metric, and then grep metric of interest. Should I try to do this?
(In reply to Alexander Zubkov from comment #2) > Probably the only way to overcome this is to run "ip route show" without > metric, and then grep metric of interest. Should I try to do this? It already does the grep: 250 # Check for route already existing: 251 ip ${family} route show ${cmd_nometric} dev "${IFACE}" 2>/dev/null | \ 252 fgrep -sq "${cmd%% *}" 253 route_already_exists=$? Build different variables for add vs show. - Rename cmd_nometric to cmd_show. - Rename cmd to cmd_add - For nodev types: -- Set nodev variable. -- cmd_add="${cmd} ${1}" ; cmd_show="${cmd_show} type ${1}" ; nodev=1 - After the loop that builds cmd_show/cmd_add, and the metric/nexthop check, add a new check for nodev variable, and append to cmd_show/cmd_add as needed.
Good. Thank you for the clues. That is was my question about - if I can do something in approved way. But why do you want to do this: > cmd_show="${cmd_show} type ${1}" For example if we have in routing table: 192.168.1.0/24 via 192.168.0.1 And we will try to add: ip route add throw 192.168.1.0/24 - it will fail, because the same prefix with the same metric already there. But this show: ip route show 192.168.1.0/24 type throw will not find that blocker.
(In reply to Alexander Zubkov from comment #4) > But this show: > > ip route show 192.168.1.0/24 type throw > > will not find that blocker. Ah, I see your concern here yes. So for cmd_show, I'm wondering if we can isolate the needed parts (prefix, vrf?, table?) and just pass that to show, and improve how we grep those results.
> # We cannot use a metric if we're using a nexthop But I can add route with metric and several nexthops. May be this is some legacy?
Created attachment 504454 [details, diff] netifrc-1.patch And here first patch for starting, I just renamed cmds and I think there was a bug also: > netmask) x="/$(_netmask2cidr "$2")" ; cmd="${cmd}${x}" ; cmd_nometric="${cmd}${x}" ; shift;; I think cmd_nometric should use cmd_nometric, not cmd? Or it was made intentionally?
(In reply to Alexander Zubkov from comment #7) > Created attachment 504454 [details, diff] [details, diff] > netifrc-1.patch > > And here first patch for starting, I just renamed cmds and I think there was > a bug also: > > > netmask) x="/$(_netmask2cidr "$2")" ; cmd="${cmd}${x}" ; cmd_nometric="${cmd}${x}" ; shift;; > > I think cmd_nometric should use cmd_nometric, not cmd? Or it was made > intentionally? Good catch, yes, that's a bug :-). So on top of this one, now have a second patch that adds the new types. Also that cmd_show ends up with just the prefix. (In reply to Alexander Zubkov from comment #6) > > # We cannot use a metric if we're using a nexthop > > But I can add route with metric and several nexthops. May be this is some > legacy? Good question, will need to dig into the history of that to see.
FYI efec0b5012efd7af5a882348c62e28f3e62c58fc in openrc.git is the original commit for nexthop vs metric.
It is interesting. But I still do not understand the reason for this. :) I'm not sure it needs to be kept. I only can suppose such options: - metric was placed after nexthop and broke things of course - it was confused with weight If this check will be removed - the only problem I can see if someone have 2 routes towards the same prefix. One with nexthops and metric 0 (default) and other of any kind with greater metric. After that change nexthops route can get greater metric than other route or the same. And this will change behaviour.
Created attachment 504470 [details, diff] netifrc-2.patch Second patch. It have several problems. I'm uploading it just to show the idea. There is a function which shows required routes and additionaly fiters metric we need. cmd_show contains only fields which identify route uniquely, except the metric: table, tos. The list of problems to fix: - cmd_show is missing prefix now, we need to figure out it for options, probably the best guess is the first non-special word - remove dev $IFACE when show, because it does not included in "unique key" of the routes - deal with the default route, because "ip route show default" shows all routes for IPv4 (bug?), not only default and "ip route show 0.0.0.0/0" shows only default, but default route is showed as "default" always
Created attachment 504532 [details, diff] netifrc-2.2.patch The thing getting little huge. I am attaching updated second patch for review. But I have not tested it yet, will do it later. It turned out there is no problem with "ip show default" because if we add family to the command - it works as expected. I also split cmd_add into two parts: cmd_prefix and cmd_add. The first should be safe to append metric to. So we can use our default metric with nexthop and similar routes.
Created attachment 505050 [details, diff] netifrc-iproute2-total.patch So I made some basic testing, fixed errors, add couple of additional improvements and here is combined patch. I have tested that routes are adding correctly including "nodev" routes, auto metric is working with multiple nexthop routes, when errors routes are showed in the warning with the same key fields. There is a little thing there. When appending route returns an error, it will dump routes with the same key fields. But as I noticed - routes are _appended_ and so it will fail only if such route really exists. And it will dump not only conflicted route, but all similar routes, which will stop _adding_ routes. Maybe such extended information is good or maybe cmd_add and cmd_show need to be redone to show only conflicting route for appending.
> Maybe such extended information is good or maybe cmd_add and cmd_show need to be redone to show only conflicting route for appending. But I think that it will be hard to cut only such route anyway, because not all options can be passed to show.
By the way, my patch to iproute2 to support filtering by metric was included into upstream: https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/commit/?id=ed7fdc950d641376352061f2799dd2d0dd72accd So when it will get all the way to the stable, that ugly part of scripting for metric filtering could be removed.
Anything stops this from going further? If there any questions to the patch - I can work on them more.
I have the same requirement here. We need to add some blackhole route, and I think here is the best place rather than rc.local, are there any plan? thanks.
I believe my patches is greatly outdated already. But I'm still here and can work on new patches if there is interest from the netifrc team.
