Merge pull request #18318 from yuwata/network-route-table-name

network: RouteTable= improvements

man/networkd.conf.xml

@@ -72,11 +72,14 @@
 
       <varlistentry>
         <term><varname>RouteTable=</varname></term>
-        <listitem><para>Specifies the route table name. Takes a route name and table number separated with a
-        colon. (<literal><replaceable>name</replaceable>:<replaceable>integer</replaceable></literal>. The
-        route table number must be an integer in the range 1…4294967295. This setting can be specified
-        multiple times. If an empty string is specified, then all options specified earlier are cleared.
-        Defaults to unset.</para></listitem>
+        <listitem><para>Defines the route table name. Takes a whitespace-separated list of the pairs of
+        route table name and number. The route table name and number in each pair are separated with a
+        colon, i.e., <literal><replaceable>name</replaceable>:<replaceable>number</replaceable></literal>.
+        The route table name must not be <literal>default</literal>, <literal>main</literal>, or
+        <literal>local</literal>, as these route table names are predefined with route table number 253,
+        254, and 255, respectively. The route table number must be an integer in the range 1…4294967295.
+        This setting can be specified multiple times. If an empty string is specified, then the list
+        specified earlier are cleared. Defaults to unset.</para></listitem>
       </varlistentry>
 
     </variablelist>

src/network/networkd-gperf.gperf

@@ -23,6 +23,6 @@ struct ConfigPerfItem;
 Network.SpeedMeter,            config_parse_bool,                      0,          offsetof(Manager, use_speed_meter)
 Network.SpeedMeterIntervalSec, config_parse_sec,                       0,          offsetof(Manager, speed_meter_interval_usec)
 Network.ManageForeignRoutes,   config_parse_bool,                      0,          offsetof(Manager, manage_foreign_routes)
-Network.RouteTable,            config_parse_route_table_names,         0,          offsetof(Manager, route_tables)
+Network.RouteTable,            config_parse_route_table_names,         0,          0
 DHCP.DUIDType,                 config_parse_duid_type,                 0,          offsetof(Manager, duid)
 DHCP.DUIDRawData,              config_parse_duid_rawdata,              0,          offsetof(Manager, duid)

src/network/networkd-manager.c

@@ -875,7 +875,8 @@ void manager_free(Manager *m) {
 
         ordered_set_free_free(m->address_pools);
 
-        m->route_tables = hashmap_free_free_key(m->route_tables);
+        hashmap_free(m->route_table_names_by_number);
+        hashmap_free(m->route_table_numbers_by_name);
 
         /* routing_policy_rule_free() access m->rules and m->rules_foreign.
          * So, it is necessary to set NULL after the sets are freed. */

src/network/networkd-manager.h

@@ -66,7 +66,8 @@ struct Manager {
         Set *routes_foreign;
 
         /* Route table name */
-        Hashmap *route_tables;
+        Hashmap *route_table_numbers_by_name;
+        Hashmap *route_table_names_by_number;
 
         /* For link speed meter*/
         bool use_speed_meter;

src/network/networkd-route.c

@@ -87,20 +87,68 @@ static const char * const route_table_table[] = {
         [RT_TABLE_LOCAL]   = "local",
 };
 
-DEFINE_STRING_TABLE_LOOKUP(route_table, int);
+DEFINE_PRIVATE_STRING_TABLE_LOOKUP(route_table, int);
 
-#define ROUTE_TABLE_STR_MAX CONST_MAX(DECIMAL_STR_MAX(int), STRLEN("default") + 1)
-static const char *format_route_table(int table, char *buf, size_t size) {
+int manager_get_route_table_from_string(const Manager *m, const char *s, uint32_t *ret) {
+        uint32_t t;
+        int r;
+
+        assert(m);
+        assert(s);
+        assert(ret);
+
+        r = route_table_from_string(s);
+        if (r >= 0) {
+                *ret = (uint32_t) r;
+                return 0;
+        }
+
+        t = PTR_TO_UINT32(hashmap_get(m->route_table_numbers_by_name, s));
+        if (t != 0) {
+                *ret = t;
+                return 0;
+        }
+
+        r = safe_atou32(s, &t);
+        if (r < 0)
+                return r;
+
+        if (t == 0)
+                return -ERANGE;
+
+        *ret = t;
+        return 0;
+}
+
+int manager_get_route_table_to_string(const Manager *m, uint32_t table, char **ret) {
+        _cleanup_free_ char *str = NULL;
         const char *s;
-        char *p = buf;
+
+        assert(m);
+        assert(ret);
+
+        if (table == 0)
+                return -EINVAL;
 
         s = route_table_to_string(table);
-        if (s)
-                strpcpy(&p, size, s);
-        else
-                strpcpyf(&p, size, "%d", table);
+        if (!s)
+                s = hashmap_get(m->route_table_names_by_number, UINT32_TO_PTR(table));
 
-        return buf;
+        if (s) {
+                /* Currently, this is only used in debugging logs. To not confuse any bug
+                 * reports, let's include the table number. */
+                if (asprintf(&str, "%s(%" PRIu32 ")", s, table) < 0)
+                        return -ENOMEM;
+
+                *ret = TAKE_PTR(str);
+                return 0;
+        }
+
+        if (asprintf(&str, "%" PRIu32, table) < 0)
+                return -ENOMEM;
+
+        *ret = TAKE_PTR(str);
+        return 0;
 }
 
 static const char * const route_protocol_table[] = {
@@ -584,15 +632,16 @@ static bool route_type_is_reject(const Route *route) {
         return IN_SET(route->type, RTN_UNREACHABLE, RTN_PROHIBIT, RTN_BLACKHOLE, RTN_THROW);
 }
 
-static void log_route_debug(const Route *route, const char *str, const Link *link) {
+static void log_route_debug(const Route *route, const char *str, const Link *link, const Manager *m) {
         assert(route);
         assert(str);
+        assert(m);
 
         /* link may be NULL. */
 
         if (DEBUG_LOGGING) {
-                _cleanup_free_ char *dst = NULL, *dst_prefixlen = NULL, *src = NULL, *gw = NULL, *prefsrc = NULL;
-                char scope[ROUTE_SCOPE_STR_MAX], table[ROUTE_TABLE_STR_MAX], protocol[ROUTE_PROTOCOL_STR_MAX];
+                _cleanup_free_ char *dst = NULL, *dst_prefixlen = NULL, *src = NULL, *gw = NULL, *prefsrc = NULL, *table = NULL;
+                char scope[ROUTE_SCOPE_STR_MAX], protocol[ROUTE_PROTOCOL_STR_MAX];
 
                 if (!in_addr_is_null(route->family, &route->dst)) {
                         (void) in_addr_to_string(route->family, &route->dst, &dst);
@@ -604,12 +653,13 @@ static void log_route_debug(const Route *route, const char *str, const Link *lin
                         (void) in_addr_to_string(route->gw_family, &route->gw, &gw);
                 if (!in_addr_is_null(route->family, &route->prefsrc))
                         (void) in_addr_to_string(route->family, &route->prefsrc, &prefsrc);
+                (void) manager_get_route_table_to_string(m, route->table, &table);
 
                 log_link_debug(link,
                                "%s route: dst: %s%s, src: %s, gw: %s, prefsrc: %s, scope: %s, table: %s, proto: %s, type: %s",
                                str, strna(dst), strempty(dst_prefixlen), strna(src), strna(gw), strna(prefsrc),
                                format_route_scope(route->scope, scope, sizeof(scope)),
-                               format_route_table(route->table, table, sizeof(table)),
+                               strna(table),
                                format_route_protocol(route->protocol, protocol, sizeof(protocol)),
                                strna(route_type_to_string(route->type)));
         }
@@ -751,7 +801,7 @@ int route_remove(
                 manager = link->manager;
         /* link may be NULL! */
 
-        log_route_debug(route, "Removing", link);
+        log_route_debug(route, "Removing", link, manager);
 
         r = sd_rtnl_message_new_route(manager->rtnl, &req,
                                       RTM_DELROUTE, route->family,
@@ -1066,7 +1116,7 @@ int route_configure(
                 return log_link_error_errno(link, SYNTHETIC_ERRNO(E2BIG),
                                             "Too many routes are configured, refusing: %m");
 
-        log_route_debug(route, "Configuring", link);
+        log_route_debug(route, "Configuring", link, link->manager);
 
         r = sd_rtnl_message_new_route(link->manager->rtnl, &req,
                                       RTM_NEWROUTE, route->family,
@@ -1295,10 +1345,10 @@ static int process_route_one(Manager *manager, Link *link, uint16_t type, const
         case RTM_NEWROUTE:
                 if (!route) {
                         if (!manager->manage_foreign_routes)
-                                log_route_debug(tmp, "Ignoring received foreign", link);
+                                log_route_debug(tmp, "Ignoring received foreign", link, manager);
                         else {
                                 /* A route appeared that we did not request */
-                                log_route_debug(tmp, "Remembering foreign", link);
+                                log_route_debug(tmp, "Remembering foreign", link, manager);
                                 r = route_add_foreign(manager, link, tmp, NULL);
                                 if (r < 0) {
                                         log_link_warning_errno(link, r, "Failed to remember foreign route, ignoring: %m");
@@ -1306,14 +1356,15 @@ static int process_route_one(Manager *manager, Link *link, uint16_t type, const
                                 }
                         }
                 } else
-                        log_route_debug(tmp, "Received remembered", link);
+                        log_route_debug(tmp, "Received remembered", link, manager);
 
                 break;
 
         case RTM_DELROUTE:
                 log_route_debug(tmp,
                                 route ? "Forgetting" :
-                                manager->manage_foreign_routes ? "Kernel removed unknown" : "Ignoring received foreign", link);
+                                manager->manage_foreign_routes ? "Kernel removed unknown" : "Ignoring received foreign",
+                                link, manager);
                 route_free(route);
                 break;
 
@@ -1868,28 +1919,6 @@ int config_parse_route_scope(
         return 0;
 }
 
-int route_table_from_string_full(Manager *m, const char *s, uint32_t *ret) {
-        int r;
-
-        assert(s);
-        assert(m);
-        assert(ret);
-
-        r = route_table_from_string(s);
-        if (r >= 0) {
-                *ret = (uint32_t) r;
-                return 0;
-        }
-
-        uint32_t t = PTR_TO_UINT32(hashmap_get(m->route_tables, s));
-        if (t != 0) {
-                *ret = t;
-                return 0;
-        }
-
-        return safe_atou32(s, ret);
-}
-
 int config_parse_route_table(
                 const char *unit,
                 const char *filename,
@@ -1921,7 +1950,7 @@ int config_parse_route_table(
                 return 0;
         }
 
-        r = route_table_from_string_full(network->manager, rvalue, &n->table);
+        r = manager_get_route_table_from_string(network->manager, rvalue, &n->table);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r,
                            "Could not parse route table number \"%s\", ignoring assignment: %m", rvalue);
@@ -2385,63 +2414,98 @@ int config_parse_route_table_names(
                 void *data,
                 void *userdata) {
 
-        _cleanup_free_ char *name = NULL;
-        Hashmap **s = data;
-        uint32_t table;
-        const char *p;
+        Manager *m = userdata;
         int r;
 
         assert(filename);
         assert(lvalue);
         assert(rvalue);
-        assert(data);
+        assert(userdata);
 
         if (isempty(rvalue)) {
-                *s = hashmap_free_free_key(*s);
+                m->route_table_names_by_number = hashmap_free(m->route_table_names_by_number);
+                m->route_table_numbers_by_name = hashmap_free(m->route_table_numbers_by_name);
                 return 0;
         }
 
-        p = rvalue;
-        r = extract_first_word(&p, &name, ":", 0);
-        if (r == -ENOMEM)
-                return log_oom();
-        if (r <= 0 || isempty(p)) {
-                log_syntax(unit, LOG_WARNING, filename, line, r,
-                           "Invalid RouteTable=, ignoring assignment: %s", rvalue);
-                return 0;
-        }
+        for (const char *p = rvalue;;) {
+                _cleanup_free_ char *name = NULL;
+                uint32_t table;
+                char *num;
 
-        if (STR_IN_SET(name, "default", "main","local")) {
-                log_syntax(unit, LOG_WARNING, filename, line, r,
-                           "Route table name %s already preconfigured. Ignoring assignment: %s", name, rvalue);
-                return 0;
-        }
+                r = extract_first_word(&p, &name, NULL, 0);
+                if (r == -ENOMEM)
+                        return log_oom();
+                if (r < 0) {
+                        log_syntax(unit, LOG_WARNING, filename, line, r,
+                                   "Invalid RouteTable=, ignoring assignment: %s", rvalue);
+                        return 0;
+                }
+                if (r == 0)
+                        return 0;
 
-        r = safe_atou32(p, &table);
-        if (r < 0) {
-                log_syntax(unit, LOG_WARNING, filename, line, r,
-                           "Failed to parse RouteTable=, ignoring assignment: %s", p);
-                return 0;
-        }
+                num = strchr(name, ':');
+                if (!num) {
+                        log_syntax(unit, LOG_WARNING, filename, line, 0,
+                                   "Invalid route table name and number pair, ignoring assignment: %s", name);
+                        continue;
+                }
 
-        if (table == 0) {
-                log_syntax(unit, LOG_WARNING, filename, line, 0,
-                           "Invalid RouteTable=, ignoring assignment: %s", p);
-                return 0;
-        }
+                *num++ = '\0';
+
+                if (STR_IN_SET(name, "default", "main", "local")) {
+                        log_syntax(unit, LOG_WARNING, filename, line, 0,
+                                   "Route table name %s already predefined. Ignoring assignment: %s:%s", name, name, num);
+                        continue;
+                }
+
+                r = safe_atou32(num, &table);
+                if (r < 0) {
+                        log_syntax(unit, LOG_WARNING, filename, line, r,
+                                   "Failed to parse route table number '%s', ignoring assignment: %s:%s", num, name, num);
+                        continue;
+                }
+                if (table == 0) {
+                        log_syntax(unit, LOG_WARNING, filename, line, 0,
+                                   "Invalid route table number, ignoring assignment: %s:%s", name, num);
+                        continue;
+                }
+
+                r = hashmap_ensure_put(&m->route_table_numbers_by_name, &string_hash_ops_free, name, UINT32_TO_PTR(table));
+                if (r == -ENOMEM)
+                        return log_oom();
+                if (r == -EEXIST) {
+                        log_syntax(unit, LOG_WARNING, filename, line, r,
+                                   "Specified route table name and number pair conflicts with others, ignoring assignment: %s:%s", name, num);
+                        continue;
+                }
+                if (r < 0) {
+                        log_syntax(unit, LOG_WARNING, filename, line, r,
+                                   "Failed to store route table name and number pair, ignoring assignment: %s:%s", name, num);
+                        continue;
+                }
+                if (r == 0)
+                        /* The entry is duplicated. It should not be added to route_table_names_by_number hashmap. */
+                        continue;
+
+                r = hashmap_ensure_put(&m->route_table_names_by_number, NULL, UINT32_TO_PTR(table), name);
+                if (r < 0) {
+                        hashmap_remove(m->route_table_numbers_by_name, name);
+
+                        if (r == -ENOMEM)
+                                return log_oom();
+                        if (r == -EEXIST)
+                                log_syntax(unit, LOG_WARNING, filename, line, r,
+                                           "Specified route table name and number pair conflicts with others, ignoring assignment: %s:%s", name, num);
+                        else
+                                log_syntax(unit, LOG_WARNING, filename, line, r,
+                                           "Failed to store route table name and number pair, ignoring assignment: %s:%s", name, num);
+                        continue;
+                }
+                assert(r > 0);
 
-        r = hashmap_ensure_put(s, &string_hash_ops, name, UINT32_TO_PTR(table));
-        if (r == -ENOMEM)
-                return log_oom();
-        if (r == -EEXIST) {
-                log_syntax(unit, LOG_WARNING, filename, line, r,
-                           "Specified RouteTable= name and value pair conflicts with others, ignoring assignment: %s", rvalue);
-                return 0;
-        }
-        if (r > 0)
                 TAKE_PTR(name);
-
-        return 0;
+        }
 }
 
 static int route_section_verify(Route *route, Network *network) {

src/network/networkd-route.h

@@ -86,10 +86,8 @@ int network_add_ipv4ll_route(Network *network);
 int network_add_default_route_on_device(Network *network);
 void network_drop_invalid_routes(Network *network);
 
-int route_table_from_string_full(Manager *m, const char *table, uint32_t *ret);
-
-const char *route_table_to_string(int d) _const_;
-int route_table_from_string(const char *d) _pure_;
+int manager_get_route_table_from_string(const Manager *m, const char *table, uint32_t *ret);
+int manager_get_route_table_to_string(const Manager *m, uint32_t table, char **ret);
 
 CONFIG_PARSER_PROTOTYPE(config_parse_gateway);
 CONFIG_PARSER_PROTOTYPE(config_parse_preferred_src);

src/network/networkd-routing-policy-rule.c

@@ -389,23 +389,25 @@ static int routing_policy_rule_consume_foreign(Manager *m, RoutingPolicyRule *ru
         return 1;
 }
 
-static void log_routing_policy_rule_debug(const RoutingPolicyRule *rule, int family, const char *str, const Link *link) {
+static void log_routing_policy_rule_debug(const RoutingPolicyRule *rule, int family, const char *str, const Link *link, const Manager *m) {
         assert(rule);
         assert(IN_SET(family, AF_INET, AF_INET6));
         assert(str);
+        assert(m);
 
         /* link may be NULL. */
 
         if (DEBUG_LOGGING) {
-                _cleanup_free_ char *from = NULL, *to = NULL;
+                _cleanup_free_ char *from = NULL, *to = NULL, *table = NULL;
 
                 (void) in_addr_to_string(family, &rule->from, &from);
                 (void) in_addr_to_string(family, &rule->to, &to);
+                (void) manager_get_route_table_to_string(m, rule->table, &table);
 
                 log_link_debug(link,
-                               "%s routing policy rule: priority: %"PRIu32", %s/%u -> %s/%u, iif: %s, oif: %s, table: %"PRIu32,
+                               "%s routing policy rule: priority: %"PRIu32", %s/%u -> %s/%u, iif: %s, oif: %s, table: %s",
                                str, rule->priority, strna(from), rule->from_prefixlen, strna(to), rule->to_prefixlen,
-                               strna(rule->iif), strna(rule->oif), rule->table);
+                               strna(rule->iif), strna(rule->oif), strna(table));
         }
 }
 
@@ -551,7 +553,7 @@ static int routing_policy_rule_remove(const RoutingPolicyRule *rule, Manager *ma
         assert(manager->rtnl);
         assert(IN_SET(rule->family, AF_INET, AF_INET6));
 
-        log_routing_policy_rule_debug(rule, rule->family, "Removing", NULL);
+        log_routing_policy_rule_debug(rule, rule->family, "Removing", NULL, manager);
 
         r = sd_rtnl_message_new_routing_policy_rule(manager->rtnl, &m, RTM_DELRULE, rule->family);
         if (r < 0)
@@ -610,7 +612,7 @@ static int routing_policy_rule_configure_internal(const RoutingPolicyRule *rule,
         assert(link->manager);
         assert(link->manager->rtnl);
 
-        log_routing_policy_rule_debug(rule, family, "Configuring", link);
+        log_routing_policy_rule_debug(rule, family, "Configuring", link, link->manager);
 
         r = sd_rtnl_message_new_routing_policy_rule(link->manager->rtnl, &m, RTM_NEWRULE, family);
         if (r < 0)
@@ -973,9 +975,9 @@ int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, Man
         switch (type) {
         case RTM_NEWRULE:
                 if (rule)
-                        log_routing_policy_rule_debug(tmp, tmp->family, "Received remembered", NULL);
+                        log_routing_policy_rule_debug(tmp, tmp->family, "Received remembered", NULL, m);
                 else {
-                        log_routing_policy_rule_debug(tmp, tmp->family, "Remembering foreign", NULL);
+                        log_routing_policy_rule_debug(tmp, tmp->family, "Remembering foreign", NULL, m);
                         r = routing_policy_rule_consume_foreign(m, TAKE_PTR(tmp));
                         if (r < 0)
                                 log_warning_errno(r, "Could not remember foreign rule, ignoring: %m");
@@ -983,10 +985,10 @@ int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, Man
                 break;
         case RTM_DELRULE:
                 if (rule) {
-                        log_routing_policy_rule_debug(tmp, tmp->family, "Forgetting", NULL);
+                        log_routing_policy_rule_debug(tmp, tmp->family, "Forgetting", NULL, m);
                         routing_policy_rule_free(rule);
                 } else
-                        log_routing_policy_rule_debug(tmp, tmp->family, "Kernel removed unknown", NULL);
+                        log_routing_policy_rule_debug(tmp, tmp->family, "Kernel removed unknown", NULL, m);
                 break;
 
         default:
@@ -1130,7 +1132,7 @@ int config_parse_routing_policy_rule_table(
         if (r < 0)
                 return log_oom();
 
-        r = route_table_from_string_full(network->manager, rvalue, &n->table);
+        r = manager_get_route_table_from_string(network->manager, rvalue, &n->table);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r,
                            "Could not parse RPDB rule route table number \"%s\", ignoring assignment: %m", rvalue);

src/network/test-network.c

@@ -12,6 +12,7 @@
 #include "network-internal.h"
 #include "networkd-manager.h"
 #include "string-util.h"
+#include "strv.h"
 #include "tests.h"
 
 static void test_deserialize_in_addr(void) {
@@ -101,6 +102,53 @@ static void test_deserialize_dhcp_routes(void) {
         }
 }
 
+static void test_route_tables_one(Manager *manager, const char *name, uint32_t number) {
+        _cleanup_free_ char *str = NULL, *expected = NULL, *num_str = NULL;
+        uint32_t t;
+
+        if (!STR_IN_SET(name, "default", "main", "local")) {
+                assert_se(streq(hashmap_get(manager->route_table_names_by_number, UINT32_TO_PTR(number)), name));
+                assert_se(PTR_TO_UINT32(hashmap_get(manager->route_table_numbers_by_name, name)) == number);
+        }
+
+        assert_se(asprintf(&expected, "%s(%" PRIu32 ")", name, number) >= 0);
+        assert_se(manager_get_route_table_to_string(manager, number, &str) >= 0);
+        assert_se(streq(str, expected));
+
+        assert_se(manager_get_route_table_from_string(manager, name, &t) >= 0);
+        assert_se(t == number);
+
+        assert_se(asprintf(&num_str, "%" PRIu32, number) >= 0);
+        assert_se(manager_get_route_table_from_string(manager, num_str, &t) >= 0);
+        assert_se(t == number);
+}
+
+static void test_route_tables(Manager *manager) {
+        assert_se(config_parse_route_table_names("manager", "filename", 1, "section", 1, "RouteTable", 0, "hoge:123 foo:456 aaa:111", manager, manager) >= 0);
+        assert_se(config_parse_route_table_names("manager", "filename", 1, "section", 1, "RouteTable", 0, "bbb:11111 ccc:22222", manager, manager) >= 0);
+        assert_se(config_parse_route_table_names("manager", "filename", 1, "section", 1, "RouteTable", 0, "ddd:22222", manager, manager) >= 0);
+
+        test_route_tables_one(manager, "hoge", 123);
+        test_route_tables_one(manager, "foo", 456);
+        test_route_tables_one(manager, "aaa", 111);
+        test_route_tables_one(manager, "bbb", 11111);
+        test_route_tables_one(manager, "ccc", 22222);
+
+        assert_se(!hashmap_get(manager->route_table_numbers_by_name, "ddd"));
+
+        test_route_tables_one(manager, "default", 253);
+        test_route_tables_one(manager, "main", 254);
+        test_route_tables_one(manager, "local", 255);
+
+        assert_se(config_parse_route_table_names("manager", "filename", 1, "section", 1, "RouteTable", 0, "", manager, manager) >= 0);
+        assert_se(!manager->route_table_names_by_number);
+        assert_se(!manager->route_table_numbers_by_name);
+
+        test_route_tables_one(manager, "default", 253);
+        test_route_tables_one(manager, "main", 254);
+        test_route_tables_one(manager, "local", 255);
+}
+
 static int test_load_config(Manager *manager) {
         int r;
 /*  TODO: should_reload, is false if the config dirs do not exist, so
@@ -241,6 +289,8 @@ int main(void) {
 
         assert_se(manager_new(&manager) >= 0);
 
+        test_route_tables(manager);
+
         r = test_load_config(manager);
         if (r == -EPERM)
                 return log_tests_skipped("Cannot load configuration");