rc.d/routing: Silence loopback routes

[pouria]

Summary

_loopback in static_routes ensures we've loopback route in
all routing tables.
However, loopback route has been added to fib 0 by the kernel itself.
Silence its output and error that appear in every boot.

PR: 259553
MFC after: 2 weeks

Personally, I don't have a problem with:

add host 127.0.0.1: gateway lo0 fib 0: route already in table
add host ::1: gateway lo0 fib 0: route already in table

But I do have a problem with the errors below since the implementation of netlink:

route: message indicates error: File exists
route: message indicates error: File exists

I've silenced them both, but I'm ok with not silencing the first error.

Test Plan

reboot or service routing restart

[markj]

libexec/rc/rc.d/routing

193

I support the goal, but is this too strong? Redirecting all stderr may silence other errors. I guess limiting to loopback helps a bit, but then there should be a comment explaining why we handle it separately.

[jlduran]

As someone who gets:

...
route: message indicates error: File exists
route: message indicates error: File exists
route: message indicates error: File exists
route: message indicates error: File exists
route: message indicates error: File exists
add host 127.0.0.1: gateway lo0 fib 0: route already in table
add host 127.0.0.1: gateway lo0 fib 1: route already in table
add host 127.0.0.1: gateway lo0 fib 2: route already in table
add host 127.0.0.1: gateway lo0 fib 3: route already in table
add host 127.0.0.1: gateway lo0 fib 4: route already in table
...

on every boot, I greatly welcome this patch. So, the reason why we don't remove it above (see inline comment) is because the route won't be shown as static?:

127.0.0.1          link#3             UH              lo0

libexec/rc/rc.d/routing

152

The issue with removing _loopback and route__loopback from here (and in IPv6) is that we want to skip adding the loopback route only for fib 0 (at least that's what I get from the commit message), by silencing (-q), and extra silencing (2>/dev/null) every route add command for lo0?

[glebius]

At what conditions the loopback route does not exist?

[pouria]

In D57470#1317589, @glebius wrote:

At what conditions the loopback route does not exist?

There are several situations in which this can happen:

sysctl net.fibs=2

Increasing net.fibs at runtime empties the corresponding FIB.
Even then, that FIB must contain at least a route to localhost.

You can also trigger the same problem by creating a fresh jail with VNET:

% mdo jail -c name=test vnet persist
[pouria@cornelia] [~] % jls
JID   IP Address     Hostname                      Path
1                                                /
[pouria@cornelia] [~] % mdo jexec -l 1
[root@] [/] # netstat -rn4
Routing tables
[root@] [/] #

Even restarting the routing daemon without any configuration can remove the localhost routes.

I am fine with seeing:

add host 127.0.0.1: gateway lo0 fib 0: route already in table

but the error output is unnecessarily verbose:

route: message indicates error: File exists

Although I don't like redirecting 2>/dev/null in my routing scripts, we do this frequently in rc.d to suppress those.

Maybe we should keep stderr redirection but remove -q?

[glebius]

Maybe filter the very specific message only?

2>&1 >/dev/null | sed -e '/route: message indicates error: File exists/d'

[jlduran]

In D57470#1317912, @glebius wrote:

Maybe filter the very specific message only?

2>&1 >/dev/null | sed -e '/route: message indicates error: File exists/d'

Oh, I remember now, it is a PINNED route.

--- a/libexec/rc/rc.d/routing
+++ b/libexec/rc/rc.d/routing
@@ -190,7 +190,12 @@ static_inet()
                        if [ $_skip = 0 ]; then
                                route_args=`get_if_var ${i%:*} route_IF`
                                if [ -n "$route_args" ]; then
-                                       ${ROUTE_CMD} ${_action} ${route_args}
+                                       if [ "${i%:*}" = "_loopback" ]; then
+                                               ${ROUTE_CMD} ${_action} ${route_args} 2>&1 >/dev/null |
+                                                   sed -e '/route: message indicates error: File exists/d' >&2
+                                       else
+                                               ${ROUTE_CMD} ${_action} ${route_args}
+                                       fi
                                else
                                        warn "route_${i%:*} not found."
                                fi
@@ -267,8 +272,14 @@ static_inet6()
                        if [ $_skip = 0 ]; then
                                ipv6_route_args=`get_if_var ${i%:*} ipv6_route_IF`
                                if [ -n "$ipv6_route_args" ]; then
-                                       ${ROUTE_CMD} ${_action} \
-                                               -inet6 ${ipv6_route_args}
+                                       if [ "${i%:*}" = "_loopback" ]; then
+                                               ${ROUTE_CMD} ${_action} -inet6 \
+                                                   ${ipv6_route_args} 2>&1 >/dev/null |
+                                                   sed -e '/route: message indicates error: File exists/d' >&2
+                                       else
+                                               ${ROUTE_CMD} ${_action} \
+                                                   -inet6 ${ipv6_route_args}
+                                       fi
                                else
                                        warn "route_${i%:*} not found"
                                fi

is fine by me, thanks!

[pouria]

Address @glebius and @jlduran comments.

[pouria]

libexec/rc/rc.d/routing

193

Now we only silence the EEXIST error.
we probably won't need a comment here anymore.

[glebius]

Not beautiful, but the best we can do.

[jlduran]

I have the exact same feeling as @glebius: Ideally we'd check whether the route already exists before invoking route(8), but that's considerably more involved than filtering the expected EEXIST case.
While I'm comfortable blaming the lines and reading the commit messages, a brief comment like:

# Loopback routes may already be added by the kernel; ignore EEXIST.

As @markj suggested would be ideal (though not compulsory).
The commit message initially threw me off. I thought the change was only silencing the message for FIB 0, but as you've probably inferred from my setup (net.add_addr_allfibs=1), it also suppresses it for the other routing tables.
Thank you for all your work!

[pouria]

Address @markj and @jlduran comments.

[markj]

libexec/rc/rc.d/routing

195

Don't you want to remove the >/dev/null redirect?

[jlduran]

libexec/rc/rc.d/routing

195

Unless you want to display:

add host 127.0.0.1: gateway lo0 fib X: route already in table
add host ::1: gateway lo0 fib X: route already in table

Personally, I prefer those messages silenced as well.
To remove the >/dev/null redirect, the -q flag must be set, to silence route's stdout.
Currently, route's stdout is redirected to /dev/null, while stderr is sent through the pipe to sed. sed removes that specific error message, and any remaining output is redirected to stderr.

[pouria]

libexec/rc/rc.d/routing

195

Don't you want to remove the >/dev/null redirect?

I personally prefer to keep the stdout. It helps users to understand how our loopback routes work.
Unfortunately, users complained about the route's output way before its stderr (caused by netlink).
So, IMHO, it's fine as is.

Again, I understand this solution is ugly, but having errors on every boot+routing restart is uglier.

What do you think?