From f65747f6e9edc4a6d01d8825e6c6bc3da284c6b9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 22:23:44 +0100 Subject: [PATCH 01/33] tests: let "run-nm-test.sh" fail with exit code 1 on failure `git bisect run` is peculiar about the exit code: error: bisect run failed: exit code 134 from '...' is < 0 or >= 128 If we just "exec" the test, it usually will fail on an assert. That results in SIGABRT or exit code 134. So out of the box that is annoying with git-bisect. Work around that and let the test wrapper always coerce any test failure to exit code 1. --- tools/run-nm-test.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/run-nm-test.sh b/tools/run-nm-test.sh index b1d3dd939a..59a489332a 100755 --- a/tools/run-nm-test.sh +++ b/tools/run-nm-test.sh @@ -300,9 +300,10 @@ fi if ! _is_true "$NMTST_USE_VALGRIND" 0; then export NM_TEST_UNDER_VALGRIND=0 - exec "${NMTST_DBUS_RUN_SESSION[@]}" \ - "$TEST" "${TEST_ARGV[@]}" - die "exec \"$TEST\" failed" + "${NMTST_DBUS_RUN_SESSION[@]}" "$TEST" "${TEST_ARGV[@]}" + r=$? + [ $r == 0 ] || die "exec \"$TEST\" failed with exit code $r" + exit 0 fi if [[ -z "${NMTST_VALGRIND}" ]]; then From 6208a1bb8456f5649bf7ffb8d8d20cbe62bfd4aa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 22:49:37 +0100 Subject: [PATCH 02/33] libnm: reorder fields in NMIPAddress/NMIPRoute struct Order the fields by their size, to minimize the alignment gaps. I guess, that doesn't matter because the alignment of the heap allocation is larger than what we can safe here. Still, there is on reason to do it any other way. Also, it's not possible via API to set family/prefix to values outside their range, so an 8bit integer is always sufficient. And we don't want that invariant to change. We don't ever want to allow the caller to set values that are clearly invalid, and will assert against that early (g_return()). Point is, we can do this and there is no danger of future problems. And even if we will support larger values, it's all an implementation detail anyway. --- src/libnm-core-impl/nm-setting-ip-config.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 0dc0a58610..1dbf92537e 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -169,9 +169,10 @@ G_DEFINE_BOXED_TYPE(NMIPAddress, nm_ip_address, nm_ip_address_dup, nm_ip_address struct NMIPAddress { guint refcount; - char *address; - int prefix, family; + gint8 family; + guint8 prefix; + char *address; GHashTable *attributes; }; @@ -608,13 +609,14 @@ G_DEFINE_BOXED_TYPE(NMIPRoute, nm_ip_route, nm_ip_route_dup, nm_ip_route_unref) struct NMIPRoute { guint refcount; - int family; - char *dest; - guint prefix; - char *next_hop; - gint64 metric; + gint8 family; + guint8 prefix; + char *dest; + char *next_hop; GHashTable *attributes; + + gint64 metric; }; /** From 00e4f216299cf1f3e840f46b94b3575b434f97fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 23:10:48 +0100 Subject: [PATCH 03/33] libnm: avoid parsing IP addresses twice in NMIPAddress/NMIPRoute API Usually the normalization (canonicalize) and validation of the IP address string both requires to parse the string. As we always do validation first, we can use the parsed address and don't need to parse it a second time. --- src/libnm-core-impl/nm-setting-ip-config.c | 221 ++++++++++----------- 1 file changed, 110 insertions(+), 111 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 1dbf92537e..9e42bc7d81 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -54,56 +54,24 @@ const NMUtilsDNSOptionDesc _nm_utils_dns_option_descs[] = { {NULL, FALSE, FALSE}}; static char * -canonicalize_ip(int family, const char *ip, gboolean null_any) +canonicalize_ip_binary(int family, const NMIPAddr *ip, gboolean null_any) { - guint8 addr_bytes[sizeof(struct in6_addr)]; - char addr_str[NM_UTILS_INET_ADDRSTRLEN]; - int ret; - if (!ip) { if (null_any) return NULL; - if (family == AF_INET) + if (NM_IS_IPv4(family)) return g_strdup("0.0.0.0"); - if (family == AF_INET6) - return g_strdup("::"); - g_return_val_if_reached(NULL); + return g_strdup("::"); } - ret = inet_pton(family, ip, addr_bytes); - g_return_val_if_fail(ret == 1, NULL); + if (null_any && nm_ip_addr_is_null(family, ip)) + return NULL; - if (null_any) { - if (!memcmp(addr_bytes, &in6addr_any, nm_utils_addr_family_to_size(family))) - return NULL; - } - - return g_strdup(inet_ntop(family, addr_bytes, addr_str, sizeof(addr_str))); -} - -static char * -canonicalize_ip_binary(int family, gconstpointer ip, gboolean null_any) -{ - char string[NM_UTILS_INET_ADDRSTRLEN]; - - if (!ip) { - if (null_any) - return NULL; - if (family == AF_INET) - return g_strdup("0.0.0.0"); - if (family == AF_INET6) - return g_strdup("::"); - g_return_val_if_reached(NULL); - } - if (null_any) { - if (!memcmp(ip, &in6addr_any, nm_utils_addr_family_to_size(family))) - return NULL; - } - return g_strdup(inet_ntop(family, ip, string, sizeof(string))); + return nm_utils_inet_ntop_dup(family, ip); } static gboolean -valid_ip(int family, const char *ip, GError **error) +valid_ip(int family, const char *ip, NMIPAddr *addr, GError **error) { if (!ip) { g_set_error(error, @@ -112,7 +80,7 @@ valid_ip(int family, const char *ip, GError **error) family == AF_INET ? _("Missing IPv4 address") : _("Missing IPv6 address")); return FALSE; } - if (!nm_utils_ipaddr_is_valid(family, ip)) { + if (!nm_utils_parse_inaddr_bin(family, ip, NULL, addr)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_FAILED, @@ -120,8 +88,9 @@ valid_ip(int family, const char *ip, GError **error) : _("Invalid IPv6 address '%s'"), ip); return FALSE; - } else - return TRUE; + } + + return TRUE; } static gboolean @@ -192,21 +161,23 @@ NMIPAddress * nm_ip_address_new(int family, const char *addr, guint prefix, GError **error) { NMIPAddress *address; + NMIPAddr addr_bin; g_return_val_if_fail(family == AF_INET || family == AF_INET6, NULL); g_return_val_if_fail(addr != NULL, NULL); - if (!valid_ip(family, addr, error)) + if (!valid_ip(family, addr, &addr_bin, error)) return NULL; if (!valid_prefix(family, prefix, error)) return NULL; - address = g_slice_new0(NMIPAddress); - address->refcount = 1; - - address->family = family; - address->address = canonicalize_ip(family, addr, FALSE); - address->prefix = prefix; + address = g_slice_new(NMIPAddress); + *address = (NMIPAddress){ + .refcount = 1, + .family = family, + .address = canonicalize_ip_binary(family, &addr_bin, FALSE), + .prefix = prefix, + }; return address; } @@ -228,7 +199,6 @@ NMIPAddress * nm_ip_address_new_binary(int family, gconstpointer addr, guint prefix, GError **error) { NMIPAddress *address; - char string[NM_UTILS_INET_ADDRSTRLEN]; g_return_val_if_fail(family == AF_INET || family == AF_INET6, NULL); g_return_val_if_fail(addr != NULL, NULL); @@ -236,12 +206,13 @@ nm_ip_address_new_binary(int family, gconstpointer addr, guint prefix, GError ** if (!valid_prefix(family, prefix, error)) return NULL; - address = g_slice_new0(NMIPAddress); - address->refcount = 1; - - address->family = family; - address->address = g_strdup(inet_ntop(family, addr, string, sizeof(string))); - address->prefix = prefix; + address = g_slice_new(NMIPAddress); + *address = (NMIPAddress){ + .refcount = 1, + .family = family, + .address = nm_utils_inet_ntop_dup(family, addr), + .prefix = prefix, + }; return address; } @@ -277,9 +248,8 @@ nm_ip_address_unref(NMIPAddress *address) address->refcount--; if (address->refcount == 0) { g_free(address->address); - if (address->attributes) - g_hash_table_unref(address->attributes); - g_slice_free(NMIPAddress, address); + nm_g_hash_table_unref(address->attributes); + nm_g_slice_free(address); } } @@ -442,12 +412,18 @@ nm_ip_address_get_address(NMIPAddress *address) void nm_ip_address_set_address(NMIPAddress *address, const char *addr) { + NMIPAddr addr_bin; + g_return_if_fail(address != NULL); - g_return_if_fail(addr != NULL); - g_return_if_fail(nm_utils_ipaddr_is_valid(address->family, addr)); + + if (!valid_ip(address->family, addr, &addr_bin, NULL)) { + g_return_if_fail(addr != NULL); + g_return_if_fail(nm_utils_ipaddr_is_valid(address->family, addr)); + nm_assert_not_reached(); + } g_free(address->address); - address->address = canonicalize_ip(address->family, addr, FALSE); + address->address = canonicalize_ip_binary(address->family, &addr_bin, FALSE); } /** @@ -480,13 +456,11 @@ nm_ip_address_get_address_binary(NMIPAddress *address, gpointer addr) void nm_ip_address_set_address_binary(NMIPAddress *address, gconstpointer addr) { - char string[NM_UTILS_INET_ADDRSTRLEN]; - g_return_if_fail(address != NULL); g_return_if_fail(addr != NULL); g_free(address->address); - address->address = g_strdup(inet_ntop(address->family, addr, string, sizeof(string))); + address->address = nm_utils_inet_ntop_dup(address->family, addr); } /** @@ -642,27 +616,30 @@ nm_ip_route_new(int family, GError **error) { NMIPRoute *route; + NMIPAddr dest_bin; + NMIPAddr next_hop_bin; g_return_val_if_fail(family == AF_INET || family == AF_INET6, NULL); g_return_val_if_fail(dest, NULL); - if (!valid_ip(family, dest, error)) + if (!valid_ip(family, dest, &dest_bin, error)) return NULL; if (!valid_prefix(family, prefix, error)) return NULL; - if (next_hop && !valid_ip(family, next_hop, error)) + if (next_hop && !valid_ip(family, next_hop, &next_hop_bin, error)) return NULL; if (!valid_metric(metric, error)) return NULL; - route = g_slice_new0(NMIPRoute); - route->refcount = 1; - - route->family = family; - route->dest = canonicalize_ip(family, dest, FALSE); - route->prefix = prefix; - route->next_hop = canonicalize_ip(family, next_hop, TRUE); - route->metric = metric; + route = g_slice_new(NMIPRoute); + *route = (NMIPRoute){ + .refcount = 1, + .family = family, + .dest = canonicalize_ip_binary(family, &dest_bin, FALSE), + .prefix = prefix, + .next_hop = canonicalize_ip_binary(family, next_hop ? &next_hop_bin : NULL, TRUE), + .metric = metric, + }; return route; } @@ -700,14 +677,15 @@ nm_ip_route_new_binary(int family, if (!valid_metric(metric, error)) return NULL; - route = g_slice_new0(NMIPRoute); - route->refcount = 1; - - route->family = family; - route->dest = canonicalize_ip_binary(family, dest, FALSE); - route->prefix = prefix; - route->next_hop = canonicalize_ip_binary(family, next_hop, TRUE); - route->metric = metric; + route = g_slice_new0(NMIPRoute); + *route = (NMIPRoute){ + .refcount = 1, + .family = family, + .dest = canonicalize_ip_binary(family, dest, FALSE), + .prefix = prefix, + .next_hop = canonicalize_ip_binary(family, next_hop, TRUE), + .metric = metric, + }; return route; } @@ -744,9 +722,8 @@ nm_ip_route_unref(NMIPRoute *route) if (route->refcount == 0) { g_free(route->dest); g_free(route->next_hop); - if (route->attributes) - g_hash_table_unref(route->attributes); - g_slice_free(NMIPRoute, route); + nm_g_hash_table_unref(route->attributes); + nm_g_slice_free(route); } } @@ -911,11 +888,17 @@ nm_ip_route_get_dest(NMIPRoute *route) void nm_ip_route_set_dest(NMIPRoute *route, const char *dest) { + NMIPAddr dest_bin; + g_return_if_fail(route != NULL); - g_return_if_fail(nm_utils_ipaddr_is_valid(route->family, dest)); + + if (!valid_ip(route->family, dest, &dest_bin, NULL)) { + g_return_if_fail(nm_utils_ipaddr_is_valid(route->family, dest)); + nm_assert_not_reached(); + } g_free(route->dest); - route->dest = canonicalize_ip(route->family, dest, FALSE); + route->dest = canonicalize_ip_binary(route->family, &dest_bin, FALSE); } /** @@ -948,13 +931,11 @@ nm_ip_route_get_dest_binary(NMIPRoute *route, gpointer dest) void nm_ip_route_set_dest_binary(NMIPRoute *route, gconstpointer dest) { - char string[NM_UTILS_INET_ADDRSTRLEN]; - g_return_if_fail(route != NULL); g_return_if_fail(dest != NULL); g_free(route->dest); - route->dest = g_strdup(inet_ntop(route->family, dest, string, sizeof(string))); + route->dest = nm_utils_inet_ntop_dup(route->family, dest); } /** @@ -1022,11 +1003,17 @@ nm_ip_route_get_next_hop(NMIPRoute *route) void nm_ip_route_set_next_hop(NMIPRoute *route, const char *next_hop) { + NMIPAddr next_hop_bin; + g_return_if_fail(route != NULL); - g_return_if_fail(!next_hop || nm_utils_ipaddr_is_valid(route->family, next_hop)); + + if (next_hop && !valid_ip(route->family, next_hop, &next_hop_bin, NULL)) { + g_return_if_fail(!next_hop || nm_utils_ipaddr_is_valid(route->family, next_hop)); + nm_assert_not_reached(); + } g_free(route->next_hop); - route->next_hop = canonicalize_ip(route->family, next_hop, TRUE); + route->next_hop = canonicalize_ip_binary(route->family, next_hop ? &next_hop_bin : NULL, TRUE); } /** @@ -1694,7 +1681,7 @@ nm_ip_routing_rule_unref(NMIPRoutingRule *self) g_free(self->iifname); g_free(self->oifname); - g_slice_free(NMIPRoutingRule, self); + nm_g_slice_free(self); } /** @@ -4002,25 +3989,31 @@ gboolean nm_setting_ip_config_add_dns(NMSettingIPConfig *setting, const char *dns) { NMSettingIPConfigPrivate *priv; - char *dns_canonical; + int addr_family; + NMIPAddr dns_bin; + char dns_canonical[NM_UTILS_INET_ADDRSTRLEN]; guint i; g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), FALSE); - g_return_val_if_fail(dns != NULL, FALSE); - g_return_val_if_fail(nm_utils_ipaddr_is_valid(NM_SETTING_IP_CONFIG_GET_FAMILY(setting), dns), - FALSE); + + addr_family = NM_SETTING_IP_CONFIG_GET_FAMILY(setting); + + if (!valid_ip(addr_family, dns, &dns_bin, NULL)) { + g_return_val_if_fail(dns != NULL, FALSE); + g_return_val_if_fail(nm_utils_ipaddr_is_valid(addr_family, dns), FALSE); + nm_assert_not_reached(); + } priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - dns_canonical = canonicalize_ip(NM_SETTING_IP_CONFIG_GET_FAMILY(setting), dns, FALSE); + nm_utils_inet_ntop(addr_family, &dns_bin, dns_canonical); + for (i = 0; i < priv->dns->len; i++) { - if (!strcmp(dns_canonical, priv->dns->pdata[i])) { - g_free(dns_canonical); + if (nm_streq(dns_canonical, priv->dns->pdata[i])) return FALSE; - } } - g_ptr_array_add(priv->dns, dns_canonical); + g_ptr_array_add(priv->dns, g_strdup(dns_canonical)); _notify(setting, PROP_DNS); return TRUE; } @@ -4059,26 +4052,32 @@ gboolean nm_setting_ip_config_remove_dns_by_value(NMSettingIPConfig *setting, const char *dns) { NMSettingIPConfigPrivate *priv; - char *dns_canonical; + int addr_family; + NMIPAddr dns_bin; + char dns_canonical[NM_UTILS_INET_ADDRSTRLEN]; guint i; g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), FALSE); - g_return_val_if_fail(dns != NULL, FALSE); - g_return_val_if_fail(nm_utils_ipaddr_is_valid(NM_SETTING_IP_CONFIG_GET_FAMILY(setting), dns), - FALSE); + + addr_family = NM_SETTING_IP_CONFIG_GET_FAMILY(setting); + + if (!valid_ip(addr_family, dns, &dns_bin, NULL)) { + g_return_val_if_fail(dns != NULL, FALSE); + g_return_val_if_fail(nm_utils_ipaddr_is_valid(addr_family, dns), FALSE); + nm_assert_not_reached(); + } priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - dns_canonical = canonicalize_ip(NM_SETTING_IP_CONFIG_GET_FAMILY(setting), dns, FALSE); + nm_utils_inet_ntop(addr_family, &dns_bin, dns_canonical); + for (i = 0; i < priv->dns->len; i++) { - if (!strcmp(dns_canonical, priv->dns->pdata[i])) { + if (nm_streq(dns_canonical, priv->dns->pdata[i])) { g_ptr_array_remove_index(priv->dns, i); _notify(setting, PROP_DNS); - g_free(dns_canonical); return TRUE; } } - g_free(dns_canonical); return FALSE; } From 6f277d8fa6375253774c1bee0d6eef962036734b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 17:09:53 +0100 Subject: [PATCH 04/33] libnm: change NMVariantAttributeSpec.str_type to work for attributes of any type First of all, all of NMVariantAttributeSpec is internal API. We only expose the typedef itself as public API, but not its fields nor their meaning. So we can change things. Change "str_type" to "type_detail", so that it can work for any kind of attribute, not only for strings. Usually, we want to avoid special cases and treat all attributes the same, based on their GVariant type. But sometimes, it is necessary to do something special with an attribute. This is what the "type_detail" encodes, but it's not only relevant for strings. --- src/libnm-core-impl/nm-setting-ip-config.c | 122 +++++++++++---------- src/libnm-core-impl/nm-setting-sriov.c | 40 +++---- src/libnm-glib-aux/nm-shared-utils.h | 8 +- 3 files changed, 90 insertions(+), 80 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 9e42bc7d81..88fbe925e9 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -1203,8 +1203,8 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_FROM, G_VARIANT_TYPE_STRING, - .v6 = TRUE, - .str_type = 'p', ), + .v6 = TRUE, + .type_detail = 'p', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_INITCWND, G_VARIANT_TYPE_UINT32, .v4 = TRUE, @@ -1246,9 +1246,9 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { .v4 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_SRC, G_VARIANT_TYPE_STRING, - .v4 = TRUE, - .v6 = TRUE, - .str_type = 'a', ), + .v4 = TRUE, + .v6 = TRUE, + .type_detail = 'a', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_TABLE, G_VARIANT_TYPE_UINT32, .v4 = TRUE, @@ -1256,9 +1256,9 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_TOS, G_VARIANT_TYPE_BYTE, .v4 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_TYPE, G_VARIANT_TYPE_STRING, - .v4 = TRUE, - .v6 = TRUE, - .str_type = 'T', ), + .v4 = TRUE, + .v6 = TRUE, + .type_detail = 'T', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_WINDOW, G_VARIANT_TYPE_UINT32, .v4 = TRUE, @@ -1302,6 +1302,7 @@ nm_ip_route_attribute_validate(const char *name, GError **error) { const NMVariantAttributeSpec *spec; + const char *string; g_return_val_if_fail(name, FALSE); g_return_val_if_fail(value, FALSE); @@ -1340,65 +1341,68 @@ nm_ip_route_attribute_validate(const char *name, return FALSE; } - if (g_variant_type_equal(spec->type, G_VARIANT_TYPE_STRING)) { - const char *string = g_variant_get_string(value, NULL); + switch (spec->type_detail) { + case 'a': /* IP address */ + string = g_variant_get_string(value, NULL); + if (!nm_utils_ipaddr_is_valid(family, string)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_FAILED, + family == AF_INET ? _("'%s' is not a valid IPv4 address") + : _("'%s' is not a valid IPv6 address"), + string); + return FALSE; + } + break; + case 'p': /* IP address + optional prefix */ + { + gs_free char *addr_free = NULL; + const char *addr; + const char *str; - switch (spec->str_type) { - case 'a': /* IP address */ - if (!nm_utils_ipaddr_is_valid(family, string)) { + string = g_variant_get_string(value, NULL); + addr = string; + + str = strchr(addr, '/'); + if (str) { + addr = nm_strndup_a(200, addr, str - addr, &addr_free); + str++; + if (_nm_utils_ascii_str_to_int64(str, 10, 0, family == AF_INET ? 32 : 128, -1) < 0) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_FAILED, - family == AF_INET ? _("'%s' is not a valid IPv4 address") - : _("'%s' is not a valid IPv6 address"), - string); + _("invalid prefix %s"), + str); return FALSE; } - break; - case 'p': /* IP address + optional prefix */ - { - gs_free char *addr_free = NULL; - const char *addr = string; - const char *str; - - str = strchr(addr, '/'); - if (str) { - addr = nm_strndup_a(200, addr, str - addr, &addr_free); - str++; - if (_nm_utils_ascii_str_to_int64(str, 10, 0, family == AF_INET ? 32 : 128, -1) - < 0) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_FAILED, - _("invalid prefix %s"), - str); - return FALSE; - } - } - if (!nm_utils_ipaddr_is_valid(family, addr)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_FAILED, - family == AF_INET ? _("'%s' is not a valid IPv4 address") - : _("'%s' is not a valid IPv6 address"), - string); - return FALSE; - } - break; } - case 'T': /* route type. */ - if (!NM_IN_SET(nm_net_aux_rtnl_rtntype_a2n(string), RTN_UNICAST, RTN_LOCAL)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("%s is not a valid route type"), - string); - return FALSE; - } - break; - default: - break; + if (!nm_utils_ipaddr_is_valid(family, addr)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_FAILED, + family == AF_INET ? _("'%s' is not a valid IPv4 address") + : _("'%s' is not a valid IPv6 address"), + string); + return FALSE; } + break; + } + case 'T': /* route type. */ + string = g_variant_get_string(value, NULL); + if (!NM_IN_SET(nm_net_aux_rtnl_rtntype_a2n(string), RTN_UNICAST, RTN_LOCAL)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("%s is not a valid route type"), + string); + return FALSE; + } + break; + case '\0': + break; + default: + nm_assert_not_reached(); + break; } return TRUE; diff --git a/src/libnm-core-impl/nm-setting-sriov.c b/src/libnm-core-impl/nm-setting-sriov.c index 3875992d08..30dd8f541b 100644 --- a/src/libnm-core-impl/nm-setting-sriov.c +++ b/src/libnm-core-impl/nm-setting-sriov.c @@ -350,13 +350,13 @@ nm_sriov_vf_get_attribute(const NMSriovVF *vf, const char *name) const NMVariantAttributeSpec *const _nm_sriov_vf_attribute_spec[] = { NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_SRIOV_VF_ATTRIBUTE_MAC, G_VARIANT_TYPE_STRING, - .str_type = 'm', ), + .type_detail = 'm', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_SRIOV_VF_ATTRIBUTE_SPOOF_CHECK, G_VARIANT_TYPE_BOOLEAN, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_SRIOV_VF_ATTRIBUTE_TRUST, G_VARIANT_TYPE_BOOLEAN, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_SRIOV_VF_ATTRIBUTE_MIN_TX_RATE, G_VARIANT_TYPE_UINT32, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_SRIOV_VF_ATTRIBUTE_MAX_TX_RATE, G_VARIANT_TYPE_UINT32, ), /* D-Bus only, synthetic attributes */ - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE("vlans", G_VARIANT_TYPE_STRING, .str_type = 'd', ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE("vlans", G_VARIANT_TYPE_STRING, .type_detail = 'd', ), NULL, }; @@ -379,6 +379,7 @@ nm_sriov_vf_attribute_validate(const char *name, GVariant *value, gboolean *know { const NMVariantAttributeSpec *const *iter; const NMVariantAttributeSpec *spec = NULL; + const char *string; g_return_val_if_fail(name, FALSE); g_return_val_if_fail(value, FALSE); @@ -391,7 +392,7 @@ nm_sriov_vf_attribute_validate(const char *name, GVariant *value, gboolean *know } } - if (!spec || spec->str_type == 'd') { + if (!spec || spec->type_detail == 'd') { NM_SET_OUT(known, FALSE); g_set_error_literal(error, NM_CONNECTION_ERROR, @@ -411,24 +412,23 @@ nm_sriov_vf_attribute_validate(const char *name, GVariant *value, gboolean *know return FALSE; } - if (g_variant_type_equal(spec->type, G_VARIANT_TYPE_STRING)) { - const char *string; - - switch (spec->str_type) { - case 'm': /* MAC address */ - string = g_variant_get_string(value, NULL); - if (!nm_utils_hwaddr_valid(string, -1)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_FAILED, - _("'%s' is not a valid MAC address"), - string); - return FALSE; - } - break; - default: - break; + switch (spec->type_detail) { + case 'm': /* MAC address */ + string = g_variant_get_string(value, NULL); + if (!nm_utils_hwaddr_valid(string, -1)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_FAILED, + _("'%s' is not a valid MAC address"), + string); + return FALSE; } + break; + case '\0': + break; + default: + nm_assert_not_reached(); + break; } return TRUE; diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 5a0518f83e..54550f2737 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -3148,7 +3148,13 @@ struct _NMVariantAttributeSpec { bool v6 : 1; bool no_value : 1; bool consumes_rest : 1; - char str_type; + + /* This indicates a non-standard parsing behavior. What this is, + * depends on the actual validation and how to handle it. + * + * Note that the entire NMVariantAttributeSpec is internal API, + * so we can change behavior and adjust it as it fits. */ + char type_detail; }; typedef struct _NMVariantAttributeSpec NMVariantAttributeSpec; From 0413b1bf8ae6749af10ac64c34f77c37b7d76050 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 16:56:05 +0100 Subject: [PATCH 05/33] libnm: rework validating route attributes to avoid duplicate check _nm_ip_route_attribute_validate_all() validates all attributes together. As such, it calls to nm_ip_route_attribute_validate(), which in turn validates one attribute at a time. Such full validation needs to check that (potentially conflicting) attributes are valid together. Hence, _nm_ip_route_attribute_validate_all() needs again peek into the attributes. Refactor the code, so that we can extract the pieces that we need and not need to parse them twice. --- src/libnm-core-impl/nm-setting-ip-config.c | 134 +++++++++++++-------- 1 file changed, 81 insertions(+), 53 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 88fbe925e9..3cc0486e2a 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -1243,7 +1243,8 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_SCOPE, G_VARIANT_TYPE_BYTE, - .v4 = TRUE, ), + .v4 = TRUE, + .type_detail = 's'), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE(NM_IP_ROUTE_ATTRIBUTE_SRC, G_VARIANT_TYPE_STRING, .v4 = TRUE, @@ -1279,35 +1280,26 @@ nm_ip_route_get_variant_attribute_spec(void) return ip_route_attribute_spec; } -/** - * nm_ip_route_attribute_validate: - * @name: the attribute name - * @value: the attribute value - * @family: IP address family of the route - * @known: (out): on return, whether the attribute name is a known one - * @error: (allow-none): return location for a #GError, or %NULL - * - * Validates a route attribute, i.e. checks that the attribute is a known one - * and the value is of the correct type and well-formed. - * - * Returns: %TRUE if the attribute is valid, %FALSE otherwise - * - * Since: 1.8 - */ -gboolean -nm_ip_route_attribute_validate(const char *name, - GVariant *value, - int family, - gboolean *known, - GError **error) +typedef struct { + int type; + int scope; +} IPRouteAttrParseData; + +static gboolean +_ip_route_attribute_validate(const char *name, + GVariant *value, + int family, + IPRouteAttrParseData *parse_data, + gboolean *known, + GError **error) { const NMVariantAttributeSpec *spec; const char *string; - g_return_val_if_fail(name, FALSE); - g_return_val_if_fail(value, FALSE); - g_return_val_if_fail(family == AF_INET || family == AF_INET6, FALSE); - g_return_val_if_fail(!error || !*error, FALSE); + nm_assert(name); + nm_assert(value); + nm_assert(family == AF_INET || family == AF_INET6); + nm_assert(!error || !*error); spec = _nm_variant_attribute_spec_find_binary_search(ip_route_attribute_spec, G_N_ELEMENTS(ip_route_attribute_spec) - 1, @@ -1388,8 +1380,12 @@ nm_ip_route_attribute_validate(const char *name, break; } case 'T': /* route type. */ + { + int type; + string = g_variant_get_string(value, NULL); - if (!NM_IN_SET(nm_net_aux_rtnl_rtntype_a2n(string), RTN_UNICAST, RTN_LOCAL)) { + type = nm_net_aux_rtnl_rtntype_a2n(string); + if (!NM_IN_SET(type, RTN_UNICAST, RTN_LOCAL)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -1397,6 +1393,14 @@ nm_ip_route_attribute_validate(const char *name, string); return FALSE; } + + if (parse_data) + parse_data->type = type; + break; + } + case 's': /* scope */ + if (parse_data) + parse_data->scope = g_variant_get_byte(value); break; case '\0': break; @@ -1408,6 +1412,36 @@ nm_ip_route_attribute_validate(const char *name, return TRUE; } +/** + * nm_ip_route_attribute_validate: + * @name: the attribute name + * @value: the attribute value + * @family: IP address family of the route + * @known: (out): on return, whether the attribute name is a known one + * @error: (allow-none): return location for a #GError, or %NULL + * + * Validates a route attribute, i.e. checks that the attribute is a known one + * and the value is of the correct type and well-formed. + * + * Returns: %TRUE if the attribute is valid, %FALSE otherwise + * + * Since: 1.8 + */ +gboolean +nm_ip_route_attribute_validate(const char *name, + GVariant *value, + int family, + gboolean *known, + GError **error) +{ + g_return_val_if_fail(name, FALSE); + g_return_val_if_fail(value, FALSE); + g_return_val_if_fail(family == AF_INET || family == AF_INET6, FALSE); + g_return_val_if_fail(!error || !*error, FALSE); + + return _ip_route_attribute_validate(name, value, family, NULL, known, error); +} + gboolean _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error) { @@ -1415,9 +1449,11 @@ _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error) gs_free NMUtilsNamedValue *attrs_free = NULL; const NMUtilsNamedValue *attrs; guint attrs_len; - GVariant *val; guint i; - guint8 u8; + IPRouteAttrParseData parse_data = { + .type = RTN_UNICAST, + .scope = -1, + }; g_return_val_if_fail(route, FALSE); g_return_val_if_fail(!error || !*error, FALSE); @@ -1430,34 +1466,26 @@ _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error) attrs_static, &attrs_free); for (i = 0; i < attrs_len; i++) { - const char *key = attrs[i].name; - GVariant *val2 = attrs[i].value_ptr; - - if (!nm_ip_route_attribute_validate(key, val2, route->family, NULL, error)) + if (!_ip_route_attribute_validate(attrs[i].name, + attrs[i].value_ptr, + route->family, + &parse_data, + NULL, + error)) return FALSE; } - if ((val = g_hash_table_lookup(route->attributes, NM_IP_ROUTE_ATTRIBUTE_TYPE))) { - int v_i; - - nm_assert(g_variant_is_of_type(val, G_VARIANT_TYPE_STRING)); - - v_i = nm_net_aux_rtnl_rtntype_a2n(g_variant_get_string(val, NULL)); - nm_assert(v_i >= 0); - - if (v_i == RTN_LOCAL && route->family == AF_INET - && (val = g_hash_table_lookup(route->attributes, NM_IP_ROUTE_ATTRIBUTE_SCOPE))) { - nm_assert(g_variant_is_of_type(val, G_VARIANT_TYPE_BYTE)); - u8 = g_variant_get_byte(val); - - if (!NM_IN_SET(u8, RT_SCOPE_HOST, RT_SCOPE_NOWHERE)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("route scope is invalid")); - return FALSE; - } + switch (parse_data.type) { + case RTN_LOCAL: + if (route->family == AF_INET && parse_data.scope >= 0 + && !NM_IN_SET(parse_data.scope, RT_SCOPE_HOST, RT_SCOPE_NOWHERE)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("route scope is invalid for local route")); + return FALSE; } + break; } return TRUE; From 8085c0121f9a80eec55c575b703bf322a56da973 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 17:57:14 +0100 Subject: [PATCH 06/33] platform: rename variable "IS_IPv4" in platform code The variable with this purpose is usually called "IS_IPv4". It's upper case, because usually this is a const variable, and because it reminds of the NM_IS_IPv4(addr_family) macro. That letter case is unusual, but it makes sense to me for the special purpose that this variable has. Anyway. The naming of this variable is a different point. Let's use the variable name that is consistent and widely used. --- src/libnm-platform/nm-linux-platform.c | 83 ++++++++++++++------------ 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 83f2207d37..411a49d599 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3268,7 +3268,7 @@ _new_from_nl_addr(struct nlmsghdr *nlh, gboolean id_only) }; struct nlattr *tb[G_N_ELEMENTS(policy)]; const struct ifaddrmsg *ifa; - gboolean is_v4; + gboolean IS_IPv4; nm_auto_nmpobj NMPObject *obj = NULL; int addr_len; guint32 lifetime, preferred, timestamp; @@ -3278,29 +3278,31 @@ _new_from_nl_addr(struct nlmsghdr *nlh, gboolean id_only) ifa = nlmsg_data(nlh); - if (!NM_IN_SET(ifa->ifa_family, AF_INET, AF_INET6)) + if (ifa->ifa_family == AF_INET) + IS_IPv4 = TRUE; + else if (ifa->ifa_family == AF_INET6) + IS_IPv4 = FALSE; + else return NULL; - is_v4 = ifa->ifa_family == AF_INET; - if (nlmsg_parse_arr(nlh, sizeof(*ifa), tb, policy) < 0) return NULL; - addr_len = is_v4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); + addr_len = IS_IPv4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); - if (ifa->ifa_prefixlen > (is_v4 ? 32 : 128)) + if (ifa->ifa_prefixlen > (IS_IPv4 ? 32 : 128)) return NULL; /*****************************************************************/ - obj = nmp_object_new(is_v4 ? NMP_OBJECT_TYPE_IP4_ADDRESS : NMP_OBJECT_TYPE_IP6_ADDRESS, NULL); + obj = nmp_object_new(IS_IPv4 ? NMP_OBJECT_TYPE_IP4_ADDRESS : NMP_OBJECT_TYPE_IP6_ADDRESS, NULL); obj->ip_address.ifindex = ifa->ifa_index; obj->ip_address.plen = ifa->ifa_prefixlen; _check_addr_or_return_null(tb, IFA_ADDRESS, addr_len); _check_addr_or_return_null(tb, IFA_LOCAL, addr_len); - if (is_v4) { + if (IS_IPv4) { /* For IPv4, kernel omits IFA_LOCAL/IFA_ADDRESS if (and only if) they * are effectively 0.0.0.0 (all-zero). */ if (tb[IFA_LOCAL]) @@ -3336,7 +3338,7 @@ _new_from_nl_addr(struct nlmsghdr *nlh, gboolean id_only) obj->ip_address.n_ifa_flags = tb[IFA_FLAGS] ? nla_get_u32(tb[IFA_FLAGS]) : ifa->ifa_flags; - if (is_v4) { + if (IS_IPv4) { if (tb[IFA_LABEL]) { char label[IFNAMSIZ]; @@ -3387,7 +3389,7 @@ _new_from_nl_route(struct nlmsghdr *nlh, gboolean id_only) }; const struct rtmsg *rtm; struct nlattr *tb[G_N_ELEMENTS(policy)]; - gboolean is_v4; + gboolean IS_IPv4; nm_auto_nmpobj NMPObject *obj = NULL; int addr_len; struct { @@ -3414,7 +3416,11 @@ _new_from_nl_route(struct nlmsghdr *nlh, gboolean id_only) * only handle ~supported~ routes. *****************************************************************/ - if (!NM_IN_SET(rtm->rtm_family, AF_INET, AF_INET6)) + if (rtm->rtm_family == AF_INET) + IS_IPv4 = TRUE; + else if (rtm->rtm_family == AF_INET6) + IS_IPv4 = FALSE; + else return NULL; if (!NM_IN_SET(rtm->rtm_type, RTN_UNICAST, RTN_LOCAL)) @@ -3425,10 +3431,9 @@ _new_from_nl_route(struct nlmsghdr *nlh, gboolean id_only) /*****************************************************************/ - is_v4 = rtm->rtm_family == AF_INET; - addr_len = is_v4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); + addr_len = IS_IPv4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); - if (rtm->rtm_dst_len > (is_v4 ? 32 : 128)) + if (rtm->rtm_dst_len > (IS_IPv4 ? 32 : 128)) return NULL; /***************************************************************** @@ -3539,7 +3544,7 @@ rta_multipath_done:; /*****************************************************************/ - obj = nmp_object_new(is_v4 ? NMP_OBJECT_TYPE_IP4_ROUTE : NMP_OBJECT_TYPE_IP6_ROUTE, NULL); + obj = nmp_object_new(IS_IPv4 ? NMP_OBJECT_TYPE_IP4_ROUTE : NMP_OBJECT_TYPE_IP6_ROUTE, NULL); obj->ip_route.type_coerced = nm_platform_route_type_coerce(rtm->rtm_type); obj->ip_route.table_coerced = nm_platform_route_table_coerce( @@ -3555,22 +3560,22 @@ rta_multipath_done:; if (tb[RTA_PRIORITY]) obj->ip_route.metric = nla_get_u32(tb[RTA_PRIORITY]); - if (is_v4) + if (IS_IPv4) obj->ip4_route.gateway = nh.gateway.addr4; else obj->ip6_route.gateway = nh.gateway.addr6; - if (is_v4) + if (IS_IPv4) obj->ip4_route.scope_inv = nm_platform_route_scope_inv(rtm->rtm_scope); if (_check_addr_or_return_null(tb, RTA_PREFSRC, addr_len)) { - if (is_v4) + if (IS_IPv4) memcpy(&obj->ip4_route.pref_src, nla_data(tb[RTA_PREFSRC]), addr_len); else memcpy(&obj->ip6_route.pref_src, nla_data(tb[RTA_PREFSRC]), addr_len); } - if (is_v4) + if (IS_IPv4) obj->ip4_route.tos = rtm->rtm_tos; else { if (tb[RTA_SRC]) { @@ -3592,7 +3597,7 @@ rta_multipath_done:; obj->ip_route.lock_initrwnd = NM_FLAGS_HAS(lock, 1 << RTAX_INITRWND); obj->ip_route.lock_mtu = NM_FLAGS_HAS(lock, 1 << RTAX_MTU); - if (!is_v4) { + if (!IS_IPv4) { if (tb[RTA_PREF]) obj->ip6_route.rt_pref = nla_get_u8(tb[RTA_PREF]); } @@ -4715,23 +4720,23 @@ ip_route_ignored_protocol(const NMPlatformIPRoute *route) static struct nl_msg * _nl_msg_new_route(int nlmsg_type, guint16 nlmsgflags, const NMPObject *obj) { - nm_auto_nlmsg struct nl_msg *msg = NULL; - const NMPClass *klass = NMP_OBJECT_GET_CLASS(obj); - gboolean is_v4 = klass->addr_family == AF_INET; - const guint32 lock = ip_route_get_lock_flag(NMP_OBJECT_CAST_IP_ROUTE(obj)); + nm_auto_nlmsg struct nl_msg *msg = NULL; + const NMPClass *klass = NMP_OBJECT_GET_CLASS(obj); + const gboolean IS_IPv4 = NM_IS_IPv4(klass->addr_family); + const guint32 lock = ip_route_get_lock_flag(NMP_OBJECT_CAST_IP_ROUTE(obj)); const guint32 table = nm_platform_route_table_uncoerce(NMP_OBJECT_CAST_IP_ROUTE(obj)->table_coerced, TRUE); const struct rtmsg rtmsg = { .rtm_family = klass->addr_family, - .rtm_tos = is_v4 ? obj->ip4_route.tos : 0, + .rtm_tos = IS_IPv4 ? obj->ip4_route.tos : 0, .rtm_table = table <= 0xFF ? table : RT_TABLE_UNSPEC, .rtm_protocol = nmp_utils_ip_config_source_coerce_to_rtprot(obj->ip_route.rt_source), .rtm_scope = - is_v4 ? nm_platform_route_scope_inv(obj->ip4_route.scope_inv) : RT_SCOPE_NOWHERE, + IS_IPv4 ? nm_platform_route_scope_inv(obj->ip4_route.scope_inv) : RT_SCOPE_NOWHERE, .rtm_type = nm_platform_route_type_uncoerce(NMP_OBJECT_CAST_IP_ROUTE(obj)->type_coerced), .rtm_flags = obj->ip_route.r_rtm_flags & ((unsigned) (RTNH_F_ONLINK)), .rtm_dst_len = obj->ip_route.plen, - .rtm_src_len = is_v4 ? 0 : NMP_OBJECT_CAST_IP6_ROUTE(obj)->src_plen, + .rtm_src_len = IS_IPv4 ? 0 : NMP_OBJECT_CAST_IP6_ROUTE(obj)->src_plen, }; gsize addr_len; @@ -4745,28 +4750,28 @@ _nl_msg_new_route(int nlmsg_type, guint16 nlmsgflags, const NMPObject *obj) if (nlmsg_append_struct(msg, &rtmsg) < 0) goto nla_put_failure; - addr_len = is_v4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); + addr_len = IS_IPv4 ? sizeof(in_addr_t) : sizeof(struct in6_addr); NLA_PUT(msg, RTA_DST, addr_len, - is_v4 ? (gconstpointer) &obj->ip4_route.network - : (gconstpointer) &obj->ip6_route.network); + IS_IPv4 ? (gconstpointer) &obj->ip4_route.network + : (gconstpointer) &obj->ip6_route.network); - if (!is_v4) { + if (!IS_IPv4) { if (!IN6_IS_ADDR_UNSPECIFIED(&NMP_OBJECT_CAST_IP6_ROUTE(obj)->src)) NLA_PUT(msg, RTA_SRC, addr_len, &obj->ip6_route.src); } NLA_PUT_U32(msg, RTA_PRIORITY, - is_v4 ? nm_platform_ip4_route_get_effective_metric(&obj->ip4_route) - : nm_platform_ip6_route_get_effective_metric(&obj->ip6_route)); + IS_IPv4 ? nm_platform_ip4_route_get_effective_metric(&obj->ip4_route) + : nm_platform_ip6_route_get_effective_metric(&obj->ip6_route)); if (table > 0xFF) NLA_PUT_U32(msg, RTA_TABLE, table); - if (is_v4) { + if (IS_IPv4) { if (NMP_OBJECT_CAST_IP4_ROUTE(obj)->pref_src) NLA_PUT(msg, RTA_PREFSRC, addr_len, &obj->ip4_route.pref_src); } else { @@ -4801,7 +4806,7 @@ _nl_msg_new_route(int nlmsg_type, guint16 nlmsgflags, const NMPObject *obj) } /* We currently don't have need for multi-hop routes... */ - if (is_v4) { + if (IS_IPv4) { NLA_PUT(msg, RTA_GATEWAY, addr_len, &obj->ip4_route.gateway); } else { if (!IN6_IS_ADDR_UNSPECIFIED(&obj->ip6_route.gateway)) @@ -4809,7 +4814,7 @@ _nl_msg_new_route(int nlmsg_type, guint16 nlmsgflags, const NMPObject *obj) } NLA_PUT_U32(msg, RTA_OIF, obj->ip_route.ifindex); - if (!is_v4 && obj->ip6_route.rt_pref != NM_ICMPV6_ROUTER_PREF_MEDIUM) + if (!IS_IPv4 && obj->ip6_route.rt_pref != NM_ICMPV6_ROUTER_PREF_MEDIUM) NLA_PUT_U8(msg, RTA_PREF, obj->ip6_route.rt_pref); return g_steal_pointer(&msg); @@ -8736,8 +8741,8 @@ ip_route_get(NMPlatform *platform, int oif_ifindex, NMPObject **out_route) { - const gboolean is_v4 = (addr_family == AF_INET); - const int addr_len = is_v4 ? 4 : 16; + const gboolean IS_IPv4 = NM_IS_IPv4(addr_family); + const int addr_len = IS_IPv4 ? 4 : 16; int try_count = 0; WaitForNlResponseResult seq_result; int nle; @@ -8758,7 +8763,7 @@ ip_route_get(NMPlatform *platform, .n.nlmsg_type = RTM_GETROUTE, .r.rtm_family = addr_family, .r.rtm_tos = 0, - .r.rtm_dst_len = is_v4 ? 32 : 128, + .r.rtm_dst_len = IS_IPv4 ? 32 : 128, .r.rtm_flags = 0x1000 /* RTM_F_LOOKUP_TABLE */, }; From 596d1645e839e5b6df05cea759d9022669917a89 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 22:52:55 +0100 Subject: [PATCH 07/33] core: use IS_IPv4 variable in nm_utils_ip_route_attribute_to_platform() It's what we do at many other places. Consistency. --- src/core/NetworkManagerUtils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 0d08efbb9c..14f9b453c8 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1316,6 +1316,7 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, NMPlatformIPRoute *r, gint64 route_table) { + const int IS_IPv4 = NM_IS_IPv4(addr_family); GVariant *variant; guint32 table; NMIPAddr addr; @@ -1324,7 +1325,7 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, gboolean onlink; nm_assert(s_route); - nm_assert_addr_family(addr_family); + nm_assert(addr_family == nm_ip_route_get_family(s_route)); nm_assert(r); nm_assert(route_table >= -1); nm_assert(route_table <= (gint64) G_MAXUINT32); @@ -1365,7 +1366,7 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, else r->table_any = TRUE; - if (NM_IS_IPv4(addr_family)) { + if (IS_IPv4) { guint8 scope; GET_ATTR(NM_IP_ROUTE_ATTRIBUTE_TOS, r4->tos, BYTE, byte, 0); @@ -1391,15 +1392,14 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, if ((variant = nm_ip_route_get_attribute(s_route, NM_IP_ROUTE_ATTRIBUTE_SRC)) && g_variant_is_of_type(variant, G_VARIANT_TYPE_STRING)) { if (inet_pton(addr_family, g_variant_get_string(variant, NULL), &addr) == 1) { - if (NM_IS_IPv4(addr_family)) + if (IS_IPv4) r4->pref_src = addr.addr4; else r6->pref_src = addr.addr6; } } - if (!NM_IS_IPv4(addr_family) - && (variant = nm_ip_route_get_attribute(s_route, NM_IP_ROUTE_ATTRIBUTE_FROM)) + if (!IS_IPv4 && (variant = nm_ip_route_get_attribute(s_route, NM_IP_ROUTE_ATTRIBUTE_FROM)) && g_variant_is_of_type(variant, G_VARIANT_TYPE_STRING)) { int prefix; From b58711f20d91393ae80eef8f9716ba69b24cf2c2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 08:15:45 +0100 Subject: [PATCH 08/33] platform: don't print NUL gateway in nm_platform_ip[46]_route_to_string() Currently, for NMPlatformIP[46]Route always has a gateway, even if it's possibly set to 0.0.0.0/::. Not sure whether kernel has a further distinction between no-gateway and all-zero gateway. Anyway. For us, a gateway of 0.0.0.0/:: means the same as having no gateway. We cannot differentiate the two (nor do we need to). Don't print that in nm_platform_ip[46]_route_to_string(). Also, because we are going to add blackhole route types, which cannot have a next-hop. But we do this change for all routes types, because it makes sense in general (and also what `ip route show` prints). --- src/libnm-platform/nm-platform.c | 84 ++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index a4863c0d2d..b95cd95ec9 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -3143,7 +3143,8 @@ nm_platform_wpan_set_channel(NMPlatform *self, int ifindex, guint8 page, guint8 static const char * _to_string_dev(NMPlatform *self, int ifindex, char *buf, size_t size) { - g_assert(buf && size >= TO_STRING_DEV_BUF_SIZE); + nm_assert(buf); + nm_assert(size >= TO_STRING_DEV_BUF_SIZE); if (ifindex) { const char *name = ifindex > 0 && self ? nm_platform_link_get_name(self, ifindex) : NULL; @@ -6458,12 +6459,19 @@ _rtm_flags_to_string_full(char *buf, gsize buf_size, unsigned rtm_flags) const char * nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsize len) { - char s_network[INET_ADDRSTRLEN], s_gateway[INET_ADDRSTRLEN]; + char s_network[INET_ADDRSTRLEN]; + char s_gateway[INET_ADDRSTRLEN]; char s_pref_src[INET_ADDRSTRLEN]; char str_dev[TO_STRING_DEV_BUF_SIZE]; char str_table[30]; - char str_scope[30], s_source[50]; - char str_tos[32], str_window[32], str_cwnd[32], str_initcwnd[32], str_initrwnd[32], str_mtu[32]; + char str_scope[30]; + char s_source[50]; + char str_tos[32]; + char str_window[32]; + char str_cwnd[32]; + char str_initcwnd[32]; + char str_initrwnd[32]; + char str_mtu[32]; char str_rtm_flags[_RTM_FLAGS_TO_STRING_MAXLEN]; char str_type[30]; char str_metric[30]; @@ -6472,7 +6480,11 @@ nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsiz return buf; inet_ntop(AF_INET, &route->network, s_network, sizeof(s_network)); - inet_ntop(AF_INET, &route->gateway, s_gateway, sizeof(s_gateway)); + + if (route->gateway == 0) + s_gateway[0] = '\0'; + else + inet_ntop(AF_INET, &route->gateway, s_gateway, sizeof(s_gateway)); _to_string_dev(NULL, route->ifindex, str_dev, sizeof(str_dev)); @@ -6482,21 +6494,22 @@ nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsiz "type %s " /* type */ "%s" /* table */ "%s/%d" - " via %s" + "%s%s" /* gateway */ "%s" " metric %s" - " mss %" G_GUINT32_FORMAT " rt-src %s" /* protocol */ - "%s" /* rtm_flags */ - "%s%s" /* scope */ - "%s%s" /* pref-src */ - "%s" /* tos */ - "%s" /* window */ - "%s" /* cwnd */ - "%s" /* initcwnd */ - "%s" /* initrwnd */ - "%s" /* mtu */ - "%s" /* r_assume_config_once */ - "%s" /* r_force_commit */ + " mss %" G_GUINT32_FORMAT /* mss */ + " rt-src %s" /* protocol */ + "%s" /* rtm_flags */ + "%s%s" /* scope */ + "%s%s" /* pref-src */ + "%s" /* tos */ + "%s" /* window */ + "%s" /* cwnd */ + "%s" /* initcwnd */ + "%s" /* initrwnd */ + "%s" /* mtu */ + "%s" /* r_assume_config_once */ + "%s" /* r_force_commit */ "", nm_net_aux_rtnl_rtntype_n2a_maybe_buf(nm_platform_route_type_uncoerce(route->type_coerced), str_type), @@ -6509,6 +6522,7 @@ nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsiz : ""), s_network, route->plen, + s_gateway[0] ? " via " : "", s_gateway, str_dev, route->metric_any @@ -6596,7 +6610,11 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz return buf; inet_ntop(AF_INET6, &route->network, s_network, sizeof(s_network)); - inet_ntop(AF_INET6, &route->gateway, s_gateway, sizeof(s_gateway)); + + if (IN6_IS_ADDR_UNSPECIFIED(&route->gateway)) + s_gateway[0] = '\0'; + else + inet_ntop(AF_INET6, &route->gateway, s_gateway, sizeof(s_gateway)); if (IN6_IS_ADDR_UNSPECIFIED(&route->pref_src)) s_pref_src[0] = 0; @@ -6611,21 +6629,22 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz "type %s " /* type */ "%s" /* table */ "%s/%d" - " via %s" + "%s%s" /* gateway */ "%s" " metric %s" - " mss %" G_GUINT32_FORMAT " rt-src %s" /* protocol */ - "%s" /* source */ - "%s" /* rtm_flags */ - "%s%s" /* pref-src */ - "%s" /* window */ - "%s" /* cwnd */ - "%s" /* initcwnd */ - "%s" /* initrwnd */ - "%s" /* mtu */ - "%s" /* pref */ - "%s" /* r_assume_config_once */ - "%s" /* r_force_commit */ + " mss %" G_GUINT32_FORMAT /* mss */ + " rt-src %s" /* protocol */ + "%s" /* source */ + "%s" /* rtm_flags */ + "%s%s" /* pref-src */ + "%s" /* window */ + "%s" /* cwnd */ + "%s" /* initcwnd */ + "%s" /* initrwnd */ + "%s" /* mtu */ + "%s" /* pref */ + "%s" /* r_assume_config_once */ + "%s" /* r_force_commit */ "", nm_net_aux_rtnl_rtntype_n2a_maybe_buf(nm_platform_route_type_uncoerce(route->type_coerced), str_type), @@ -6638,6 +6657,7 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz : ""), s_network, route->plen, + s_gateway[0] ? " via " : "", s_gateway, str_dev, route->metric_any From 1123d3a5fb3b1f1a36809d1ea55e1c21c99c35c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 15:31:12 +0100 Subject: [PATCH 09/33] platform: don't check for valid ifindex in _vt_cmd_obj_is_alive_ipx_route() _vt_cmd_obj_is_alive_ipx_route() is called by nmp_object_is_alive(). Non-alive objects are not put into the cache. That certainly makes sense for RTM_F_CLONED routes, because they are generated ad-hoc during the `ip route get` request. Checking for the ifindex is not necessary. For one, some route types (blackhole, unreachable, prohibit) don't have an ifindex. Also, the purpose of _vt_cmd_obj_is_alive_ipx_route() is not to validate the object. Just don't create objects without an ifindex, if you think the route needs an ifindex. Checking here is not useful. We also don't check that other fields like rt_source are valid, so there is no need to do it for the ifindex either. --- src/libnm-platform/nmp-object.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index ef94942e22..3154a23e80 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -1740,8 +1740,7 @@ _vt_cmd_obj_is_alive_ipx_route(const NMPObject *obj) * Instead we create a dead object, and nmp_cache_update_netlink() * will remove the old version of the update. **/ - return NMP_OBJECT_CAST_IP_ROUTE(obj)->ifindex > 0 - && !NM_FLAGS_HAS(obj->ip_route.r_rtm_flags, RTM_F_CLONED); + return !NM_FLAGS_HAS(NMP_OBJECT_CAST_IP_ROUTE(obj)->r_rtm_flags, RTM_F_CLONED); } static gboolean From d4ad9666bd4b94c1186bb2dc3f81e4efa5d0e94c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 21:17:48 +0100 Subject: [PATCH 10/33] platform: don't treat ifindex zero special in nmp_lookup_init_object() So far, certain NMObject types could not have an ifindex of zero. Hence, nmp_lookup_init_object() took such an ifindex to mean lookup all objects of that type. Soon, we will support blackhole/unreachable/prohibit route types, which have their ifindex set to zero. It is still useful to lookup those routes types via nmp_lookup_init_object(). Change behaviour how to interpret the ifindex. Note that this also affects various callers of nmp_lookup_init_object(). If somebody was relying on the previous behavior, it would need fixing. --- src/core/platform/nm-fake-platform.c | 11 +++++------ src/core/platform/tests/test-common.c | 6 +++++- src/libnm-platform/nmp-object.c | 10 +++++++++- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/core/platform/nm-fake-platform.c b/src/core/platform/nm-fake-platform.c index ebb6a795b9..7d8986d764 100644 --- a/src/core/platform/nm-fake-platform.c +++ b/src/core/platform/nm-fake-platform.c @@ -957,11 +957,10 @@ ipx_address_delete(NMPlatform *platform, peer_addr_i = peer_addr ? *((guint32 *) peer_addr) : 0; nmp_cache_iter_for_each (&iter, - nm_platform_lookup_object(platform, - addr_family == AF_INET - ? NMP_OBJECT_TYPE_IP4_ADDRESS - : NMP_OBJECT_TYPE_IP6_ADDRESS, - 0), + nm_platform_lookup_obj_type(platform, + addr_family == AF_INET + ? NMP_OBJECT_TYPE_IP4_ADDRESS + : NMP_OBJECT_TYPE_IP6_ADDRESS), &o) { const NMPObject *obj_old = NULL; @@ -1139,7 +1138,7 @@ ip_route_add(NMPlatform *platform, gboolean has_route_to_gw = FALSE; nmp_cache_iter_for_each (&iter, - nm_platform_lookup_object(platform, NMP_OBJECT_GET_TYPE(obj), 0), + nm_platform_lookup_obj_type(platform, NMP_OBJECT_GET_TYPE(obj)), &o) { if (addr_family == AF_INET) { const NMPlatformIP4Route *item = NMP_OBJECT_CAST_IP4_ROUTE(o); diff --git a/src/core/platform/tests/test-common.c b/src/core/platform/tests/test-common.c index 24426a5414..9053d35173 100644 --- a/src/core/platform/tests/test-common.c +++ b/src/core/platform/tests/test-common.c @@ -102,7 +102,11 @@ nmtstp_platform_ip_address_find(NMPlatform *self, int ifindex, int addr_family, nm_assert_addr_family(addr_family); nm_assert(addr); - nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4), ifindex); + if (ifindex > 0) + nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4), ifindex); + else + nmp_lookup_init_obj_type(&lookup, NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4)); + nm_platform_iter_obj_for_each (&iter, self, &lookup, &obj) { const NMPlatformIPAddress *a = NMP_OBJECT_CAST_IP_ADDRESS(obj); diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 3154a23e80..8bd43ef0e5 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -2142,7 +2142,15 @@ nmp_lookup_init_object(NMPLookup *lookup, NMPObjectType obj_type, int ifindex) NMP_OBJECT_TYPE_QDISC, NMP_OBJECT_TYPE_TFILTER)); - if (ifindex <= 0) { + if (G_UNLIKELY( + (ifindex < 0) + || (ifindex == 0 + && !NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE)))) { + /* This function used to have a fallback that meant to lookup all objects, if + * ifindex is non-positive. As routes can have a zero ifindex, that fallback is + * confusing and no longer supported. Only have this code, to catch accidental bugs + * after the API change. */ + nm_assert_not_reached(); return nmp_lookup_init_obj_type(lookup, obj_type); } From 7ad14b86f80f0dc106e1c5756fb90ce1c96a3758 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 18:50:54 +0100 Subject: [PATCH 11/33] platform: add nm_platform_route_type_is_nodev() helper --- .../platform/tests/test-platform-general.c | 26 +++++++++++++++++++ src/libnm-platform/nm-platform.h | 6 +++++ 2 files changed, 32 insertions(+) diff --git a/src/core/platform/tests/test-platform-general.c b/src/core/platform/tests/test-platform-general.c index 7597f4f13e..9629326aa3 100644 --- a/src/core/platform/tests/test-platform-general.c +++ b/src/core/platform/tests/test-platform-general.c @@ -748,6 +748,31 @@ test_platform_ip_address_pretty_sort_cmp(gconstpointer test_data) /*****************************************************************************/ +static void +test_route_type_is_nodev(void) +{ + int i; + + for (i = -1; i <= 257; i++) { + gboolean is_nodev; + + switch ((guint8) i) { + case RTN_BLACKHOLE: + case RTN_UNREACHABLE: + case RTN_PROHIBIT: + is_nodev = TRUE; + break; + default: + is_nodev = FALSE; + break; + } + + g_assert_cmpint(is_nodev, ==, nm_platform_route_type_is_nodev(i)); + } +} + +/*****************************************************************************/ + NMTST_DEFINE(); int @@ -767,6 +792,7 @@ main(int argc, char **argv) g_test_add_data_func("/general/platform_ip_address_pretty_sort_cmp/6/2", GINT_TO_POINTER(2), test_platform_ip_address_pretty_sort_cmp); + g_test_add_func("/general/test_route_type_is_nodev", test_route_type_is_nodev); return g_test_run(); } diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index e55610260d..0882f240e3 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -1408,6 +1408,12 @@ _nm_platform_link_get_inet6_addr_gen_mode(const NMPlatformLink *pllink) return _nm_platform_uint8_inv(pllink->inet6_addr_gen_mode_inv); } +static inline gboolean +nm_platform_route_type_is_nodev(guint8 type) +{ + return NM_IN_SET(type, 6 /* RTN_BLACKHOLE */, 7 /* RTN_UNREACHABLE */, 8 /* RTN_PROHIBIT */); +} + /** * nm_platform_route_type_coerce: * @table: the route type, in its original value. From 92f51c6b43ad334faca83107c025ce746eea4c0f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 1 Feb 2022 23:49:37 +0100 Subject: [PATCH 12/33] platform: add support for blackhole,unreachable,prohibit route type --- src/core/platform/tests/test-route.c | 62 ++++++++++++++++++++++++++ src/libnm-platform/nm-linux-platform.c | 44 +++++++++++++++--- src/libnm-platform/nmp-object.c | 10 +++-- 3 files changed, 108 insertions(+), 8 deletions(-) diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index 37909d55ef..4974a409cb 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -1886,6 +1886,64 @@ again: /*****************************************************************************/ +static void +test_blackhole(gconstpointer test_data) +{ + int TEST_IDX = GPOINTER_TO_INT(test_data); + const int addr_family = (TEST_IDX == 1) ? AF_INET : AF_INET6; + const int IS_IPv4 = NM_IS_IPv4(addr_family); + const NMDedupMultiHeadEntry *head_entry; + NMDedupMultiIter iter; + const NMPObject *obj; + NMPObject obj_stack; + NMPlatformIPXRoute rr = {}; + int r = -1; + int i; + + if (IS_IPv4) { + rr.r4 = (const NMPlatformIP4Route){ + .type_coerced = nmtst_rand_select(RTN_BLACKHOLE, RTN_UNREACHABLE, RTN_PROHIBIT), + }; + } else { + rr.r6 = (const NMPlatformIP6Route){ + .type_coerced = nmtst_rand_select(RTN_BLACKHOLE, RTN_UNREACHABLE, RTN_PROHIBIT), + .metric = 1000, + }; + } + + nm_platform_ip_route_normalize(addr_family, &rr.rx); + + if (IS_IPv4) + r = nm_platform_ip4_route_add(NM_PLATFORM_GET, NMP_NLM_FLAG_APPEND, &rr.r4); + else + r = nm_platform_ip6_route_add(NM_PLATFORM_GET, NMP_NLM_FLAG_APPEND, &rr.r6); + + g_assert_cmpint(r, ==, 0); + + nmp_object_stackinit(&obj_stack, NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4), &rr); + + obj = nm_platform_lookup_obj(NM_PLATFORM_GET, NMP_CACHE_ID_TYPE_OBJECT_TYPE, &obj_stack); + + _LOGT(">>> adding %s", + nmp_object_to_string(&obj_stack, NMP_OBJECT_TO_STRING_ALL, g_alloca(1000), 1000)); + _LOGT(">>> found %s", + nmp_object_to_string(obj, NMP_OBJECT_TO_STRING_ALL, g_alloca(1000), 1000)); + + g_assert(obj); + + head_entry = nm_platform_lookup_object(NM_PLATFORM_GET, NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4), 0); + g_assert(head_entry); + g_assert_cmpint(head_entry->len, ==, 1); + i = 0; + nm_dedup_multi_iter_for_each (&iter, head_entry) { + i++; + g_assert(iter.current->obj == obj); + } + g_assert_cmpint(i, ==, 1); +} + +/*****************************************************************************/ + NMTstpSetupFunc const _nmtstp_setup_platform_func = SETUP; void @@ -1923,4 +1981,8 @@ _nmtstp_setup_tests(void) add_test_func_data("/route/rule/3", test_rule, GINT_TO_POINTER(3)); add_test_func_data("/route/rule/4", test_rule, GINT_TO_POINTER(4)); } + if (nmtstp_is_root_test()) { + add_test_func_data("/route/blackhole/1", test_blackhole, GINT_TO_POINTER(1)); + add_test_func_data("/route/blackhole/2", test_blackhole, GINT_TO_POINTER(2)); + } } diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 411a49d599..93935b98d4 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3423,7 +3423,12 @@ _new_from_nl_route(struct nlmsghdr *nlh, gboolean id_only) else return NULL; - if (!NM_IN_SET(rtm->rtm_type, RTN_UNICAST, RTN_LOCAL)) + if (!NM_IN_SET(rtm->rtm_type, + RTN_UNICAST, + RTN_LOCAL, + RTN_BLACKHOLE, + RTN_UNREACHABLE, + RTN_PROHIBIT)) return NULL; if (nlmsg_parse_arr(nlh, sizeof(struct rtmsg), tb, policy) < 0) @@ -3497,16 +3502,45 @@ rta_multipath_done:; /* If no nexthops have been provided via RTA_MULTIPATH * we add it as regular nexthop to maintain backwards * compatibility */ - nh.ifindex = ifindex; - nh.gateway = gateway; + nh.ifindex = ifindex; + nh.gateway = gateway; + nh.is_present = TRUE; } else { /* Kernel supports new style nexthop configuration, * verify that it is a duplicate and ignore old-style nexthop. */ if (nh.ifindex != ifindex || memcmp(&nh.gateway, &gateway, addr_len) != 0) return NULL; } - } else if (!nh.is_present) - return NULL; + } + + if (nm_platform_route_type_is_nodev(rtm->rtm_type)) { + /* These routes are special. They don't have an device/ifindex. + * + * Well, actually, for IPv6 kernel will always say that the device is + * 1 (lo). Of course it does!! */ + if (nh.is_present) { + if (IS_IPv4) { + if (nh.ifindex != 0 || nh.gateway.addr4 != 0) { + /* we only accept kernel to notify about the ifindex/gateway, if it + * is zero. This is only to be a bit forgiving, but we really don't + * know how to handle such routes that have an ifindex. */ + return NULL; + } + } else { + if (!NM_IN_SET(nh.ifindex, 0, 1) || !IN6_IS_ADDR_UNSPECIFIED(&nh.gateway.addr6)) { + /* We allow an ifindex of 1 (will be normalized to zero). Otherwise, + * we don't expect a device/next hop. */ + return NULL; + } + nh.ifindex = 0; + } + } + } else { + if (!nh.is_present) { + /* a "normal" route needs a device. This is not the route we are looking for. */ + return NULL; + } + } /*****************************************************************/ diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 8bd43ef0e5..d518e6e589 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -386,7 +386,11 @@ _idx_obj_part(const DedupMultiIdxType *idx_type, nm_hash_update_val(h, obj_a); return 0; } - nm_assert(NMP_OBJECT_CAST_OBJ_WITH_IFINDEX(obj_a)->ifindex > 0); + nm_assert(NMP_OBJECT_CAST_OBJ_WITH_IFINDEX(obj_a)->ifindex > 0 + || (NMP_OBJECT_CAST_OBJ_WITH_IFINDEX(obj_a)->ifindex == 0 + && NM_IN_SET(NMP_OBJECT_GET_TYPE(obj_a), + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE))); if (obj_b) { return NMP_OBJECT_GET_TYPE(obj_a) == NMP_OBJECT_GET_TYPE(obj_b) && NMP_OBJECT_CAST_OBJ_WITH_IFINDEX(obj_a)->ifindex @@ -401,14 +405,14 @@ _idx_obj_part(const DedupMultiIdxType *idx_type, case NMP_CACHE_ID_TYPE_ROUTES_BY_WEAK_ID: obj_type = NMP_OBJECT_GET_TYPE(obj_a); if (!NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE) - || NMP_OBJECT_CAST_IP_ROUTE(obj_a)->ifindex <= 0) { + || NMP_OBJECT_CAST_IP_ROUTE(obj_a)->ifindex < 0) { if (h) nm_hash_update_val(h, obj_a); return 0; } if (obj_b) { return obj_type == NMP_OBJECT_GET_TYPE(obj_b) - && NMP_OBJECT_CAST_IP_ROUTE(obj_b)->ifindex > 0 + && NMP_OBJECT_CAST_IP_ROUTE(obj_b)->ifindex >= 0 && (obj_type == NMP_OBJECT_TYPE_IP4_ROUTE ? (nm_platform_ip4_route_cmp(&obj_a->ip4_route, &obj_b->ip4_route, From ea4f6d7994af3820eb4646ea2bbe915fa27b09a2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 15:43:04 +0100 Subject: [PATCH 13/33] platform: rename NMPRulesManager API to NMPRouteManager Routes of type blackhole, unreachable, prohibit don't have an ifindex/device. They are thus in many ways similar to routing rules, as they are global. We need a mediator to keep track which routes to configure. This will be very similar to what NMPRulesManager already does for routing rules. Rename the API, so that it also can be used for routes. Renaming the file will be done next, so that git's rename detection doesn't get too confused. --- src/core/devices/nm-device.c | 37 +++-- src/core/nm-netns.c | 32 ++-- src/core/nm-netns.h | 2 +- src/core/platform/tests/test-route.c | 64 +++---- src/libnm-platform/nmp-rules-manager.c | 220 ++++++++++++------------- src/libnm-platform/nmp-rules-manager.h | 60 +++---- 6 files changed, 208 insertions(+), 207 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index a22bd82483..a0e69687bf 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -9283,7 +9283,7 @@ static void _routing_rules_sync(NMDevice *self, NMTernary set_mode) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); - NMPRulesManager *rules_manager = nm_netns_get_rules_manager(nm_device_get_netns(self)); + NMPRouteManager *route_manager = nm_netns_get_route_manager(nm_device_get_netns(self)); NMDeviceClass *klass = NM_DEVICE_GET_CLASS(self); gboolean untrack_only_dirty = FALSE; gboolean keep_deleted_rules; @@ -9301,9 +9301,9 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) int is_ipv4; untrack_only_dirty = TRUE; - nmp_rules_manager_set_dirty(rules_manager, user_tag_1); + nmp_route_manager_set_dirty(route_manager, user_tag_1); if (klass->get_extra_rules) - nmp_rules_manager_set_dirty(rules_manager, user_tag_2); + nmp_route_manager_set_dirty(route_manager, user_tag_2); applied_connection = nm_device_get_applied_connection(self); @@ -9323,13 +9323,13 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) nm_ip_routing_rule_to_platform(rule, &plrule); /* We track this rule, but we also make it explicitly not weakly-tracked - * (meaning to untrack NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG at + * (meaning to untrack NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG at * the same time). */ - nmp_rules_manager_track(rules_manager, - &plrule, - 10, - user_tag_1, - NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); + nmp_route_manager_track_rule(route_manager, + &plrule, + 10, + user_tag_1, + NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); } } @@ -9339,24 +9339,25 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) extra_rules = klass->get_extra_rules(self); if (extra_rules) { for (i = 0; i < extra_rules->len; i++) { - nmp_rules_manager_track(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(extra_rules->pdata[i]), - 10, - user_tag_2, - NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); + nmp_route_manager_track_rule( + route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(extra_rules->pdata[i]), + 10, + user_tag_2, + NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); } } } } - nmp_rules_manager_untrack_all(rules_manager, user_tag_1, !untrack_only_dirty); + nmp_route_manager_untrack_all(route_manager, user_tag_1, !untrack_only_dirty); if (klass->get_extra_rules) - nmp_rules_manager_untrack_all(rules_manager, user_tag_2, !untrack_only_dirty); + nmp_route_manager_untrack_all(route_manager, user_tag_2, !untrack_only_dirty); keep_deleted_rules = FALSE; if (set_mode == NM_TERNARY_DEFAULT) { /* when exiting NM, we leave the device up and the rules configured. - * We just all nmp_rules_manager_sync() to forget about the synced rules, + * We just all nmp_route_manager_sync_rules() to forget about the synced rules, * but we don't actually delete them. * * FIXME: that is a problem after restart of NetworkManager, because these @@ -9370,7 +9371,7 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) * file and track them after restart again. */ keep_deleted_rules = TRUE; } - nmp_rules_manager_sync(rules_manager, keep_deleted_rules); + nmp_route_manager_sync_rules(route_manager, keep_deleted_rules); } static gboolean diff --git a/src/core/nm-netns.c b/src/core/nm-netns.c index 4d3c3172ae..1253c8b4a2 100644 --- a/src/core/nm-netns.c +++ b/src/core/nm-netns.c @@ -25,7 +25,7 @@ typedef struct { NMNetns *_self_signal_user_data; NMPlatform *platform; NMPNetns *platform_netns; - NMPRulesManager *rules_manager; + NMPRouteManager *route_manager; GHashTable *l3cfgs; GHashTable *shared_ips; CList l3cfg_signal_pending_lst_head; @@ -79,10 +79,10 @@ nm_netns_get_platform(NMNetns *self) return NM_NETNS_GET_PRIVATE(self)->platform; } -NMPRulesManager * -nm_netns_get_rules_manager(NMNetns *self) +NMPRouteManager * +nm_netns_get_route_manager(NMNetns *self) { - return NM_NETNS_GET_PRIVATE(self)->rules_manager; + return NM_NETNS_GET_PRIVATE(self)->route_manager; } NMDedupMultiIndex * @@ -397,14 +397,14 @@ constructed(GObject *object) priv->platform_netns = nm_platform_netns_get(priv->platform); - priv->rules_manager = nmp_rules_manager_new(priv->platform); + priv->route_manager = nmp_route_manager_new(priv->platform); /* Weakly track the default rules with a dummy user-tag. These * rules are always weekly tracked... */ - nmp_rules_manager_track_default(priv->rules_manager, - AF_UNSPEC, - 0, - nm_netns_parent_class /* static dummy user-tag */); + nmp_route_manager_track_rule_default(priv->route_manager, + AF_UNSPEC, + 0, + nm_netns_parent_class /* static dummy user-tag */); /* Also weakly track all existing rules. These were added before NetworkManager * starts, so they are probably none of NetworkManager's business. @@ -414,12 +414,12 @@ constructed(GObject *object) * of NetworkManager, we just don't know. * * For that reason, whenever we will touch such rules later one, we make them - * fully owned and no longer weekly tracked. See %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */ - nmp_rules_manager_track_from_platform(priv->rules_manager, - NULL, - AF_UNSPEC, - 0, - NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); + * fully owned and no longer weekly tracked. See %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */ + nmp_route_manager_track_rule_from_platform(priv->route_manager, + NULL, + AF_UNSPEC, + 0, + NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG); G_OBJECT_CLASS(nm_netns_parent_class)->constructed(object); @@ -469,7 +469,7 @@ dispose(GObject *object) g_clear_object(&priv->platform); nm_clear_pointer(&priv->l3cfgs, g_hash_table_unref); - nm_clear_pointer(&priv->rules_manager, nmp_rules_manager_unref); + nm_clear_pointer(&priv->route_manager, nmp_route_manager_unref); G_OBJECT_CLASS(nm_netns_parent_class)->dispose(object); } diff --git a/src/core/nm-netns.h b/src/core/nm-netns.h index 0cb5637544..deb1d1f006 100644 --- a/src/core/nm-netns.h +++ b/src/core/nm-netns.h @@ -29,7 +29,7 @@ NMNetns *nm_netns_new(struct _NMPlatform *platform); struct _NMPlatform *nm_netns_get_platform(NMNetns *self); NMPNetns *nm_netns_get_platform_netns(NMNetns *self); -struct _NMPRulesManager *nm_netns_get_rules_manager(NMNetns *self); +struct _NMPRouteManager *nm_netns_get_route_manager(NMNetns *self); struct _NMDedupMultiIndex *nm_netns_get_multi_idx(NMNetns *self); diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index 4974a409cb..969ed7f2f5 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -1637,8 +1637,8 @@ again: if (TEST_SYNC) { gs_unref_hashtable GHashTable *unique_priorities = g_hash_table_new(NULL, NULL); - nm_auto_unref_rules_manager NMPRulesManager *rules_manager = - nmp_rules_manager_new(platform); + nm_auto_unref_route_manager NMPRouteManager *route_manager = + nmp_route_manager_new(platform); gs_unref_ptrarray GPtrArray *objs_sync = NULL; gconstpointer USER_TAG_1 = &platform; gconstpointer USER_TAG_2 = &unique_priorities; @@ -1660,29 +1660,29 @@ again: } for (i = 0; i < objs_sync->len; i++) { - nmp_rules_manager_track(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - 1, - USER_TAG_1, - NULL); + nmp_route_manager_track_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + 1, + USER_TAG_1, + NULL); if (nmtst_get_rand_bool()) { /* this has no effect, because a negative priority (of same absolute value) * has lower priority than the positive priority above. */ - nmp_rules_manager_track(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - -1, - USER_TAG_2, - NULL); + nmp_route_manager_track_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + -1, + USER_TAG_2, + NULL); } if (nmtst_get_rand_uint32() % objs_sync->len == 0) { - nmp_rules_manager_sync(rules_manager, FALSE); + nmp_route_manager_sync_rules(route_manager, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, i + 1); } } - nmp_rules_manager_sync(rules_manager, FALSE); + nmp_route_manager_sync_rules(route_manager, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, objs_sync->len); @@ -1690,37 +1690,37 @@ again: for (i = 0; i < objs_sync->len; i++) { switch (nmtst_get_rand_uint32() % 3) { case 0: - nmp_rules_manager_untrack(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - USER_TAG_1); - nmp_rules_manager_untrack(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - USER_TAG_1); + nmp_route_manager_untrack_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + USER_TAG_1); + nmp_route_manager_untrack_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + USER_TAG_1); break; case 1: - nmp_rules_manager_track(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - -1, - USER_TAG_1, - NULL); + nmp_route_manager_track_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + -1, + USER_TAG_1, + NULL); break; case 2: - nmp_rules_manager_track(rules_manager, - NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), - -2, - USER_TAG_2, - NULL); + nmp_route_manager_track_rule(route_manager, + NMP_OBJECT_CAST_ROUTING_RULE(objs_sync->pdata[i]), + -2, + USER_TAG_2, + NULL); break; } if (nmtst_get_rand_uint32() % objs_sync->len == 0) { - nmp_rules_manager_sync(rules_manager, FALSE); + nmp_route_manager_sync_rules(route_manager, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, objs_sync->len - i - 1); } } - nmp_rules_manager_sync(rules_manager, FALSE); + nmp_route_manager_sync_rules(route_manager, FALSE); } else { for (i = 0; i < objs->len;) { diff --git a/src/libnm-platform/nmp-rules-manager.c b/src/libnm-platform/nmp-rules-manager.c index 746d1eaa74..bd63d462d8 100644 --- a/src/libnm-platform/nmp-rules-manager.c +++ b/src/libnm-platform/nmp-rules-manager.c @@ -13,7 +13,7 @@ /*****************************************************************************/ -struct _NMPRulesManager { +struct _NMPRouteManager { NMPlatform *platform; GHashTable *by_obj; GHashTable *by_user_tag; @@ -23,12 +23,12 @@ struct _NMPRulesManager { /*****************************************************************************/ -static void _rules_init(NMPRulesManager *self); +static void _rules_init(NMPRouteManager *self); /*****************************************************************************/ #define _NMLOG_DOMAIN LOGD_PLATFORM -#define _NMLOG_PREFIX_NAME "rules-manager" +#define _NMLOG_PREFIX_NAME "route-manager" #define _NMLOG(level, ...) \ G_STMT_START \ @@ -50,10 +50,10 @@ static void _rules_init(NMPRulesManager *self); /*****************************************************************************/ static gboolean -NMP_IS_RULES_MANAGER(gpointer self) +NMP_IS_ROUTE_MANAGER(gpointer self) { - return self && ((NMPRulesManager *) self)->ref_count > 0 - && NM_IS_PLATFORM(((NMPRulesManager *) self)->platform); + return self && ((NMPRouteManager *) self)->ref_count > 0 + && NM_IS_PLATFORM(((NMPRouteManager *) self)->platform); } #define _USER_TAG_LOG(user_tag) nm_hash_obfuscate_ptr(1240261787u, (user_tag)) @@ -109,13 +109,13 @@ typedef struct { /* indicates whether we configured/removed the rule (during sync()). We need that, so * if the rule gets untracked, that we know to remove/restore it. * - * This makes NMPRulesManager stateful (beyond the configuration that indicates + * This makes NMPRouteManager stateful (beyond the configuration that indicates * which rules are tracked). * After a restart, NetworkManager would no longer remember which rules were added * by us. * * That is partially fixed by NetworkManager taking over the rules that it - * actively configures (see %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG). */ + * actively configures (see %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG). */ ConfigState config_state; } RulesObjData; @@ -126,7 +126,7 @@ typedef struct { /*****************************************************************************/ -static void _rules_data_untrack(NMPRulesManager *self, +static void _rules_data_untrack(NMPRouteManager *self, RulesData *rules_data, gboolean remove_user_tag_data, gboolean make_owned_by_us); @@ -289,8 +289,8 @@ _rules_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user } /** - * nmp_rules_manager_track: - * @self: the #NMPRulesManager instance + * nmp_route_manager_track_rule: + * @self: the #NMPRouteManager instance * @routing_rule: the #NMPlatformRoutingRule to track or untrack * @track_priority: the priority for tracking the rule. Note that * negative values indicate a forced absence of the rule. Priorities @@ -302,17 +302,17 @@ _rules_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user * @user_tag: the tag associated with tracking this rule. The same tag * must be used to untrack the rule later. * @user_tag_untrack: if not %NULL, at the same time untrack this user-tag - * for the same rule. Note that this is different from a plain nmp_rules_manager_untrack(), + * for the same rule. Note that this is different from a plain nmp_route_manager_untrack_rule(), * because it enforces ownership of the now tracked rule. On the other hand, - * a plain nmp_rules_manager_untrack() merely forgets about the tracking. - * The purpose here is to set this to %NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. + * a plain nmp_route_manager_untrack_rule() merely forgets about the tracking. + * The purpose here is to set this to %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */ void -nmp_rules_manager_track(NMPRulesManager *self, - const NMPlatformRoutingRule *routing_rule, - gint32 track_priority, - gconstpointer user_tag, - gconstpointer user_tag_untrack) +nmp_route_manager_track_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack) { NMPObject obj_stack; const NMPObject *p_obj_stack; @@ -323,7 +323,7 @@ nmp_rules_manager_track(NMPRulesManager *self, guint32 track_priority_val; gboolean track_priority_present; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(routing_rule); g_return_if_fail(user_tag); nm_assert(track_priority != G_MININT32); @@ -414,14 +414,14 @@ nmp_rules_manager_track(NMPRulesManager *self, } static void -_rules_data_untrack(NMPRulesManager *self, +_rules_data_untrack(NMPRouteManager *self, RulesData *rules_data, gboolean remove_user_tag_data, gboolean make_owned_by_us) { RulesObjData *obj_data; - nm_assert(NMP_IS_RULES_MANAGER(self)); + nm_assert(NMP_IS_ROUTE_MANAGER(self)); _rules_data_assert(rules_data, TRUE); nm_assert(self->by_data); nm_assert(g_hash_table_lookup(self->by_data, rules_data) == rules_data); @@ -465,15 +465,15 @@ _rules_data_untrack(NMPRulesManager *self, } void -nmp_rules_manager_untrack(NMPRulesManager *self, - const NMPlatformRoutingRule *routing_rule, - gconstpointer user_tag) +nmp_route_manager_untrack_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gconstpointer user_tag) { NMPObject obj_stack; const NMPObject *p_obj_stack; RulesData *rules_data; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(routing_rule); g_return_if_fail(user_tag); @@ -489,12 +489,12 @@ nmp_rules_manager_untrack(NMPRulesManager *self, } void -nmp_rules_manager_set_dirty(NMPRulesManager *self, gconstpointer user_tag) +nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) { RulesData *rules_data; RulesUserTagData *user_tag_data; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); if (!self->by_data) @@ -509,7 +509,7 @@ nmp_rules_manager_set_dirty(NMPRulesManager *self, gconstpointer user_tag) } void -nmp_rules_manager_untrack_all(NMPRulesManager *self, +nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, gboolean all /* or only dirty */) { @@ -517,7 +517,7 @@ nmp_rules_manager_untrack_all(NMPRulesManager *self, RulesData *rules_data_safe; RulesUserTagData *user_tag_data; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); if (!self->by_data) @@ -539,7 +539,7 @@ nmp_rules_manager_untrack_all(NMPRulesManager *self, } void -nmp_rules_manager_sync(NMPRulesManager *self, gboolean keep_deleted_rules) +nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) { const NMDedupMultiHeadEntry *pl_head_entry; NMDedupMultiIter pl_iter; @@ -550,7 +550,7 @@ nmp_rules_manager_sync(NMPRulesManager *self, gboolean keep_deleted_rules) guint i; const RulesData *rd_best; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); if (!self->by_data) return; @@ -643,18 +643,18 @@ nmp_rules_manager_sync(NMPRulesManager *self, gboolean keep_deleted_rules) } void -nmp_rules_manager_track_from_platform(NMPRulesManager *self, - NMPlatform *platform, - int addr_family, - gint32 tracking_priority, - gconstpointer user_tag) +nmp_route_manager_track_rule_from_platform(NMPRouteManager *self, + NMPlatform *platform, + int addr_family, + gint32 tracking_priority, + gconstpointer user_tag) { NMPLookup lookup; const NMDedupMultiHeadEntry *head_entry; NMDedupMultiIter iter; const NMPObject *o; - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); if (!platform) platform = self->platform; @@ -671,87 +671,87 @@ nmp_rules_manager_track_from_platform(NMPRulesManager *self, if (addr_family != AF_UNSPEC && rr->addr_family != addr_family) continue; - nmp_rules_manager_track(self, rr, tracking_priority, user_tag, NULL); + nmp_route_manager_track_rule(self, rr, tracking_priority, user_tag, NULL); } } /*****************************************************************************/ void -nmp_rules_manager_track_default(NMPRulesManager *self, - int addr_family, - gint32 track_priority, - gconstpointer user_tag) +nmp_route_manager_track_rule_default(NMPRouteManager *self, + int addr_family, + gint32 track_priority, + gconstpointer user_tag) { - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); nm_assert(NM_IN_SET(addr_family, AF_UNSPEC, AF_INET, AF_INET6)); /* track the default rules. See also `man ip-rule`. */ if (NM_IN_SET(addr_family, AF_UNSPEC, AF_INET)) { - nmp_rules_manager_track(self, - &((NMPlatformRoutingRule){ - .addr_family = AF_INET, - .priority = 0, - .table = RT_TABLE_LOCAL, - .action = FR_ACT_TO_TBL, - .protocol = RTPROT_KERNEL, - }), - track_priority, - user_tag, - NULL); - nmp_rules_manager_track(self, - &((NMPlatformRoutingRule){ - .addr_family = AF_INET, - .priority = 32766, - .table = RT_TABLE_MAIN, - .action = FR_ACT_TO_TBL, - .protocol = RTPROT_KERNEL, - }), - track_priority, - user_tag, - NULL); - nmp_rules_manager_track(self, - &((NMPlatformRoutingRule){ - .addr_family = AF_INET, - .priority = 32767, - .table = RT_TABLE_DEFAULT, - .action = FR_ACT_TO_TBL, - .protocol = RTPROT_KERNEL, - }), - track_priority, - user_tag, - NULL); + nmp_route_manager_track_rule(self, + &((NMPlatformRoutingRule){ + .addr_family = AF_INET, + .priority = 0, + .table = RT_TABLE_LOCAL, + .action = FR_ACT_TO_TBL, + .protocol = RTPROT_KERNEL, + }), + track_priority, + user_tag, + NULL); + nmp_route_manager_track_rule(self, + &((NMPlatformRoutingRule){ + .addr_family = AF_INET, + .priority = 32766, + .table = RT_TABLE_MAIN, + .action = FR_ACT_TO_TBL, + .protocol = RTPROT_KERNEL, + }), + track_priority, + user_tag, + NULL); + nmp_route_manager_track_rule(self, + &((NMPlatformRoutingRule){ + .addr_family = AF_INET, + .priority = 32767, + .table = RT_TABLE_DEFAULT, + .action = FR_ACT_TO_TBL, + .protocol = RTPROT_KERNEL, + }), + track_priority, + user_tag, + NULL); } if (NM_IN_SET(addr_family, AF_UNSPEC, AF_INET6)) { - nmp_rules_manager_track(self, - &((NMPlatformRoutingRule){ - .addr_family = AF_INET6, - .priority = 0, - .table = RT_TABLE_LOCAL, - .action = FR_ACT_TO_TBL, - .protocol = RTPROT_KERNEL, - }), - track_priority, - user_tag, - NULL); - nmp_rules_manager_track(self, - &((NMPlatformRoutingRule){ - .addr_family = AF_INET6, - .priority = 32766, - .table = RT_TABLE_MAIN, - .action = FR_ACT_TO_TBL, - .protocol = RTPROT_KERNEL, - }), - track_priority, - user_tag, - NULL); + nmp_route_manager_track_rule(self, + &((NMPlatformRoutingRule){ + .addr_family = AF_INET6, + .priority = 0, + .table = RT_TABLE_LOCAL, + .action = FR_ACT_TO_TBL, + .protocol = RTPROT_KERNEL, + }), + track_priority, + user_tag, + NULL); + nmp_route_manager_track_rule(self, + &((NMPlatformRoutingRule){ + .addr_family = AF_INET6, + .priority = 32766, + .table = RT_TABLE_MAIN, + .action = FR_ACT_TO_TBL, + .protocol = RTPROT_KERNEL, + }), + track_priority, + user_tag, + NULL); } } static void -_rules_init(NMPRulesManager *self) +_rules_init(NMPRouteManager *self) { if (self->by_data) return; @@ -768,15 +768,15 @@ _rules_init(NMPRulesManager *self) /*****************************************************************************/ -NMPRulesManager * -nmp_rules_manager_new(NMPlatform *platform) +NMPRouteManager * +nmp_route_manager_new(NMPlatform *platform) { - NMPRulesManager *self; + NMPRouteManager *self; g_return_val_if_fail(NM_IS_PLATFORM(platform), NULL); - self = g_slice_new(NMPRulesManager); - *self = (NMPRulesManager){ + self = g_slice_new(NMPRouteManager); + *self = (NMPRouteManager){ .ref_count = 1, .platform = g_object_ref(platform), }; @@ -784,17 +784,17 @@ nmp_rules_manager_new(NMPlatform *platform) } void -nmp_rules_manager_ref(NMPRulesManager *self) +nmp_route_manager_ref(NMPRouteManager *self) { - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); self->ref_count++; } void -nmp_rules_manager_unref(NMPRulesManager *self) +nmp_route_manager_unref(NMPRouteManager *self) { - g_return_if_fail(NMP_IS_RULES_MANAGER(self)); + g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); if (--self->ref_count > 0) return; @@ -805,5 +805,5 @@ nmp_rules_manager_unref(NMPRulesManager *self) g_hash_table_destroy(self->by_data); } g_object_unref(self->platform); - g_slice_free(NMPRulesManager, self); + g_slice_free(NMPRouteManager, self); } diff --git a/src/libnm-platform/nmp-rules-manager.h b/src/libnm-platform/nmp-rules-manager.h index 65e8ddb236..3bc4a49301 100644 --- a/src/libnm-platform/nmp-rules-manager.h +++ b/src/libnm-platform/nmp-rules-manager.h @@ -1,53 +1,53 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#ifndef __NMP_RULES_MANAGER_H__ -#define __NMP_RULES_MANAGER_H__ +#ifndef __NMP_ROUTE_MANAGER_H__ +#define __NMP_ROUTE_MANAGER_H__ #include "nm-platform.h" /*****************************************************************************/ -#define NMP_RULES_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG ((const void *) nmp_rules_manager_new) +#define NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG ((const void *) nmp_route_manager_new) -typedef struct _NMPRulesManager NMPRulesManager; +typedef struct _NMPRouteManager NMPRouteManager; -NMPRulesManager *nmp_rules_manager_new(NMPlatform *platform); +NMPRouteManager *nmp_route_manager_new(NMPlatform *platform); -void nmp_rules_manager_ref(NMPRulesManager *self); -void nmp_rules_manager_unref(NMPRulesManager *self); +void nmp_route_manager_ref(NMPRouteManager *self); +void nmp_route_manager_unref(NMPRouteManager *self); -#define nm_auto_unref_rules_manager nm_auto(_nmp_rules_manager_unref) -NM_AUTO_DEFINE_FCN0(NMPRulesManager *, _nmp_rules_manager_unref, nmp_rules_manager_unref); +#define nm_auto_unref_route_manager nm_auto(_nmp_route_manager_unref) +NM_AUTO_DEFINE_FCN0(NMPRouteManager *, _nmp_route_manager_unref, nmp_route_manager_unref); -void nmp_rules_manager_track(NMPRulesManager *self, - const NMPlatformRoutingRule *routing_rule, - gint32 track_priority, - gconstpointer user_tag, - gconstpointer user_tag_untrack); +void nmp_route_manager_track_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack); -void nmp_rules_manager_track_default(NMPRulesManager *self, - int addr_family, - gint32 track_priority, - gconstpointer user_tag); +void nmp_route_manager_track_rule_default(NMPRouteManager *self, + int addr_family, + gint32 track_priority, + gconstpointer user_tag); -void nmp_rules_manager_track_from_platform(NMPRulesManager *self, - NMPlatform *platform, - int addr_family, - gint32 tracking_priority, - gconstpointer user_tag); +void nmp_route_manager_track_rule_from_platform(NMPRouteManager *self, + NMPlatform *platform, + int addr_family, + gint32 tracking_priority, + gconstpointer user_tag); -void nmp_rules_manager_untrack(NMPRulesManager *self, - const NMPlatformRoutingRule *routing_rule, - gconstpointer user_tag); +void nmp_route_manager_untrack_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gconstpointer user_tag); -void nmp_rules_manager_set_dirty(NMPRulesManager *self, gconstpointer user_tag); +void nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag); -void nmp_rules_manager_untrack_all(NMPRulesManager *self, +void nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, gboolean all /* or only dirty */); -void nmp_rules_manager_sync(NMPRulesManager *self, gboolean keep_deleted_rules); +void nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules); /*****************************************************************************/ -#endif /* __NMP_RULES_MANAGER_H__ */ +#endif /* __NMP_ROUTE_MANAGER_H__ */ From 3996933c579a1eef59bcfc442644372ab19dd90c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:01:27 +0100 Subject: [PATCH 14/33] platform: rename "nmp-route-manager.h" to "nmp-rules-manager.h" --- Makefile.am | 4 ++-- src/core/devices/nm-device-wireguard.c | 2 +- src/core/devices/nm-device.c | 2 +- src/core/nm-netns.c | 2 +- src/core/platform/tests/test-route.c | 2 +- src/libnm-platform/meson.build | 2 +- .../{nmp-rules-manager.c => nmp-route-manager.c} | 2 +- .../{nmp-rules-manager.h => nmp-route-manager.h} | 0 8 files changed, 8 insertions(+), 8 deletions(-) rename src/libnm-platform/{nmp-rules-manager.c => nmp-route-manager.c} (99%) rename src/libnm-platform/{nmp-rules-manager.h => nmp-route-manager.h} (100%) diff --git a/Makefile.am b/Makefile.am index bc8dc4cf27..ab06c6db6e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -603,8 +603,8 @@ src_libnm_platform_libnm_platform_la_SOURCES = \ src/libnm-platform/nmp-netns.h \ src/libnm-platform/nmp-object.c \ src/libnm-platform/nmp-object.h \ - src/libnm-platform/nmp-rules-manager.c \ - src/libnm-platform/nmp-rules-manager.h \ + src/libnm-platform/nmp-route-manager.c \ + src/libnm-platform/nmp-route-manager.h \ src/libnm-platform/wifi/nm-wifi-utils-nl80211.c \ src/libnm-platform/wifi/nm-wifi-utils-nl80211.h \ src/libnm-platform/wifi/nm-wifi-utils-private.h \ diff --git a/src/core/devices/nm-device-wireguard.c b/src/core/devices/nm-device-wireguard.c index 4399e83478..bdb96cb298 100644 --- a/src/core/devices/nm-device-wireguard.c +++ b/src/core/devices/nm-device-wireguard.c @@ -18,7 +18,7 @@ #include "nm-device-private.h" #include "libnm-platform/nm-platform.h" #include "libnm-platform/nmp-object.h" -#include "libnm-platform/nmp-rules-manager.h" +#include "libnm-platform/nmp-route-manager.h" #include "nm-device-factory.h" #include "nm-active-connection.h" #include "nm-act-request.h" diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index a0e69687bf..f7836567a0 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -41,7 +41,7 @@ #include "libnm-platform/nm-platform.h" #include "libnm-platform/nm-platform-utils.h" #include "libnm-platform/nmp-object.h" -#include "libnm-platform/nmp-rules-manager.h" +#include "libnm-platform/nmp-route-manager.h" #include "ndisc/nm-ndisc.h" #include "ndisc/nm-lndp-ndisc.h" diff --git a/src/core/nm-netns.c b/src/core/nm-netns.c index 1253c8b4a2..f120e43d06 100644 --- a/src/core/nm-netns.c +++ b/src/core/nm-netns.c @@ -15,7 +15,7 @@ #include "nm-l3cfg.h" #include "libnm-platform/nm-platform.h" #include "libnm-platform/nmp-netns.h" -#include "libnm-platform/nmp-rules-manager.h" +#include "libnm-platform/nmp-route-manager.h" /*****************************************************************************/ diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index 969ed7f2f5..fb6ca72d32 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -10,7 +10,7 @@ #include "nm-core-utils.h" #include "libnm-platform/nm-platform-utils.h" -#include "libnm-platform/nmp-rules-manager.h" +#include "libnm-platform/nmp-route-manager.h" #include "test-common.h" diff --git a/src/libnm-platform/meson.build b/src/libnm-platform/meson.build index 3e4f41a929..d0a7c6c20e 100644 --- a/src/libnm-platform/meson.build +++ b/src/libnm-platform/meson.build @@ -9,7 +9,7 @@ libnm_platform = static_library( 'nm-platform.c', 'nmp-netns.c', 'nmp-object.c', - 'nmp-rules-manager.c', + 'nmp-route-manager.c', 'wifi/nm-wifi-utils-nl80211.c', 'wifi/nm-wifi-utils.c', 'wpan/nm-wpan-utils.c', diff --git a/src/libnm-platform/nmp-rules-manager.c b/src/libnm-platform/nmp-route-manager.c similarity index 99% rename from src/libnm-platform/nmp-rules-manager.c rename to src/libnm-platform/nmp-route-manager.c index bd63d462d8..3e83186ff7 100644 --- a/src/libnm-platform/nmp-rules-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -2,7 +2,7 @@ #include "libnm-glib-aux/nm-default-glib-i18n-lib.h" -#include "nmp-rules-manager.h" +#include "nmp-route-manager.h" #include #include diff --git a/src/libnm-platform/nmp-rules-manager.h b/src/libnm-platform/nmp-route-manager.h similarity index 100% rename from src/libnm-platform/nmp-rules-manager.h rename to src/libnm-platform/nmp-route-manager.h From 5b3e96451b68e8ff38c4ecfb3c2cd6d74a2faf49 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:05:46 +0100 Subject: [PATCH 15/33] platform: drop lazy initialization _rules_init() of NMPRouteManager Let's just always allocate the hash tables. We will likely need them, and three hash tables are relatively cheap. --- src/libnm-platform/nmp-route-manager.c | 49 ++++++-------------------- 1 file changed, 11 insertions(+), 38 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index 3e83186ff7..3174eeef86 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -23,10 +23,6 @@ struct _NMPRouteManager { /*****************************************************************************/ -static void _rules_init(NMPRouteManager *self); - -/*****************************************************************************/ - #define _NMLOG_DOMAIN LOGD_PLATFORM #define _NMLOG_PREFIX_NAME "route-manager" @@ -328,8 +324,6 @@ nmp_route_manager_track_rule(NMPRouteManager *self, g_return_if_fail(user_tag); nm_assert(track_priority != G_MININT32); - _rules_init(self); - p_obj_stack = nmp_object_stackinit(&obj_stack, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule); nm_assert(nmp_object_is_visible(p_obj_stack)); @@ -477,8 +471,6 @@ nmp_route_manager_untrack_rule(NMPRouteManager *self, g_return_if_fail(routing_rule); g_return_if_fail(user_tag); - _rules_init(self); - p_obj_stack = nmp_object_stackinit(&obj_stack, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule); nm_assert(nmp_object_is_visible(p_obj_stack)); @@ -497,9 +489,6 @@ nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); - if (!self->by_data) - return; - user_tag_data = g_hash_table_lookup(self->by_user_tag, &user_tag); if (!user_tag_data) return; @@ -520,9 +509,6 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); - if (!self->by_data) - return; - user_tag_data = g_hash_table_lookup(self->by_user_tag, &user_tag); if (!user_tag_data) return; @@ -552,9 +538,6 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); - if (!self->by_data) - return; - _LOGD("sync%s", keep_deleted_rules ? " (don't remove any rules)" : ""); pl_head_entry = nm_platform_lookup_obj_type(self->platform, NMP_OBJECT_TYPE_ROUTING_RULE); @@ -750,22 +733,6 @@ nmp_route_manager_track_rule_default(NMPRouteManager *self, } } -static void -_rules_init(NMPRouteManager *self) -{ - if (self->by_data) - return; - - self->by_data = - g_hash_table_new_full(_rules_data_hash, _rules_data_equal, NULL, _rules_data_destroy); - self->by_obj = - g_hash_table_new_full(_rules_obj_hash, _rules_obj_equal, NULL, _rules_obj_destroy); - self->by_user_tag = g_hash_table_new_full(_rules_user_tag_hash, - _rules_user_tag_equal, - NULL, - _rules_user_tag_destroy); -} - /*****************************************************************************/ NMPRouteManager * @@ -779,6 +746,14 @@ nmp_route_manager_new(NMPlatform *platform) *self = (NMPRouteManager){ .ref_count = 1, .platform = g_object_ref(platform), + .by_data = + g_hash_table_new_full(_rules_data_hash, _rules_data_equal, NULL, _rules_data_destroy), + .by_obj = + g_hash_table_new_full(_rules_obj_hash, _rules_obj_equal, NULL, _rules_obj_destroy), + .by_user_tag = g_hash_table_new_full(_rules_user_tag_hash, + _rules_user_tag_equal, + NULL, + _rules_user_tag_destroy), }; return self; } @@ -799,11 +774,9 @@ nmp_route_manager_unref(NMPRouteManager *self) if (--self->ref_count > 0) return; - if (self->by_data) { - g_hash_table_destroy(self->by_user_tag); - g_hash_table_destroy(self->by_obj); - g_hash_table_destroy(self->by_data); - } + g_hash_table_destroy(self->by_user_tag); + g_hash_table_destroy(self->by_obj); + g_hash_table_destroy(self->by_data); g_object_unref(self->platform); g_slice_free(NMPRouteManager, self); } From 75959e2f1acbebfb8a0b6b570e079fe60aa8f2a6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:29:11 +0100 Subject: [PATCH 16/33] platform: rename internals in "nmp-route-manager.c" We will not only track (routing) rules, but also routes. Rename. --- src/libnm-platform/nmp-route-manager.c | 296 +++++++++++++------------ 1 file changed, 149 insertions(+), 147 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index 3174eeef86..e7a9c005cd 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -68,7 +68,7 @@ typedef struct { * * Otherwise, the track_priority_val goes together with track_priority_present. * In case of one rule being tracked multiple times (with different priorities), - * the one with higher priority wins. See _rules_obj_get_best_data(). + * the one with higher priority wins. See _track_obj_data_get_best_data(). * Then, the winning present state either enforces that the rule is present * or absent. * @@ -79,7 +79,7 @@ typedef struct { bool track_priority_present : 1; bool dirty : 1; -} RulesData; +} TrackData; typedef enum { CONFIG_STATE_NONE = 0, @@ -113,92 +113,92 @@ typedef struct { * That is partially fixed by NetworkManager taking over the rules that it * actively configures (see %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG). */ ConfigState config_state; -} RulesObjData; +} TrackObjData; typedef struct { gconstpointer user_tag; CList user_tag_lst_head; -} RulesUserTagData; +} TrackUserTagData; /*****************************************************************************/ -static void _rules_data_untrack(NMPRouteManager *self, - RulesData *rules_data, +static void _track_data_untrack(NMPRouteManager *self, + TrackData *track_data, gboolean remove_user_tag_data, gboolean make_owned_by_us); /*****************************************************************************/ static void -_rules_data_assert(const RulesData *rules_data, gboolean linked) +_track_data_assert(const TrackData *track_data, gboolean linked) { - nm_assert(rules_data); - nm_assert(NMP_OBJECT_GET_TYPE(rules_data->obj) == NMP_OBJECT_TYPE_ROUTING_RULE); - nm_assert(nmp_object_is_visible(rules_data->obj)); - nm_assert(rules_data->user_tag); - nm_assert(!linked || !c_list_is_empty(&rules_data->obj_lst)); - nm_assert(!linked || !c_list_is_empty(&rules_data->user_tag_lst)); + nm_assert(track_data); + nm_assert(NMP_OBJECT_GET_TYPE(track_data->obj) == NMP_OBJECT_TYPE_ROUTING_RULE); + nm_assert(nmp_object_is_visible(track_data->obj)); + nm_assert(track_data->user_tag); + nm_assert(!linked || !c_list_is_empty(&track_data->obj_lst)); + nm_assert(!linked || !c_list_is_empty(&track_data->user_tag_lst)); } static guint -_rules_data_hash(gconstpointer data) +_track_data_hash(gconstpointer data) { - const RulesData *rules_data = data; + const TrackData *track_data = data; NMHashState h; - _rules_data_assert(rules_data, FALSE); + _track_data_assert(track_data, FALSE); nm_hash_init(&h, 269297543u); - nm_platform_routing_rule_hash_update(NMP_OBJECT_CAST_ROUTING_RULE(rules_data->obj), + nm_platform_routing_rule_hash_update(NMP_OBJECT_CAST_ROUTING_RULE(track_data->obj), NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID, &h); - nm_hash_update_val(&h, rules_data->user_tag); + nm_hash_update_val(&h, track_data->user_tag); return nm_hash_complete(&h); } static gboolean -_rules_data_equal(gconstpointer data_a, gconstpointer data_b) +_track_data_equal(gconstpointer data_a, gconstpointer data_b) { - const RulesData *rules_data_a = data_a; - const RulesData *rules_data_b = data_b; + const TrackData *track_data_a = data_a; + const TrackData *track_data_b = data_b; - _rules_data_assert(rules_data_a, FALSE); - _rules_data_assert(rules_data_b, FALSE); + _track_data_assert(track_data_a, FALSE); + _track_data_assert(track_data_b, FALSE); - return rules_data_a->user_tag == rules_data_b->user_tag - && (nm_platform_routing_rule_cmp(NMP_OBJECT_CAST_ROUTING_RULE(rules_data_a->obj), - NMP_OBJECT_CAST_ROUTING_RULE(rules_data_b->obj), + return track_data_a->user_tag == track_data_b->user_tag + && (nm_platform_routing_rule_cmp(NMP_OBJECT_CAST_ROUTING_RULE(track_data_a->obj), + NMP_OBJECT_CAST_ROUTING_RULE(track_data_b->obj), NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID) == 0); } static void -_rules_data_destroy(gpointer data) +_track_data_destroy(gpointer data) { - RulesData *rules_data = data; + TrackData *track_data = data; - _rules_data_assert(rules_data, FALSE); + _track_data_assert(track_data, FALSE); - c_list_unlink_stale(&rules_data->obj_lst); - c_list_unlink_stale(&rules_data->user_tag_lst); - nmp_object_unref(rules_data->obj); - g_slice_free(RulesData, rules_data); + c_list_unlink_stale(&track_data->obj_lst); + c_list_unlink_stale(&track_data->user_tag_lst); + nmp_object_unref(track_data->obj); + g_slice_free(TrackData, track_data); } -static const RulesData * -_rules_obj_get_best_data(RulesObjData *obj_data) +static const TrackData * +_track_obj_data_get_best_data(TrackObjData *obj_data) { - RulesData *rules_data; - const RulesData *rd_best = NULL; + TrackData *track_data; + const TrackData *td_best = NULL; - c_list_for_each_entry (rules_data, &obj_data->obj_lst_head, obj_lst) { - _rules_data_assert(rules_data, TRUE); + c_list_for_each_entry (track_data, &obj_data->obj_lst_head, obj_lst) { + _track_data_assert(track_data, TRUE); - if (rd_best) { - if (rd_best->track_priority_val > rules_data->track_priority_val) + if (td_best) { + if (td_best->track_priority_val > track_data->track_priority_val) continue; - if (rd_best->track_priority_val == rules_data->track_priority_val) { - if (rd_best->track_priority_present || !rules_data->track_priority_present) { + if (td_best->track_priority_val == track_data->track_priority_val) { + if (td_best->track_priority_present || !track_data->track_priority_present) { /* if the priorities are identical, then "present" wins over * "!present" (absent). */ continue; @@ -206,16 +206,16 @@ _rules_obj_get_best_data(RulesObjData *obj_data) } } - rd_best = rules_data; + td_best = track_data; } - return rd_best; + return td_best; } static guint -_rules_obj_hash(gconstpointer data) +_track_obj_data_hash(gconstpointer data) { - const RulesObjData *obj_data = data; + const TrackObjData *obj_data = data; NMHashState h; nm_hash_init(&h, 432817559u); @@ -226,10 +226,10 @@ _rules_obj_hash(gconstpointer data) } static gboolean -_rules_obj_equal(gconstpointer data_a, gconstpointer data_b) +_track_obj_data_equal(gconstpointer data_a, gconstpointer data_b) { - const RulesObjData *obj_data_a = data_a; - const RulesObjData *obj_data_b = data_b; + const TrackObjData *obj_data_a = data_a; + const TrackObjData *obj_data_b = data_b; return (nm_platform_routing_rule_cmp(NMP_OBJECT_CAST_ROUTING_RULE(obj_data_a->obj), NMP_OBJECT_CAST_ROUTING_RULE(obj_data_b->obj), @@ -238,50 +238,50 @@ _rules_obj_equal(gconstpointer data_a, gconstpointer data_b) } static void -_rules_obj_destroy(gpointer data) +_track_obj_data_destroy(gpointer data) { - RulesObjData *obj_data = data; + TrackObjData *obj_data = data; c_list_unlink_stale(&obj_data->obj_lst_head); nmp_object_unref(obj_data->obj); - g_slice_free(RulesObjData, obj_data); + g_slice_free(TrackObjData, obj_data); } static guint -_rules_user_tag_hash(gconstpointer data) +_track_user_tag_data_hash(gconstpointer data) { - const RulesUserTagData *user_tag_data = data; + const TrackUserTagData *user_tag_data = data; return nm_hash_val(644693447u, user_tag_data->user_tag); } static gboolean -_rules_user_tag_equal(gconstpointer data_a, gconstpointer data_b) +_track_user_tag_data_equal(gconstpointer data_a, gconstpointer data_b) { - const RulesUserTagData *user_tag_data_a = data_a; - const RulesUserTagData *user_tag_data_b = data_b; + const TrackUserTagData *user_tag_data_a = data_a; + const TrackUserTagData *user_tag_data_b = data_b; return user_tag_data_a->user_tag == user_tag_data_b->user_tag; } static void -_rules_user_tag_destroy(gpointer data) +_track_user_tag_data_destroy(gpointer data) { - RulesUserTagData *user_tag_data = data; + TrackUserTagData *user_tag_data = data; c_list_unlink_stale(&user_tag_data->user_tag_lst_head); - g_slice_free(RulesUserTagData, user_tag_data); + g_slice_free(TrackUserTagData, user_tag_data); } -static RulesData * -_rules_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user_tag) +static TrackData * +_track_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user_tag) { - RulesData rules_data_needle = { + TrackData track_data_needle = { .obj = obj, .user_tag = user_tag, }; - return g_hash_table_lookup(by_data, &rules_data_needle); + return g_hash_table_lookup(by_data, &track_data_needle); } /** @@ -312,9 +312,9 @@ nmp_route_manager_track_rule(NMPRouteManager *self, { NMPObject obj_stack; const NMPObject *p_obj_stack; - RulesData *rules_data; - RulesObjData *obj_data; - RulesUserTagData *user_tag_data; + TrackData *track_data; + TrackObjData *obj_data; + TrackUserTagData *user_tag_data; gboolean changed = FALSE; guint32 track_priority_val; gboolean track_priority_present; @@ -336,11 +336,11 @@ nmp_route_manager_track_rule(NMPRouteManager *self, track_priority_present = FALSE; } - rules_data = _rules_data_lookup(self->by_data, p_obj_stack, user_tag); + track_data = _track_data_lookup(self->by_data, p_obj_stack, user_tag); - if (!rules_data) { - rules_data = g_slice_new(RulesData); - *rules_data = (RulesData){ + if (!track_data) { + track_data = g_slice_new(TrackData); + *track_data = (TrackData){ .obj = nm_dedup_multi_index_obj_intern(nm_platform_get_multi_idx(self->platform), p_obj_stack), .user_tag = user_tag, @@ -348,98 +348,98 @@ nmp_route_manager_track_rule(NMPRouteManager *self, .track_priority_present = track_priority_present, .dirty = FALSE, }; - g_hash_table_add(self->by_data, rules_data); + g_hash_table_add(self->by_data, track_data); - obj_data = g_hash_table_lookup(self->by_obj, &rules_data->obj); + obj_data = g_hash_table_lookup(self->by_obj, &track_data->obj); if (!obj_data) { - obj_data = g_slice_new(RulesObjData); - *obj_data = (RulesObjData){ - .obj = nmp_object_ref(rules_data->obj), + obj_data = g_slice_new(TrackObjData); + *obj_data = (TrackObjData){ + .obj = nmp_object_ref(track_data->obj), .obj_lst_head = C_LIST_INIT(obj_data->obj_lst_head), .config_state = CONFIG_STATE_NONE, }; g_hash_table_add(self->by_obj, obj_data); } - c_list_link_tail(&obj_data->obj_lst_head, &rules_data->obj_lst); + c_list_link_tail(&obj_data->obj_lst_head, &track_data->obj_lst); - user_tag_data = g_hash_table_lookup(self->by_user_tag, &rules_data->user_tag); + user_tag_data = g_hash_table_lookup(self->by_user_tag, &track_data->user_tag); if (!user_tag_data) { - user_tag_data = g_slice_new(RulesUserTagData); - *user_tag_data = (RulesUserTagData){ + user_tag_data = g_slice_new(TrackUserTagData); + *user_tag_data = (TrackUserTagData){ .user_tag = user_tag, .user_tag_lst_head = C_LIST_INIT(user_tag_data->user_tag_lst_head), }; g_hash_table_add(self->by_user_tag, user_tag_data); } - c_list_link_tail(&user_tag_data->user_tag_lst_head, &rules_data->user_tag_lst); + c_list_link_tail(&user_tag_data->user_tag_lst_head, &track_data->user_tag_lst); changed = TRUE; } else { - rules_data->dirty = FALSE; - if (rules_data->track_priority_val != track_priority_val - || rules_data->track_priority_present != track_priority_present) { - rules_data->track_priority_val = track_priority_val; - rules_data->track_priority_present = track_priority_present; + track_data->dirty = FALSE; + if (track_data->track_priority_val != track_priority_val + || track_data->track_priority_present != track_priority_present) { + track_data->track_priority_val = track_priority_val; + track_data->track_priority_present = track_priority_present; changed = TRUE; } } if (user_tag_untrack) { if (user_tag != user_tag_untrack) { - RulesData *rules_data_untrack; + TrackData *track_data_untrack; - rules_data_untrack = _rules_data_lookup(self->by_data, p_obj_stack, user_tag_untrack); - if (rules_data_untrack) - _rules_data_untrack(self, rules_data_untrack, FALSE, TRUE); + track_data_untrack = _track_data_lookup(self->by_data, p_obj_stack, user_tag_untrack); + if (track_data_untrack) + _track_data_untrack(self, track_data_untrack, FALSE, TRUE); } else nm_assert_not_reached(); } - _rules_data_assert(rules_data, TRUE); + _track_data_assert(track_data, TRUE); if (changed) { _LOGD("routing-rule: track [" NM_HASH_OBFUSCATE_PTR_FMT ",%s%u] \"%s\")", - _USER_TAG_LOG(rules_data->user_tag), - (rules_data->track_priority_val == 0 + _USER_TAG_LOG(track_data->user_tag), + (track_data->track_priority_val == 0 ? "" - : (rules_data->track_priority_present ? "+" : "-")), - (guint) rules_data->track_priority_val, - nmp_object_to_string(rules_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); + : (track_data->track_priority_present ? "+" : "-")), + (guint) track_data->track_priority_val, + nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); } } static void -_rules_data_untrack(NMPRouteManager *self, - RulesData *rules_data, +_track_data_untrack(NMPRouteManager *self, + TrackData *track_data, gboolean remove_user_tag_data, gboolean make_owned_by_us) { - RulesObjData *obj_data; + TrackObjData *obj_data; nm_assert(NMP_IS_ROUTE_MANAGER(self)); - _rules_data_assert(rules_data, TRUE); + _track_data_assert(track_data, TRUE); nm_assert(self->by_data); - nm_assert(g_hash_table_lookup(self->by_data, rules_data) == rules_data); + nm_assert(g_hash_table_lookup(self->by_data, track_data) == track_data); _LOGD("routing-rule: untrack [" NM_HASH_OBFUSCATE_PTR_FMT "] \"%s\"", - _USER_TAG_LOG(rules_data->user_tag), - nmp_object_to_string(rules_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); + _USER_TAG_LOG(track_data->user_tag), + nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); #if NM_MORE_ASSERTS { - RulesUserTagData *user_tag_data; + TrackUserTagData *user_tag_data; - user_tag_data = g_hash_table_lookup(self->by_user_tag, &rules_data->user_tag); + user_tag_data = g_hash_table_lookup(self->by_user_tag, &track_data->user_tag); nm_assert(user_tag_data); - nm_assert(c_list_contains(&user_tag_data->user_tag_lst_head, &rules_data->user_tag_lst)); + nm_assert(c_list_contains(&user_tag_data->user_tag_lst_head, &track_data->user_tag_lst)); } #endif - nm_assert(!c_list_is_empty(&rules_data->user_tag_lst)); + nm_assert(!c_list_is_empty(&track_data->user_tag_lst)); - obj_data = g_hash_table_lookup(self->by_obj, &rules_data->obj); + obj_data = g_hash_table_lookup(self->by_obj, &track_data->obj); nm_assert(obj_data); - nm_assert(c_list_contains(&obj_data->obj_lst_head, &rules_data->obj_lst)); - nm_assert(obj_data == g_hash_table_lookup(self->by_obj, &rules_data->obj)); + nm_assert(c_list_contains(&obj_data->obj_lst_head, &track_data->obj_lst)); + nm_assert(obj_data == g_hash_table_lookup(self->by_obj, &track_data->obj)); if (make_owned_by_us) { if (obj_data->config_state == CONFIG_STATE_NONE) { @@ -447,15 +447,15 @@ _rules_data_untrack(NMPRouteManager *self, * sync. */ obj_data->config_state = CONFIG_STATE_OWNED_BY_US; } - } else if (remove_user_tag_data && c_list_length_is(&rules_data->user_tag_lst, 1)) - g_hash_table_remove(self->by_user_tag, &rules_data->user_tag); + } else if (remove_user_tag_data && c_list_length_is(&track_data->user_tag_lst, 1)) + g_hash_table_remove(self->by_user_tag, &track_data->user_tag); /* if obj_data is marked to be "added_by_us" or "removed_by_us", we need to keep this entry * around for the next sync -- so that we can undo what we did earlier. */ - if (obj_data->config_state == CONFIG_STATE_NONE && c_list_length_is(&rules_data->obj_lst, 1)) - g_hash_table_remove(self->by_obj, &rules_data->obj); + if (obj_data->config_state == CONFIG_STATE_NONE && c_list_length_is(&track_data->obj_lst, 1)) + g_hash_table_remove(self->by_obj, &track_data->obj); - g_hash_table_remove(self->by_data, rules_data); + g_hash_table_remove(self->by_data, track_data); } void @@ -465,7 +465,7 @@ nmp_route_manager_untrack_rule(NMPRouteManager *self, { NMPObject obj_stack; const NMPObject *p_obj_stack; - RulesData *rules_data; + TrackData *track_data; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(routing_rule); @@ -475,16 +475,16 @@ nmp_route_manager_untrack_rule(NMPRouteManager *self, nm_assert(nmp_object_is_visible(p_obj_stack)); - rules_data = _rules_data_lookup(self->by_data, p_obj_stack, user_tag); - if (rules_data) - _rules_data_untrack(self, rules_data, TRUE, FALSE); + track_data = _track_data_lookup(self->by_data, p_obj_stack, user_tag); + if (track_data) + _track_data_untrack(self, track_data, TRUE, FALSE); } void nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) { - RulesData *rules_data; - RulesUserTagData *user_tag_data; + TrackData *track_data; + TrackUserTagData *user_tag_data; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); @@ -493,8 +493,8 @@ nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) if (!user_tag_data) return; - c_list_for_each_entry (rules_data, &user_tag_data->user_tag_lst_head, user_tag_lst) - rules_data->dirty = TRUE; + c_list_for_each_entry (track_data, &user_tag_data->user_tag_lst_head, user_tag_lst) + track_data->dirty = TRUE; } void @@ -502,9 +502,9 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, gboolean all /* or only dirty */) { - RulesData *rules_data; - RulesData *rules_data_safe; - RulesUserTagData *user_tag_data; + TrackData *track_data; + TrackData *track_data_safe; + TrackUserTagData *user_tag_data; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); g_return_if_fail(user_tag); @@ -513,12 +513,12 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, if (!user_tag_data) return; - c_list_for_each_entry_safe (rules_data, - rules_data_safe, + c_list_for_each_entry_safe (track_data, + track_data_safe, &user_tag_data->user_tag_lst_head, user_tag_lst) { - if (all || rules_data->dirty) - _rules_data_untrack(self, rules_data, FALSE, FALSE); + if (all || track_data->dirty) + _track_data_untrack(self, track_data, FALSE, FALSE); } if (c_list_is_empty(&user_tag_data->user_tag_lst_head)) g_hash_table_remove(self->by_user_tag, user_tag_data); @@ -531,10 +531,10 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) NMDedupMultiIter pl_iter; const NMPObject *plobj; gs_unref_ptrarray GPtrArray *rules_to_delete = NULL; - RulesObjData *obj_data; + TrackObjData *obj_data; GHashTableIter h_iter; guint i; - const RulesData *rd_best; + const TrackData *td_best; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); @@ -551,14 +551,14 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) continue; } - rd_best = _rules_obj_get_best_data(obj_data); - if (rd_best) { - if (rd_best->track_priority_present) { + td_best = _track_obj_data_get_best_data(obj_data); + if (td_best) { + if (td_best->track_priority_present) { if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US) obj_data->config_state = CONFIG_STATE_ADDED_BY_US; continue; } - if (rd_best->track_priority_val == 0) { + if (td_best->track_priority_val == 0) { if (!NM_IN_SET(obj_data->config_state, CONFIG_STATE_ADDED_BY_US, CONFIG_STATE_OWNED_BY_US)) { @@ -591,19 +591,19 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) g_hash_table_iter_init(&h_iter, self->by_obj); while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_data, NULL)) { - rd_best = _rules_obj_get_best_data(obj_data); + td_best = _track_obj_data_get_best_data(obj_data); - if (!rd_best) { + if (!td_best) { g_hash_table_iter_remove(&h_iter); continue; } - if (!rd_best->track_priority_present) { + if (!td_best->track_priority_present) { if (obj_data->config_state == CONFIG_STATE_OWNED_BY_US) obj_data->config_state = CONFIG_STATE_REMOVED_BY_US; continue; } - if (rd_best->track_priority_val == 0) { + if (td_best->track_priority_val == 0) { if (!NM_IN_SET(obj_data->config_state, CONFIG_STATE_REMOVED_BY_US, CONFIG_STATE_OWNED_BY_US)) { @@ -747,13 +747,15 @@ nmp_route_manager_new(NMPlatform *platform) .ref_count = 1, .platform = g_object_ref(platform), .by_data = - g_hash_table_new_full(_rules_data_hash, _rules_data_equal, NULL, _rules_data_destroy), - .by_obj = - g_hash_table_new_full(_rules_obj_hash, _rules_obj_equal, NULL, _rules_obj_destroy), - .by_user_tag = g_hash_table_new_full(_rules_user_tag_hash, - _rules_user_tag_equal, + g_hash_table_new_full(_track_data_hash, _track_data_equal, NULL, _track_data_destroy), + .by_obj = g_hash_table_new_full(_track_obj_data_hash, + _track_obj_data_equal, + NULL, + _track_obj_data_destroy), + .by_user_tag = g_hash_table_new_full(_track_user_tag_data_hash, + _track_user_tag_data_equal, NULL, - _rules_user_tag_destroy), + _track_user_tag_data_destroy), }; return self; } From 1baa301047f55c16a844e5b9c4a1d1cf415e88f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:43:22 +0100 Subject: [PATCH 17/33] platform: use __NMLOG_DEFAULT() in "nmp-route-manager.c" --- src/libnm-platform/nmp-route-manager.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index e7a9c005cd..7d5366878f 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -26,22 +26,7 @@ struct _NMPRouteManager { #define _NMLOG_DOMAIN LOGD_PLATFORM #define _NMLOG_PREFIX_NAME "route-manager" -#define _NMLOG(level, ...) \ - G_STMT_START \ - { \ - const NMLogLevel __level = (level); \ - \ - if (nm_logging_enabled(__level, _NMLOG_DOMAIN)) { \ - _nm_log(__level, \ - _NMLOG_DOMAIN, \ - 0, \ - NULL, \ - NULL, \ - "%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ - _NMLOG_PREFIX_NAME _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ - } \ - } \ - G_STMT_END +#define _NMLOG(level, ...) __NMLOG_DEFAULT(level, LOGD_PLATFORM, _NMLOG_PREFIX_NAME, __VA_ARGS__) /*****************************************************************************/ From 3e6c8d220a8dc0e10c427bb22d5aedb779448737 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:44:47 +0100 Subject: [PATCH 18/33] platform: use NM_HASH_OBFUSCATE_PTR() in "nmp-route-manager.c" NM_HASH_OBFUSCATE_PTR() is some snake-oil to not log raw pointer values. It obviously makes debugging harder. But we don't need to generate differently obfuscated pointer values. At least, let most users use the same obfuscation, so that the values are comparable. --- src/libnm-platform/nmp-route-manager.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index 7d5366878f..f13ee59aba 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -37,8 +37,6 @@ NMP_IS_ROUTE_MANAGER(gpointer self) && NM_IS_PLATFORM(((NMPRouteManager *) self)->platform); } -#define _USER_TAG_LOG(user_tag) nm_hash_obfuscate_ptr(1240261787u, (user_tag)) - /*****************************************************************************/ typedef struct { @@ -383,7 +381,7 @@ nmp_route_manager_track_rule(NMPRouteManager *self, if (changed) { _LOGD("routing-rule: track [" NM_HASH_OBFUSCATE_PTR_FMT ",%s%u] \"%s\")", - _USER_TAG_LOG(track_data->user_tag), + NM_HASH_OBFUSCATE_PTR(track_data->user_tag), (track_data->track_priority_val == 0 ? "" : (track_data->track_priority_present ? "+" : "-")), @@ -406,7 +404,7 @@ _track_data_untrack(NMPRouteManager *self, nm_assert(g_hash_table_lookup(self->by_data, track_data) == track_data); _LOGD("routing-rule: untrack [" NM_HASH_OBFUSCATE_PTR_FMT "] \"%s\"", - _USER_TAG_LOG(track_data->user_tag), + NM_HASH_OBFUSCATE_PTR(track_data->user_tag), nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); #if NM_MORE_ASSERTS From cfdecf5e96215d3a2956ac9169476deb862668ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:59:20 +0100 Subject: [PATCH 19/33] platform: use nm_g_slice_free() in "nmp-route-manager.c" --- src/libnm-platform/nmp-route-manager.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index f13ee59aba..b17b47d3ad 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -165,7 +165,7 @@ _track_data_destroy(gpointer data) c_list_unlink_stale(&track_data->obj_lst); c_list_unlink_stale(&track_data->user_tag_lst); nmp_object_unref(track_data->obj); - g_slice_free(TrackData, track_data); + nm_g_slice_free(track_data); } static const TrackData * @@ -227,7 +227,7 @@ _track_obj_data_destroy(gpointer data) c_list_unlink_stale(&obj_data->obj_lst_head); nmp_object_unref(obj_data->obj); - g_slice_free(TrackObjData, obj_data); + nm_g_slice_free(obj_data); } static guint @@ -253,7 +253,7 @@ _track_user_tag_data_destroy(gpointer data) TrackUserTagData *user_tag_data = data; c_list_unlink_stale(&user_tag_data->user_tag_lst_head); - g_slice_free(TrackUserTagData, user_tag_data); + nm_g_slice_free(user_tag_data); } static TrackData * @@ -763,5 +763,5 @@ nmp_route_manager_unref(NMPRouteManager *self) g_hash_table_destroy(self->by_obj); g_hash_table_destroy(self->by_data); g_object_unref(self->platform); - g_slice_free(NMPRouteManager, self); + nm_g_slice_free(self); } From 2e04d64232a589e0c7934d970dc8e079122b5951 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 18:05:35 +0100 Subject: [PATCH 20/33] platform: use nm_pdirect_{hash,equal}() in "nmp-route-manager.c" No need for a dedicated implementation just to compare two indirect pointers. --- src/libnm-platform/nmp-route-manager.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index b17b47d3ad..583e9bdf9d 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -230,23 +230,6 @@ _track_obj_data_destroy(gpointer data) nm_g_slice_free(obj_data); } -static guint -_track_user_tag_data_hash(gconstpointer data) -{ - const TrackUserTagData *user_tag_data = data; - - return nm_hash_val(644693447u, user_tag_data->user_tag); -} - -static gboolean -_track_user_tag_data_equal(gconstpointer data_a, gconstpointer data_b) -{ - const TrackUserTagData *user_tag_data_a = data_a; - const TrackUserTagData *user_tag_data_b = data_b; - - return user_tag_data_a->user_tag == user_tag_data_b->user_tag; -} - static void _track_user_tag_data_destroy(gpointer data) { @@ -725,6 +708,8 @@ nmp_route_manager_new(NMPlatform *platform) g_return_val_if_fail(NM_IS_PLATFORM(platform), NULL); + G_STATIC_ASSERT_EXPR(G_STRUCT_OFFSET(TrackUserTagData, user_tag) == 0); + self = g_slice_new(NMPRouteManager); *self = (NMPRouteManager){ .ref_count = 1, @@ -735,8 +720,8 @@ nmp_route_manager_new(NMPlatform *platform) _track_obj_data_equal, NULL, _track_obj_data_destroy), - .by_user_tag = g_hash_table_new_full(_track_user_tag_data_hash, - _track_user_tag_data_equal, + .by_user_tag = g_hash_table_new_full(nm_pdirect_hash, + nm_pdirect_equal, NULL, _track_user_tag_data_destroy), }; From 7c27c63bece5488a71950ce32178432d523c7ac3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 17:39:14 +0100 Subject: [PATCH 21/33] platform: extend NMPRouteManager to work for routes --- src/core/devices/nm-device.c | 4 +- src/core/platform/tests/test-route.c | 8 +- src/libnm-platform/nmp-route-manager.c | 136 ++++++++++++++++--------- src/libnm-platform/nmp-route-manager.h | 43 ++++++-- 4 files changed, 126 insertions(+), 65 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index f7836567a0..30e0fe8fef 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -9357,7 +9357,7 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) keep_deleted_rules = FALSE; if (set_mode == NM_TERNARY_DEFAULT) { /* when exiting NM, we leave the device up and the rules configured. - * We just all nmp_route_manager_sync_rules() to forget about the synced rules, + * We just call nmp_route_manager_sync() to forget about the synced rules, * but we don't actually delete them. * * FIXME: that is a problem after restart of NetworkManager, because these @@ -9371,7 +9371,7 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) * file and track them after restart again. */ keep_deleted_rules = TRUE; } - nmp_route_manager_sync_rules(route_manager, keep_deleted_rules); + nmp_route_manager_sync(route_manager, NMP_OBJECT_TYPE_ROUTING_RULE, keep_deleted_rules); } static gboolean diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index fb6ca72d32..cf0236e0e6 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -1675,14 +1675,14 @@ again: NULL); } if (nmtst_get_rand_uint32() % objs_sync->len == 0) { - nmp_route_manager_sync_rules(route_manager, FALSE); + nmp_route_manager_sync(route_manager, NMP_OBJECT_TYPE_ROUTING_RULE, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, i + 1); } } - nmp_route_manager_sync_rules(route_manager, FALSE); + nmp_route_manager_sync(route_manager, NMP_OBJECT_TYPE_ROUTING_RULE, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, objs_sync->len); @@ -1713,14 +1713,14 @@ again: break; } if (nmtst_get_rand_uint32() % objs_sync->len == 0) { - nmp_route_manager_sync_rules(route_manager, FALSE); + nmp_route_manager_sync(route_manager, NMP_OBJECT_TYPE_ROUTING_RULE, FALSE); g_assert_cmpint(nmtstp_platform_routing_rules_get_count(platform, AF_UNSPEC), ==, objs_sync->len - i - 1); } } - nmp_route_manager_sync_rules(route_manager, FALSE); + nmp_route_manager_sync(route_manager, NMP_OBJECT_TYPE_ROUTING_RULE, FALSE); } else { for (i = 0; i < objs->len;) { diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index 583e9bdf9d..c2a83de70a 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -116,7 +116,10 @@ static void _track_data_assert(const TrackData *track_data, gboolean linked) { nm_assert(track_data); - nm_assert(NMP_OBJECT_GET_TYPE(track_data->obj) == NMP_OBJECT_TYPE_ROUTING_RULE); + nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(track_data->obj), + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE, + NMP_OBJECT_TYPE_ROUTING_RULE)); nm_assert(nmp_object_is_visible(track_data->obj)); nm_assert(track_data->user_tag); nm_assert(!linked || !c_list_is_empty(&track_data->obj_lst)); @@ -132,9 +135,7 @@ _track_data_hash(gconstpointer data) _track_data_assert(track_data, FALSE); nm_hash_init(&h, 269297543u); - nm_platform_routing_rule_hash_update(NMP_OBJECT_CAST_ROUTING_RULE(track_data->obj), - NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID, - &h); + nmp_object_id_hash_update(track_data->obj, &h); nm_hash_update_val(&h, track_data->user_tag); return nm_hash_complete(&h); } @@ -149,10 +150,7 @@ _track_data_equal(gconstpointer data_a, gconstpointer data_b) _track_data_assert(track_data_b, FALSE); return track_data_a->user_tag == track_data_b->user_tag - && (nm_platform_routing_rule_cmp(NMP_OBJECT_CAST_ROUTING_RULE(track_data_a->obj), - NMP_OBJECT_CAST_ROUTING_RULE(track_data_b->obj), - NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID) - == 0); + && nmp_object_id_equal(track_data_a->obj, track_data_b->obj); } static void @@ -199,13 +197,8 @@ static guint _track_obj_data_hash(gconstpointer data) { const TrackObjData *obj_data = data; - NMHashState h; - nm_hash_init(&h, 432817559u); - nm_platform_routing_rule_hash_update(NMP_OBJECT_CAST_ROUTING_RULE(obj_data->obj), - NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID, - &h); - return nm_hash_complete(&h); + return nmp_object_id_hash(obj_data->obj); } static gboolean @@ -214,10 +207,7 @@ _track_obj_data_equal(gconstpointer data_a, gconstpointer data_b) const TrackObjData *obj_data_a = data_a; const TrackObjData *obj_data_b = data_b; - return (nm_platform_routing_rule_cmp(NMP_OBJECT_CAST_ROUTING_RULE(obj_data_a->obj), - NMP_OBJECT_CAST_ROUTING_RULE(obj_data_b->obj), - NM_PLATFORM_ROUTING_RULE_CMP_TYPE_ID) - == 0); + return nmp_object_id_equal(obj_data_a->obj, obj_data_b->obj); } static void @@ -250,10 +240,15 @@ _track_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user return g_hash_table_lookup(by_data, &track_data_needle); } +/*****************************************************************************/ + /** - * nmp_route_manager_track_rule: + * nmp_route_manager_track: * @self: the #NMPRouteManager instance - * @routing_rule: the #NMPlatformRoutingRule to track or untrack + * @obj_type: the NMPObjectType of @obj that we are tracking. + * @obj: the NMPlatformObject (of type NMPObjectType) to track. Usually + * a #NMPlatformRoutingRule, #NMPlatformIP4Route or #NMPlatformIP6Route + * pointer. * @track_priority: the priority for tracking the rule. Note that * negative values indicate a forced absence of the rule. Priorities * are compared with their absolute values (with higher absolute @@ -270,11 +265,12 @@ _track_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user * The purpose here is to set this to %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. */ void -nmp_route_manager_track_rule(NMPRouteManager *self, - const NMPlatformRoutingRule *routing_rule, - gint32 track_priority, - gconstpointer user_tag, - gconstpointer user_tag_untrack) +nmp_route_manager_track(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack) { NMPObject obj_stack; const NMPObject *p_obj_stack; @@ -286,11 +282,18 @@ nmp_route_manager_track_rule(NMPRouteManager *self, gboolean track_priority_present; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); - g_return_if_fail(routing_rule); + g_return_if_fail(obj); g_return_if_fail(user_tag); + + /* The route must not be tied to an interface. We can only handle here + * blackhole/unreachable/prohibit route types. */ + g_return_if_fail(obj_type == NMP_OBJECT_TYPE_ROUTING_RULE + || (NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE) + && ((const NMPlatformIPRoute *) obj)->ifindex == 0)); + nm_assert(track_priority != G_MININT32); - p_obj_stack = nmp_object_stackinit(&obj_stack, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule); + p_obj_stack = nmp_object_stackinit(&obj_stack, obj_type, obj); nm_assert(nmp_object_is_visible(p_obj_stack)); @@ -363,12 +366,13 @@ nmp_route_manager_track_rule(NMPRouteManager *self, _track_data_assert(track_data, TRUE); if (changed) { - _LOGD("routing-rule: track [" NM_HASH_OBFUSCATE_PTR_FMT ",%s%u] \"%s\")", + _LOGD("track [" NM_HASH_OBFUSCATE_PTR_FMT ",%s%u] %s \"%s\"", NM_HASH_OBFUSCATE_PTR(track_data->user_tag), (track_data->track_priority_val == 0 ? "" : (track_data->track_priority_present ? "+" : "-")), (guint) track_data->track_priority_val, + NMP_OBJECT_GET_CLASS(track_data->obj)->obj_type_name, nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); } } @@ -386,8 +390,9 @@ _track_data_untrack(NMPRouteManager *self, nm_assert(self->by_data); nm_assert(g_hash_table_lookup(self->by_data, track_data) == track_data); - _LOGD("routing-rule: untrack [" NM_HASH_OBFUSCATE_PTR_FMT "] \"%s\"", + _LOGD("untrack [" NM_HASH_OBFUSCATE_PTR_FMT "] %s \"%s\"", NM_HASH_OBFUSCATE_PTR(track_data->user_tag), + NMP_OBJECT_GET_CLASS(track_data->obj)->obj_type_name, nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); #if NM_MORE_ASSERTS @@ -425,19 +430,24 @@ _track_data_untrack(NMPRouteManager *self, } void -nmp_route_manager_untrack_rule(NMPRouteManager *self, - const NMPlatformRoutingRule *routing_rule, - gconstpointer user_tag) +nmp_route_manager_untrack(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gconstpointer user_tag) { NMPObject obj_stack; const NMPObject *p_obj_stack; TrackData *track_data; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); - g_return_if_fail(routing_rule); + nm_assert(NM_IN_SET(obj_type, + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE, + NMP_OBJECT_TYPE_ROUTING_RULE)); + g_return_if_fail(obj); g_return_if_fail(user_tag); - p_obj_stack = nmp_object_stackinit(&obj_stack, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule); + p_obj_stack = nmp_object_stackinit(&obj_stack, obj_type, obj); nm_assert(nmp_object_is_visible(p_obj_stack)); @@ -490,29 +500,41 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, g_hash_table_remove(self->by_user_tag, user_tag_data); } +/*****************************************************************************/ + void -nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) +nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean keep_deleted) { const NMDedupMultiHeadEntry *pl_head_entry; NMDedupMultiIter pl_iter; const NMPObject *plobj; - gs_unref_ptrarray GPtrArray *rules_to_delete = NULL; + gs_unref_ptrarray GPtrArray *objs_to_delete = NULL; TrackObjData *obj_data; GHashTableIter h_iter; guint i; const TrackData *td_best; g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); + g_return_if_fail(NM_IN_SET(obj_type, + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE, + NMP_OBJECT_TYPE_ROUTING_RULE)); - _LOGD("sync%s", keep_deleted_rules ? " (don't remove any rules)" : ""); + _LOGD("sync %s%s", + nmp_class_from_type(obj_type)->obj_type_name, + keep_deleted ? " (don't remove any)" : ""); + + if (obj_type == NMP_OBJECT_TYPE_ROUTING_RULE) + pl_head_entry = nm_platform_lookup_obj_type(self->platform, obj_type); + else + pl_head_entry = nm_platform_lookup_object(self->platform, obj_type, 0); - pl_head_entry = nm_platform_lookup_obj_type(self->platform, NMP_OBJECT_TYPE_ROUTING_RULE); if (pl_head_entry) { nmp_cache_iter_for_each (&pl_iter, pl_head_entry, &plobj) { obj_data = g_hash_table_lookup(self->by_obj, &plobj); if (!obj_data) { - /* this rule is not tracked. It was externally added, hence we + /* this obj is not tracked. It was externally added, hence we * ignore it. */ continue; } @@ -535,28 +557,36 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) } } - if (keep_deleted_rules) { - _LOGD("forget/leak rule added by us: %s", + if (keep_deleted) { + _LOGD("forget/leak object added by us: %s \"%s\"", + NMP_OBJECT_GET_CLASS(plobj)->obj_type_name, nmp_object_to_string(plobj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); continue; } - if (!rules_to_delete) - rules_to_delete = g_ptr_array_new_with_free_func((GDestroyNotify) nmp_object_unref); + if (!objs_to_delete) + objs_to_delete = g_ptr_array_new_with_free_func((GDestroyNotify) nmp_object_unref); - g_ptr_array_add(rules_to_delete, (gpointer) nmp_object_ref(plobj)); + g_ptr_array_add(objs_to_delete, (gpointer) nmp_object_ref(plobj)); obj_data->config_state = CONFIG_STATE_REMOVED_BY_US; } } - if (rules_to_delete) { - for (i = 0; i < rules_to_delete->len; i++) - nm_platform_object_delete(self->platform, rules_to_delete->pdata[i]); + if (objs_to_delete) { + for (i = 0; i < objs_to_delete->len; i++) + nm_platform_object_delete(self->platform, objs_to_delete->pdata[i]); } g_hash_table_iter_init(&h_iter, self->by_obj); while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_data, NULL)) { + if (NMP_OBJECT_GET_TYPE(obj_data->obj) != obj_type) { + /* Here we need to iterate over all objects (rules and ip4/ip6 routes) + * and skip over the non-interesting ones. It might be better to + * track 3 separate CList by object type. */ + continue; + } + td_best = _track_obj_data_get_best_data(obj_data); if (!td_best) { @@ -585,12 +615,18 @@ nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules) continue; obj_data->config_state = CONFIG_STATE_ADDED_BY_US; - nm_platform_routing_rule_add(self->platform, - NMP_NLM_FLAG_ADD, - NMP_OBJECT_CAST_ROUTING_RULE(obj_data->obj)); + + if (obj_type == NMP_OBJECT_TYPE_ROUTING_RULE) { + nm_platform_routing_rule_add(self->platform, + NMP_NLM_FLAG_ADD, + NMP_OBJECT_CAST_ROUTING_RULE(obj_data->obj)); + } else + nm_platform_ip_route_add(self->platform, NMP_NLM_FLAG_APPEND, obj_data->obj); } } +/*****************************************************************************/ + void nmp_route_manager_track_rule_from_platform(NMPRouteManager *self, NMPlatform *platform, diff --git a/src/libnm-platform/nmp-route-manager.h b/src/libnm-platform/nmp-route-manager.h index 3bc4a49301..17af8ee0c5 100644 --- a/src/libnm-platform/nmp-route-manager.h +++ b/src/libnm-platform/nmp-route-manager.h @@ -19,11 +19,27 @@ void nmp_route_manager_unref(NMPRouteManager *self); #define nm_auto_unref_route_manager nm_auto(_nmp_route_manager_unref) NM_AUTO_DEFINE_FCN0(NMPRouteManager *, _nmp_route_manager_unref, nmp_route_manager_unref); -void nmp_route_manager_track_rule(NMPRouteManager *self, - const NMPlatformRoutingRule *routing_rule, - gint32 track_priority, - gconstpointer user_tag, - gconstpointer user_tag_untrack); +void nmp_route_manager_track(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack); + +static inline void +nmp_route_manager_track_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack) +{ + nmp_route_manager_track(self, + NMP_OBJECT_TYPE_ROUTING_RULE, + routing_rule, + track_priority, + user_tag, + user_tag_untrack); +} void nmp_route_manager_track_rule_default(NMPRouteManager *self, int addr_family, @@ -36,9 +52,18 @@ void nmp_route_manager_track_rule_from_platform(NMPRouteManager *self, gint32 tracking_priority, gconstpointer user_tag); -void nmp_route_manager_untrack_rule(NMPRouteManager *self, - const NMPlatformRoutingRule *routing_rule, - gconstpointer user_tag); +void nmp_route_manager_untrack(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gconstpointer user_tag); + +static inline void +nmp_route_manager_untrack_rule(NMPRouteManager *self, + const NMPlatformRoutingRule *routing_rule, + gconstpointer user_tag) +{ + nmp_route_manager_untrack(self, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule, user_tag); +} void nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag); @@ -46,7 +71,7 @@ void nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, gboolean all /* or only dirty */); -void nmp_route_manager_sync_rules(NMPRouteManager *self, gboolean keep_deleted_rules); +void nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean keep_deleted); /*****************************************************************************/ From f315ca9e8436c9a16cfaa31d041a762ef506c96a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Feb 2022 18:55:34 +0100 Subject: [PATCH 22/33] platform: track linked list of objects in NMPRouteManager by type We now track up to three kinds of object types in NMPRouteManager. There is only one place, where we need to iterate over all objects of the same type (e.g. all ipv4-routes), and that is nmp_route_manager_sync(). Previously, we only had one GHashTable with all the object, and when iterating we had to skip over them after checking the type. That has some overhead, but OK. The ugliness with iterating over a GHashTable is that the order is non deterministic. We should have a defined order in which things happen. To achieve that, track three different CList, one for each object type. Also, I expect that to be slightly faster, as you only have to iterate over the list you care about. --- src/libnm-platform/nmp-route-manager.c | 51 ++++++++++++++++++++------ 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index c2a83de70a..a4cbf2e8bb 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -18,6 +18,7 @@ struct _NMPRouteManager { GHashTable *by_obj; GHashTable *by_user_tag; GHashTable *by_data; + CList by_obj_lst_heads[3]; guint ref_count; }; @@ -85,6 +86,8 @@ typedef struct { const NMPObject *obj; CList obj_lst_head; + CList by_obj_lst; + /* indicates whether we configured/removed the rule (during sync()). We need that, so * if the rule gets untracked, that we know to remove/restore it. * @@ -112,6 +115,25 @@ static void _track_data_untrack(NMPRouteManager *self, /*****************************************************************************/ +static CList * +_by_obj_lst_head(NMPRouteManager *self, NMPObjectType obj_type) +{ + G_STATIC_ASSERT(G_N_ELEMENTS(self->by_obj_lst_heads) == 3); + + switch (obj_type) { + case NMP_OBJECT_TYPE_IP4_ROUTE: + return &self->by_obj_lst_heads[0]; + case NMP_OBJECT_TYPE_IP6_ROUTE: + return &self->by_obj_lst_heads[1]; + case NMP_OBJECT_TYPE_ROUTING_RULE: + return &self->by_obj_lst_heads[2]; + default: + return nm_assert_unreachable_val(NULL); + } +} + +/*****************************************************************************/ + static void _track_data_assert(const TrackData *track_data, gboolean linked) { @@ -216,6 +238,7 @@ _track_obj_data_destroy(gpointer data) TrackObjData *obj_data = data; c_list_unlink_stale(&obj_data->obj_lst_head); + c_list_unlink_stale(&obj_data->by_obj_lst); nmp_object_unref(obj_data->obj); nm_g_slice_free(obj_data); } @@ -328,6 +351,7 @@ nmp_route_manager_track(NMPRouteManager *self, .config_state = CONFIG_STATE_NONE, }; g_hash_table_add(self->by_obj, obj_data); + c_list_link_tail(_by_obj_lst_head(self, obj_type), &obj_data->by_obj_lst); } c_list_link_tail(&obj_data->obj_lst_head, &track_data->obj_lst); @@ -510,7 +534,8 @@ nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean k const NMPObject *plobj; gs_unref_ptrarray GPtrArray *objs_to_delete = NULL; TrackObjData *obj_data; - GHashTableIter h_iter; + TrackObjData *obj_data_safe; + CList *by_obj_lst_head; guint i; const TrackData *td_best; @@ -578,19 +603,15 @@ nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean k nm_platform_object_delete(self->platform, objs_to_delete->pdata[i]); } - g_hash_table_iter_init(&h_iter, self->by_obj); - while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_data, NULL)) { - if (NMP_OBJECT_GET_TYPE(obj_data->obj) != obj_type) { - /* Here we need to iterate over all objects (rules and ip4/ip6 routes) - * and skip over the non-interesting ones. It might be better to - * track 3 separate CList by object type. */ - continue; - } + by_obj_lst_head = _by_obj_lst_head(self, obj_type); + + c_list_for_each_entry_safe (obj_data, obj_data_safe, by_obj_lst_head, by_obj_lst) { + nm_assert(NMP_OBJECT_GET_TYPE(obj_data->obj) == obj_type); td_best = _track_obj_data_get_best_data(obj_data); if (!td_best) { - g_hash_table_iter_remove(&h_iter); + g_hash_table_remove(self->by_obj, obj_data); continue; } @@ -752,14 +773,17 @@ nmp_route_manager_new(NMPlatform *platform) .platform = g_object_ref(platform), .by_data = g_hash_table_new_full(_track_data_hash, _track_data_equal, NULL, _track_data_destroy), - .by_obj = g_hash_table_new_full(_track_obj_data_hash, + .by_obj = g_hash_table_new_full(_track_obj_data_hash, _track_obj_data_equal, NULL, _track_obj_data_destroy), - .by_user_tag = g_hash_table_new_full(nm_pdirect_hash, + .by_user_tag = g_hash_table_new_full(nm_pdirect_hash, nm_pdirect_equal, NULL, _track_user_tag_data_destroy), + .by_obj_lst_heads[0] = C_LIST_INIT(self->by_obj_lst_heads[0]), + .by_obj_lst_heads[1] = C_LIST_INIT(self->by_obj_lst_heads[1]), + .by_obj_lst_heads[2] = C_LIST_INIT(self->by_obj_lst_heads[2]), }; return self; } @@ -783,6 +807,9 @@ nmp_route_manager_unref(NMPRouteManager *self) g_hash_table_destroy(self->by_user_tag); g_hash_table_destroy(self->by_obj); g_hash_table_destroy(self->by_data); + nm_assert(c_list_is_empty(&self->by_obj_lst_heads[0])); + nm_assert(c_list_is_empty(&self->by_obj_lst_heads[1])); + nm_assert(c_list_is_empty(&self->by_obj_lst_heads[2])); g_object_unref(self->platform); nm_g_slice_free(self); } From 81f6ba83776a89036983ec226932f5730fa52032 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 19:16:31 +0100 Subject: [PATCH 23/33] platform: return self from nmp_route_manager_ref() It's just more convenient. --- src/libnm-platform/nmp-route-manager.c | 5 +++-- src/libnm-platform/nmp-route-manager.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index a4cbf2e8bb..6828bb57f9 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -788,12 +788,13 @@ nmp_route_manager_new(NMPlatform *platform) return self; } -void +NMPRouteManager * nmp_route_manager_ref(NMPRouteManager *self) { - g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); + g_return_val_if_fail(NMP_IS_ROUTE_MANAGER(self), NULL); self->ref_count++; + return self; } void diff --git a/src/libnm-platform/nmp-route-manager.h b/src/libnm-platform/nmp-route-manager.h index 17af8ee0c5..23e4c2d341 100644 --- a/src/libnm-platform/nmp-route-manager.h +++ b/src/libnm-platform/nmp-route-manager.h @@ -13,8 +13,8 @@ typedef struct _NMPRouteManager NMPRouteManager; NMPRouteManager *nmp_route_manager_new(NMPlatform *platform); -void nmp_route_manager_ref(NMPRouteManager *self); -void nmp_route_manager_unref(NMPRouteManager *self); +NMPRouteManager *nmp_route_manager_ref(NMPRouteManager *self); +void nmp_route_manager_unref(NMPRouteManager *self); #define nm_auto_unref_route_manager nm_auto(_nmp_route_manager_unref) NM_AUTO_DEFINE_FCN0(NMPRouteManager *, _nmp_route_manager_unref, nmp_route_manager_unref); From 5489aa596b23890480bc64657c96e2997a762f06 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 19:23:27 +0100 Subject: [PATCH 24/33] platform: return boolean changed value from nmp_route_manager_track() --- src/libnm-platform/nmp-route-manager.c | 57 +++++++++++++++++--------- src/libnm-platform/nmp-route-manager.h | 44 ++++++++++---------- 2 files changed, 60 insertions(+), 41 deletions(-) diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index 6828bb57f9..c5ca02600c 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -286,8 +286,10 @@ _track_data_lookup(GHashTable *by_data, const NMPObject *obj, gconstpointer user * because it enforces ownership of the now tracked rule. On the other hand, * a plain nmp_route_manager_untrack_rule() merely forgets about the tracking. * The purpose here is to set this to %NMP_ROUTE_MANAGER_EXTERN_WEAKLY_TRACKED_USER_TAG. + * + * Returns: %TRUE, if something changed. */ -void +gboolean nmp_route_manager_track(NMPRouteManager *self, NMPObjectType obj_type, gconstpointer obj, @@ -300,19 +302,22 @@ nmp_route_manager_track(NMPRouteManager *self, TrackData *track_data; TrackObjData *obj_data; TrackUserTagData *user_tag_data; - gboolean changed = FALSE; + gboolean changed = FALSE; + gboolean changed_untrack = FALSE; guint32 track_priority_val; gboolean track_priority_present; - g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); - g_return_if_fail(obj); - g_return_if_fail(user_tag); + g_return_val_if_fail(NMP_IS_ROUTE_MANAGER(self), FALSE); + g_return_val_if_fail(obj, FALSE); + g_return_val_if_fail(user_tag, FALSE); /* The route must not be tied to an interface. We can only handle here * blackhole/unreachable/prohibit route types. */ - g_return_if_fail(obj_type == NMP_OBJECT_TYPE_ROUTING_RULE - || (NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE) - && ((const NMPlatformIPRoute *) obj)->ifindex == 0)); + g_return_val_if_fail( + obj_type == NMP_OBJECT_TYPE_ROUTING_RULE + || (NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE) + && ((const NMPlatformIPRoute *) obj)->ifindex == 0), + FALSE); nm_assert(track_priority != G_MININT32); @@ -381,8 +386,10 @@ nmp_route_manager_track(NMPRouteManager *self, TrackData *track_data_untrack; track_data_untrack = _track_data_lookup(self->by_data, p_obj_stack, user_tag_untrack); - if (track_data_untrack) + if (track_data_untrack) { _track_data_untrack(self, track_data_untrack, FALSE, TRUE); + changed_untrack = TRUE; + } } else nm_assert_not_reached(); } @@ -399,6 +406,8 @@ nmp_route_manager_track(NMPRouteManager *self, NMP_OBJECT_GET_CLASS(track_data->obj)->obj_type_name, nmp_object_to_string(track_data->obj, NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); } + + return changed || changed_untrack; } static void @@ -453,7 +462,7 @@ _track_data_untrack(NMPRouteManager *self, g_hash_table_remove(self->by_data, track_data); } -void +gboolean nmp_route_manager_untrack(NMPRouteManager *self, NMPObjectType obj_type, gconstpointer obj, @@ -462,22 +471,27 @@ nmp_route_manager_untrack(NMPRouteManager *self, NMPObject obj_stack; const NMPObject *p_obj_stack; TrackData *track_data; + gboolean changed = FALSE; - g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); + g_return_val_if_fail(NMP_IS_ROUTE_MANAGER(self), FALSE); nm_assert(NM_IN_SET(obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE, NMP_OBJECT_TYPE_ROUTING_RULE)); - g_return_if_fail(obj); - g_return_if_fail(user_tag); + g_return_val_if_fail(obj, FALSE); + g_return_val_if_fail(user_tag, FALSE); p_obj_stack = nmp_object_stackinit(&obj_stack, obj_type, obj); nm_assert(nmp_object_is_visible(p_obj_stack)); track_data = _track_data_lookup(self->by_data, p_obj_stack, user_tag); - if (track_data) + if (track_data) { _track_data_untrack(self, track_data, TRUE, FALSE); + changed = TRUE; + } + + return changed; } void @@ -497,7 +511,7 @@ nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) track_data->dirty = TRUE; } -void +gboolean nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, gboolean all /* or only dirty */) @@ -505,23 +519,28 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, TrackData *track_data; TrackData *track_data_safe; TrackUserTagData *user_tag_data; + gboolean changed = FALSE; - g_return_if_fail(NMP_IS_ROUTE_MANAGER(self)); - g_return_if_fail(user_tag); + g_return_val_if_fail(NMP_IS_ROUTE_MANAGER(self), FALSE); + g_return_val_if_fail(user_tag, FALSE); user_tag_data = g_hash_table_lookup(self->by_user_tag, &user_tag); if (!user_tag_data) - return; + return FALSE; c_list_for_each_entry_safe (track_data, track_data_safe, &user_tag_data->user_tag_lst_head, user_tag_lst) { - if (all || track_data->dirty) + if (all || track_data->dirty) { _track_data_untrack(self, track_data, FALSE, FALSE); + changed = TRUE; + } } if (c_list_is_empty(&user_tag_data->user_tag_lst_head)) g_hash_table_remove(self->by_user_tag, user_tag_data); + + return changed; } /*****************************************************************************/ diff --git a/src/libnm-platform/nmp-route-manager.h b/src/libnm-platform/nmp-route-manager.h index 23e4c2d341..a61a68ca98 100644 --- a/src/libnm-platform/nmp-route-manager.h +++ b/src/libnm-platform/nmp-route-manager.h @@ -19,26 +19,26 @@ void nmp_route_manager_unref(NMPRouteManager *self); #define nm_auto_unref_route_manager nm_auto(_nmp_route_manager_unref) NM_AUTO_DEFINE_FCN0(NMPRouteManager *, _nmp_route_manager_unref, nmp_route_manager_unref); -void nmp_route_manager_track(NMPRouteManager *self, - NMPObjectType obj_type, - gconstpointer obj, - gint32 track_priority, - gconstpointer user_tag, - gconstpointer user_tag_untrack); +gboolean nmp_route_manager_track(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gint32 track_priority, + gconstpointer user_tag, + gconstpointer user_tag_untrack); -static inline void +static inline gboolean nmp_route_manager_track_rule(NMPRouteManager *self, const NMPlatformRoutingRule *routing_rule, gint32 track_priority, gconstpointer user_tag, gconstpointer user_tag_untrack) { - nmp_route_manager_track(self, - NMP_OBJECT_TYPE_ROUTING_RULE, - routing_rule, - track_priority, - user_tag, - user_tag_untrack); + return nmp_route_manager_track(self, + NMP_OBJECT_TYPE_ROUTING_RULE, + routing_rule, + track_priority, + user_tag, + user_tag_untrack); } void nmp_route_manager_track_rule_default(NMPRouteManager *self, @@ -52,24 +52,24 @@ void nmp_route_manager_track_rule_from_platform(NMPRouteManager *self, gint32 tracking_priority, gconstpointer user_tag); -void nmp_route_manager_untrack(NMPRouteManager *self, - NMPObjectType obj_type, - gconstpointer obj, - gconstpointer user_tag); +gboolean nmp_route_manager_untrack(NMPRouteManager *self, + NMPObjectType obj_type, + gconstpointer obj, + gconstpointer user_tag); -static inline void +static inline gboolean nmp_route_manager_untrack_rule(NMPRouteManager *self, const NMPlatformRoutingRule *routing_rule, gconstpointer user_tag) { - nmp_route_manager_untrack(self, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule, user_tag); + return nmp_route_manager_untrack(self, NMP_OBJECT_TYPE_ROUTING_RULE, routing_rule, user_tag); } void nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag); -void nmp_route_manager_untrack_all(NMPRouteManager *self, - gconstpointer user_tag, - gboolean all /* or only dirty */); +gboolean nmp_route_manager_untrack_all(NMPRouteManager *self, + gconstpointer user_tag, + gboolean all /* or only dirty */); void nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean keep_deleted); From 9e90bb081786c4e525b2cc25bdef63f634778b31 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Feb 2022 11:56:17 +0100 Subject: [PATCH 25/33] platform: improve way to prune dirty route-manager entries The general idea is that when we have entries tracked by the route-manager, that we can mark them all as dirty. Then, calling the "track" function will reset the dirty flag. Finally, there is a method to delete all dirty entries. As we can lookup an entry with O(1) (using dictionaries), we can sync the list of tracked objects with O(n). We just need to track all the ones we care about, and then delete those that were not touched (that is, are still dirty). Previously, we had to explicitly mark all entries as dirty. We can do better. Just let nmp_route_manager_untrack_all() mark the survivors as dirty right away. This way, we can save iterating the list once. It also makes sense because the only purpose of the dirty flag is to aid this prune mechanism with track/untrack-all. So, untrack-all can just help out, and leave the remaining entries dirty, so that the next track does the right thing. --- src/core/devices/nm-device.c | 7 ++----- src/libnm-platform/nmp-route-manager.c | 6 +++++- src/libnm-platform/nmp-route-manager.h | 3 ++- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 30e0fe8fef..5cb3d27065 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -9301,9 +9301,6 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) int is_ipv4; untrack_only_dirty = TRUE; - nmp_route_manager_set_dirty(route_manager, user_tag_1); - if (klass->get_extra_rules) - nmp_route_manager_set_dirty(route_manager, user_tag_2); applied_connection = nm_device_get_applied_connection(self); @@ -9350,9 +9347,9 @@ _routing_rules_sync(NMDevice *self, NMTernary set_mode) } } - nmp_route_manager_untrack_all(route_manager, user_tag_1, !untrack_only_dirty); + nmp_route_manager_untrack_all(route_manager, user_tag_1, !untrack_only_dirty, TRUE); if (klass->get_extra_rules) - nmp_route_manager_untrack_all(route_manager, user_tag_2, !untrack_only_dirty); + nmp_route_manager_untrack_all(route_manager, user_tag_2, !untrack_only_dirty, TRUE); keep_deleted_rules = FALSE; if (set_mode == NM_TERNARY_DEFAULT) { diff --git a/src/libnm-platform/nmp-route-manager.c b/src/libnm-platform/nmp-route-manager.c index c5ca02600c..c31c9806a5 100644 --- a/src/libnm-platform/nmp-route-manager.c +++ b/src/libnm-platform/nmp-route-manager.c @@ -514,7 +514,8 @@ nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag) gboolean nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, - gboolean all /* or only dirty */) + gboolean all /* or only dirty */, + gboolean make_survivors_dirty) { TrackData *track_data; TrackData *track_data_safe; @@ -535,7 +536,10 @@ nmp_route_manager_untrack_all(NMPRouteManager *self, if (all || track_data->dirty) { _track_data_untrack(self, track_data, FALSE, FALSE); changed = TRUE; + continue; } + if (make_survivors_dirty) + track_data->dirty = TRUE; } if (c_list_is_empty(&user_tag_data->user_tag_lst_head)) g_hash_table_remove(self->by_user_tag, user_tag_data); diff --git a/src/libnm-platform/nmp-route-manager.h b/src/libnm-platform/nmp-route-manager.h index a61a68ca98..97ec3840df 100644 --- a/src/libnm-platform/nmp-route-manager.h +++ b/src/libnm-platform/nmp-route-manager.h @@ -69,7 +69,8 @@ void nmp_route_manager_set_dirty(NMPRouteManager *self, gconstpointer user_tag); gboolean nmp_route_manager_untrack_all(NMPRouteManager *self, gconstpointer user_tag, - gboolean all /* or only dirty */); + gboolean all /* or only dirty */, + gboolean make_survivors_dirty); void nmp_route_manager_sync(NMPRouteManager *self, NMPObjectType obj_type, gboolean keep_deleted); From e32bc6d248fa09d1652465577af4fd03eb39e0f1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 19:33:06 +0100 Subject: [PATCH 26/33] core/l3cfg: rework generating list of routes in _l3_commit_one() This will be required next, when we will have also routes without a device. Split the generation of the route list out. --- src/core/nm-l3cfg.c | 101 ++++++++++++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index aed4cf05c9..dfb80c7cb9 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -1008,18 +1008,11 @@ typedef struct { } ObjStatesSyncFilterData; static gboolean -_obj_states_sync_filter(/* const NMDedupMultiObj * */ gconstpointer o, gpointer user_data) +_obj_states_sync_filter(NML3Cfg *self, const NMPObject *obj, NML3CfgCommitType commit_type) { - char sbuf[sizeof(_nm_utils_to_string_buffer)]; - const NMPObject *obj = o; - const ObjStatesSyncFilterData *sync_filter_data = user_data; - NMPObjectType obj_type; - ObjStateData *obj_state; - NML3Cfg *self; - - nm_assert(sync_filter_data); - nm_assert(NM_IS_L3CFG(sync_filter_data->self)); - self = sync_filter_data->self; + char sbuf[sizeof(_nm_utils_to_string_buffer)]; + NMPObjectType obj_type; + ObjStateData *obj_state; obj_type = NMP_OBJECT_GET_TYPE(obj); @@ -1027,14 +1020,14 @@ _obj_states_sync_filter(/* const NMDedupMultiObj * */ gconstpointer o, gpointer && NMP_OBJECT_CAST_IP4_ADDRESS(obj)->a_acd_not_ready) return FALSE; - obj_state = g_hash_table_lookup(sync_filter_data->self->priv.p->obj_state_hash, &obj); + obj_state = g_hash_table_lookup(self->priv.p->obj_state_hash, &obj); - nm_assert_obj_state(sync_filter_data->self, obj_state); + nm_assert_obj_state(self, obj_state); nm_assert(obj_state->obj == obj); nm_assert(c_list_is_empty(&obj_state->os_zombie_lst)); if (!obj_state->os_nm_configured) { - if (sync_filter_data->commit_type == NM_L3_CFG_COMMIT_TYPE_ASSUME + if (commit_type == NM_L3_CFG_COMMIT_TYPE_ASSUME && !_obj_state_data_get_assume_config_once(obj_state)) return FALSE; @@ -1051,13 +1044,72 @@ _obj_states_sync_filter(/* const NMDedupMultiObj * */ gconstpointer o, gpointer return TRUE; } - if (!obj_state->os_plobj && sync_filter_data->commit_type != NM_L3_CFG_COMMIT_TYPE_REAPPLY + if (!obj_state->os_plobj && commit_type != NM_L3_CFG_COMMIT_TYPE_REAPPLY && !nmp_object_get_force_commit(obj)) return FALSE; return TRUE; } +static gboolean +_obj_states_sync_filter_predicate(gconstpointer o, gpointer user_data) +{ + const NMPObject *obj = o; + const ObjStatesSyncFilterData *sync_filter_data = user_data; + + return _obj_states_sync_filter(sync_filter_data->self, obj, sync_filter_data->commit_type); +} + +static GPtrArray * +_commit_collect_addresses(NML3Cfg *self, int addr_family, NML3CfgCommitType commit_type) +{ + const int IS_IPv4 = NM_IS_IPv4(addr_family); + const NMDedupMultiHeadEntry *head_entry; + const ObjStatesSyncFilterData sync_filter_data = { + .self = self, + .commit_type = commit_type, + }; + + head_entry = nm_l3_config_data_lookup_objs(self->priv.p->combined_l3cd_commited, + NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4)); + return nm_dedup_multi_objs_to_ptr_array_head(head_entry, + _obj_states_sync_filter_predicate, + (gpointer) &sync_filter_data); +} + +static void +_commit_collect_routes(NML3Cfg *self, + int addr_family, + NML3CfgCommitType commit_type, + GPtrArray **routes) +{ + const int IS_IPv4 = NM_IS_IPv4(addr_family); + const NMDedupMultiHeadEntry *head_entry; + const NMDedupMultiEntry *entry; + + nm_assert(routes && !*routes); + + head_entry = nm_l3_config_data_lookup_objs(self->priv.p->combined_l3cd_commited, + NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4)); + + if (!head_entry) + return; + + c_list_for_each_entry (entry, &head_entry->lst_entries_head, lst_entries) { + const NMPObject *obj = entry->obj; + + if (!_obj_states_sync_filter(self, obj, commit_type)) + continue; + + if (!*routes) { + *routes = + g_ptr_array_new_full(head_entry->len, (GDestroyNotify) nm_dedup_multi_obj_unref); + } + + g_ptr_array_add(*routes, (gpointer) nmp_object_ref(obj)); + } +} + static void _obj_state_zombie_lst_get_prune_lists(NML3Cfg *self, int addr_family, @@ -3789,6 +3841,7 @@ out_prune: } /*****************************************************************************/ + static const char * ip6_privacy_to_str(NMSettingIP6ConfigPrivacy ip6_privacy) { @@ -4070,23 +4123,9 @@ _l3_commit_one(NML3Cfg *self, _l3_cfg_commit_type_to_string(commit_type, sbuf_commit_type, sizeof(sbuf_commit_type))); if (self->priv.p->combined_l3cd_commited) { - const NMDedupMultiHeadEntry *head_entry; - const ObjStatesSyncFilterData sync_filter_data = { - .self = self, - .commit_type = commit_type, - }; + addresses = _commit_collect_addresses(self, addr_family, commit_type); - head_entry = nm_l3_config_data_lookup_objs(self->priv.p->combined_l3cd_commited, - NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4)); - addresses = nm_dedup_multi_objs_to_ptr_array_head(head_entry, - _obj_states_sync_filter, - (gpointer) &sync_filter_data); - - head_entry = nm_l3_config_data_lookup_objs(self->priv.p->combined_l3cd_commited, - NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4)); - routes = nm_dedup_multi_objs_to_ptr_array_head(head_entry, - _obj_states_sync_filter, - (gpointer) &sync_filter_data); + _commit_collect_routes(self, addr_family, commit_type, &routes); route_table_sync = nm_l3_config_data_get_route_table_sync(self->priv.p->combined_l3cd_commited, From 6255e0dcac06111a121541f982d11eefbae20cb1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Feb 2022 23:53:30 +0100 Subject: [PATCH 27/33] core: handle blackhole/unreachable/prohibit route types in core Specifically, in nm_utils_ip_route_attribute_to_platform() and in _l3_config_data_add_obj() handle such new route type. For the moment, they cannot be stored in a valid NMSettingIPConfig, but later this will be necessary. --- src/core/NetworkManagerUtils.c | 3 ++- src/core/nm-l3-config-data.c | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/core/NetworkManagerUtils.c b/src/core/NetworkManagerUtils.c index 14f9b453c8..c50dbadad2 100644 --- a/src/core/NetworkManagerUtils.c +++ b/src/core/NetworkManagerUtils.c @@ -1347,7 +1347,8 @@ nm_utils_ip_route_attribute_to_platform(int addr_family, int type; type = nm_net_aux_rtnl_rtntype_a2n(g_variant_get_string(variant, NULL)); - nm_assert(NM_IN_SET(type, RTN_UNICAST, RTN_LOCAL)); + nm_assert( + NM_IN_SET(type, RTN_UNICAST, RTN_LOCAL, RTN_BLACKHOLE, RTN_UNREACHABLE, RTN_PROHIBIT)); r->type_coerced = nm_platform_route_type_coerce(type); } else diff --git a/src/core/nm-l3-config-data.c b/src/core/nm-l3-config-data.c index 4b099884f8..03593ea28c 100644 --- a/src/core/nm-l3-config-data.c +++ b/src/core/nm-l3-config-data.c @@ -1094,8 +1094,22 @@ _l3_config_data_add_obj(NMDedupMultiIndex *multi_idx, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ADDRESS, NMP_OBJECT_TYPE_IP6_ROUTE)); + nm_assert((!!obj_new) != (!!pl_new)); + + if (NM_IN_SET(idx_type->obj_type, NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE)) { + const NMPlatformIPRoute *r; + + r = obj_new ? NMP_OBJECT_CAST_IP_ROUTE(obj_new) : (NMPlatformIPRoute *) pl_new; + + if (nm_platform_route_type_is_nodev(nm_platform_route_type_uncoerce(r->type_coerced))) { + /* such routes don't have a device/next-hop. We track them without ifindex. */ + ifindex = 0; + } + } + /* we go through extra lengths to accept a full obj_new object. That one, - * can be reused by increasing the ref-count. */ + * can be reused by increasing the ref-count. We thus accept any ifindex, and + * set it here. */ if (!obj_new) { nm_assert(pl_new); obj_new = nmp_object_stackinit(&obj_new_stackinit, idx_type->obj_type, pl_new); From 9ab53e561a636a48e772d48c96d6bd2e0be13329 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 20:52:49 +0100 Subject: [PATCH 28/33] core/l3cfg: let NML3Cfg handle nodev (blackhole) routes Certain route types (blackhole, unreachable, prohibit) are not tied to an interface. They are thus global and we need to track them system wide (or better: per network namespace). That is done by NMPRouteManager. For the routing rules, it's NMDevice itself to track/untrack the rules. That is done for historical reasons, at the time, NML3Cfg did not exit. Now with NML3Cfg, it seems that also NML3Cfg should be the part that handles nodev routes. One reason is that we want to move IP functionality out of NMDevice. So callers (NMDevice) would just add blackhole routes to the NML3ConfigData and let NML3Cfg handle them. Still, to handle these routes is rather different from regular routes. Normally, NML3Cfg tracks an object state (ObjStateData) for each address/route, and it hooks into platform signals to update the os_plobj field. Those signals are dispatched by NMNetns and are only per-ifindex. Hence, NML3Cfg wouldn't be notified about those nodev routes. Consequently, there os_plobj could not be (efficiently) maintained and there is no ObjStateData for such routes. Instead, all that NML3Cfg does is have the routes in the NML3ConfigData and tell NMPRouteManager about them. Seems simple enough. The only question is when should NMPRouteManager sync? For now, we sync when the track/untracking brings any changes and during reapply. Which is probably fine. --- src/core/nm-l3cfg.c | 107 ++++++++++++++++++++++++++++++++++++++++---- src/core/nm-l3cfg.h | 14 +++--- 2 files changed, 106 insertions(+), 15 deletions(-) diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index dfb80c7cb9..e7aee79a04 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -12,6 +12,7 @@ #include "libnm-glib-aux/nm-time-utils.h" #include "libnm-platform/nm-platform.h" #include "libnm-platform/nmp-object.h" +#include "libnm-platform/nmp-route-manager.h" #include "nm-netns.h" #include "n-acd/src/n-acd.h" #include "nm-l3-ipv4ll.h" @@ -410,6 +411,25 @@ static NM_UTILS_LOOKUP_DEFINE(_l3_acd_addr_state_to_string, "external-removed"), NM_UTILS_LOOKUP_ITEM(NM_L3_ACD_ADDR_STATE_USED, "used"), ); +static gboolean +_obj_is_route_nodev(const NMPObject *obj) +{ + gboolean has_ifindex; + + nm_assert(obj); + + has_ifindex = (NMP_OBJECT_CAST_OBJ_WITH_IFINDEX(obj)->ifindex > 0); + + nm_assert(has_ifindex + == !(NM_IN_SET(NMP_OBJECT_GET_TYPE(obj), + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE) + && nm_platform_route_type_is_nodev(nm_platform_route_type_uncoerce( + NMP_OBJECT_CAST_IP_ROUTE(obj)->type_coerced)))); + + return !has_ifindex; +} + /*****************************************************************************/ NMIPConfig * @@ -952,6 +972,11 @@ _obj_states_update_all(NML3Cfg *self) self->priv.p->combined_l3cd_commited, &obj, obj_type) { + if (_obj_is_route_nodev(obj)) { + /* this is a nodev route. We don't track an obj-state for this. */ + continue; + } + obj_state = g_hash_table_lookup(self->priv.p->obj_state_hash, &obj); if (!obj_state) { obj_state = @@ -1081,13 +1106,15 @@ static void _commit_collect_routes(NML3Cfg *self, int addr_family, NML3CfgCommitType commit_type, - GPtrArray **routes) + GPtrArray **routes, + GPtrArray **routes_nodev) { const int IS_IPv4 = NM_IS_IPv4(addr_family); const NMDedupMultiHeadEntry *head_entry; const NMDedupMultiEntry *entry; nm_assert(routes && !*routes); + nm_assert(routes_nodev && !*routes_nodev); head_entry = nm_l3_config_data_lookup_objs(self->priv.p->combined_l3cd_commited, NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4)); @@ -1097,16 +1124,20 @@ _commit_collect_routes(NML3Cfg *self, c_list_for_each_entry (entry, &head_entry->lst_entries_head, lst_entries) { const NMPObject *obj = entry->obj; + GPtrArray **r; - if (!_obj_states_sync_filter(self, obj, commit_type)) - continue; - - if (!*routes) { - *routes = - g_ptr_array_new_full(head_entry->len, (GDestroyNotify) nm_dedup_multi_obj_unref); + if (_obj_is_route_nodev(obj)) + r = routes_nodev; + else { + if (!_obj_states_sync_filter(self, obj, commit_type)) + continue; + r = routes; } - g_ptr_array_add(*routes, (gpointer) nmp_object_ref(obj)); + if (!*r) + *r = g_ptr_array_new_full(head_entry->len, (GDestroyNotify) nm_dedup_multi_obj_unref); + + g_ptr_array_add(*r, (gpointer) nmp_object_ref(obj)); } } @@ -3403,6 +3434,53 @@ nm_l3cfg_remove_config_all_dirty(NML3Cfg *self, gconstpointer tag) /*****************************************************************************/ +#define _NODEV_ROUTES_TAG(self, IS_IPv4) ((gconstpointer) (&(&(self)->priv.route_manager)[IS_IPv4])) + +static gboolean +_nodev_routes_untrack(NML3Cfg *self, int addr_family) +{ + return nmp_route_manager_untrack_all(self->priv.route_manager, + _NODEV_ROUTES_TAG(self, NM_IS_IPv4(addr_family)), + FALSE, + TRUE); +} + +static void +_nodev_routes_sync(NML3Cfg *self, + int addr_family, + NML3CfgCommitType commit_type, + GPtrArray *routes_nodev) +{ + const int IS_IPv4 = NM_IS_IPv4(addr_family); + const NMPObjectType obj_type = NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4); + guint i; + gboolean changed = FALSE; + + if (!routes_nodev) + goto out_clear; + + for (i = 0; i < routes_nodev->len; i++) { + const NMPObject *obj = routes_nodev->pdata[i]; + + if (nmp_route_manager_track(self->priv.route_manager, + obj_type, + NMP_OBJECT_CAST_IP_ROUTE(obj), + 1, + _NODEV_ROUTES_TAG(self, IS_IPv4), + NULL)) + changed = TRUE; + } + +out_clear: + if (_nodev_routes_untrack(self, addr_family)) + changed = TRUE; + + if (changed || commit_type >= NM_L3_CFG_COMMIT_TYPE_REAPPLY) + nmp_route_manager_sync(self->priv.route_manager, NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4), FALSE); +} + +/*****************************************************************************/ + typedef struct { NML3Cfg *self; gconstpointer tag; @@ -4102,6 +4180,7 @@ _l3_commit_one(NML3Cfg *self, const int IS_IPv4 = NM_IS_IPv4(addr_family); gs_unref_ptrarray GPtrArray *addresses = NULL; gs_unref_ptrarray GPtrArray *routes = NULL; + gs_unref_ptrarray GPtrArray *routes_nodev = NULL; gs_unref_ptrarray GPtrArray *addresses_prune = NULL; gs_unref_ptrarray GPtrArray *routes_prune = NULL; gs_unref_ptrarray GPtrArray *routes_temporary_not_available_arr = NULL; @@ -4125,7 +4204,7 @@ _l3_commit_one(NML3Cfg *self, if (self->priv.p->combined_l3cd_commited) { addresses = _commit_collect_addresses(self, addr_family, commit_type); - _commit_collect_routes(self, addr_family, commit_type, &routes); + _commit_collect_routes(self, addr_family, commit_type, &routes, &routes_nodev); route_table_sync = nm_l3_config_data_get_route_table_sync(self->priv.p->combined_l3cd_commited, @@ -4163,6 +4242,8 @@ _l3_commit_one(NML3Cfg *self, addresses, addresses_prune); + _nodev_routes_sync(self, addr_family, commit_type, routes_nodev); + if (!nm_platform_ip_route_sync(self->priv.platform, addr_family, self->priv.ifindex, @@ -4597,6 +4678,8 @@ constructed(GObject *object) self->priv.platform = g_object_ref(nm_netns_get_platform(self->priv.netns)); nm_assert(NM_IS_PLATFORM(self->priv.platform)); + self->priv.route_manager = nmp_route_manager_ref(nm_netns_get_route_manager(self->priv.netns)); + _LOGT("created (netns=" NM_HASH_OBFUSCATE_PTR_FMT ")", NM_HASH_OBFUSCATE_PTR(self->priv.netns)); G_OBJECT_CLASS(nm_l3cfg_parent_class)->constructed(object); @@ -4647,8 +4730,14 @@ finalize(GObject *object) nm_assert(c_list_is_empty(&self->priv.p->obj_state_temporary_not_available_lst_head)); nm_assert(c_list_is_empty(&self->priv.p->obj_state_zombie_lst_head)); + if (_nodev_routes_untrack(self, AF_INET)) + nmp_route_manager_sync(self->priv.route_manager, NMP_OBJECT_TYPE_IP4_ROUTE, FALSE); + if (_nodev_routes_untrack(self, AF_INET6)) + nmp_route_manager_sync(self->priv.route_manager, NMP_OBJECT_TYPE_IP6_ROUTE, FALSE); + g_clear_object(&self->priv.netns); g_clear_object(&self->priv.platform); + nm_clear_pointer(&self->priv.route_manager, nmp_route_manager_unref); nm_clear_l3cd(&self->priv.p->combined_l3cd_merged); nm_clear_l3cd(&self->priv.p->combined_l3cd_commited); diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index 6fd8f9de80..7dc9faccfe 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -195,16 +195,18 @@ typedef struct { } NML3ConfigNotifyData; struct _NML3CfgPrivate; +struct _NMPRouteManager; struct _NML3Cfg { GObject parent; struct { - struct _NML3CfgPrivate *p; - NMNetns *netns; - NMPlatform *platform; - const NMPObject *plobj; - const NMPObject *plobj_next; - int ifindex; + struct _NML3CfgPrivate *p; + NMNetns *netns; + NMPlatform *platform; + struct _NMPRouteManager *route_manager; + const NMPObject *plobj; + const NMPObject *plobj_next; + int ifindex; } priv; }; From 84598adddffc5e135cc6c7ee632e028dc9d8201a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Feb 2022 19:35:05 +0100 Subject: [PATCH 29/33] libnm: allow configuring blackhole/unreachable/prohibit routes --- src/libnm-core-impl/nm-setting-ip-config.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 3cc0486e2a..52aa651570 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -1385,7 +1385,12 @@ _ip_route_attribute_validate(const char *name, string = g_variant_get_string(value, NULL); type = nm_net_aux_rtnl_rtntype_a2n(string); - if (!NM_IN_SET(type, RTN_UNICAST, RTN_LOCAL)) { + if (!NM_IN_SET(type, + RTN_UNICAST, + RTN_LOCAL, + RTN_BLACKHOLE, + RTN_UNREACHABLE, + RTN_PROHIBIT)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -1486,6 +1491,18 @@ _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error) return FALSE; } break; + case RTN_BLACKHOLE: + case RTN_UNREACHABLE: + case RTN_PROHIBIT: + if (route->next_hop) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("a %s route cannot have a next-hop"), + nm_net_aux_rtnl_rtntype_n2a(parse_data.type)); + return FALSE; + } + break; } return TRUE; From 41a177486b4c84fd6d7ce5b488c22c991b9c4a84 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Feb 2022 20:00:25 +0100 Subject: [PATCH 30/33] tools: re-use regular expression in process_data() Yes, they get cached by the library already. Still, no need for doing this repeatedly. --- tools/generate-docs-nm-property-infos.py | 45 +++++++++++++----------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/tools/generate-docs-nm-property-infos.py b/tools/generate-docs-nm-property-infos.py index 41349e6d33..1790fd78ca 100755 --- a/tools/generate-docs-nm-property-infos.py +++ b/tools/generate-docs-nm-property-infos.py @@ -54,27 +54,29 @@ def scan_doc_comments(plugin, setting_node, file, start_tag, end_tag): return +keywords = [ + "property", + "variable", + "format", + "values", + "default", + "example", + "description", + "description-docbook", +] +kwd_first_line_re = re.compile( + r"^\s*\**\s+({}):\s+(.*?)\s*$".format("|".join(keywords)) +) +kwd_more_line_re = re.compile(r"^\s*\**\s+(.*?)\s*$") + + def process_data(data): parsed_data = {} if not data: return parsed_data - keywords = [ - "property", - "variable", - "format", - "values", - "default", - "example", - "description", - "description-docbook", - ] - kwd_pat = "|".join(keywords) keyword = "" for line in data: - kwd_first_line_found = re.search( - r"^\s*\**\s+({}):\s+(.*?)\s*$".format(kwd_pat), line - ) - kwd_more_line_found = re.search(r"^\s*\**\s+(.*?)\s*$", line) + kwd_first_line_found = kwd_first_line_re.search(line) if kwd_first_line_found: keyword = kwd_first_line_found.group(1) if keyword == "description-docbook": @@ -82,16 +84,17 @@ def process_data(data): else: value = kwd_first_line_found.group(2) + " " parsed_data[keyword] = value - elif kwd_more_line_found: + continue + kwd_more_line_found = kwd_more_line_re.search(line) + if kwd_more_line_found: if not keyword: print("Extra mess in a comment: %s" % (line)) exit(1) + if keyword == "description-docbook": + value = kwd_more_line_found.group(1) + "\n" else: - if keyword == "description-docbook": - value = kwd_more_line_found.group(1) + "\n" - else: - value = kwd_more_line_found.group(1) + " " - parsed_data[keyword] += value + value = kwd_more_line_found.group(1) + " " + parsed_data[keyword] += value for keyword in keywords: if keyword == "variable" and keyword not in parsed_data: parsed_data[keyword] = parsed_data["property"] From 35599b4349c18b771d86ea6a7f577909cfcb7a2f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Feb 2022 20:02:17 +0100 Subject: [PATCH 31/33] tools: fix constructing XML by dropping broken pretty_xml() I don't understand the code, but it mangles the XML. There is no difference in the markup we have so far. But if you have nested XML (like for description-docbook tag) there are cases where this is wrong. There is also no need to prettify anything. If you want pretty-formatted XML, do it yourself, for example with $ tidy --indent yes --indent-spaces 4 --indent-attributes yes --wrap-attributes yes --input-xml yes --output-xml yes src/libnm-client-impl/nm-property-infos-nmcli.xml I think this was initially done, because we had the tool in perl, and when migrating, we wanted to generate the exactly same output. And it was the same output, and it was fine for the input we have. But with different input, it's wrong. Drop it now. --- tools/generate-docs-nm-property-infos.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tools/generate-docs-nm-property-infos.py b/tools/generate-docs-nm-property-infos.py index 1790fd78ca..25aa272a69 100755 --- a/tools/generate-docs-nm-property-infos.py +++ b/tools/generate-docs-nm-property-infos.py @@ -123,18 +123,6 @@ def write_data(setting_node, parsed_data): property_node.append(des) -def pretty_xml(element, newline, level=0): - if element: - if (element.text is None) or element.text.isspace(): - element.text = newline - else: - element.text = newline + element.text.strip() + newline - temp = list(element) - for subelement in temp: - subelement.tail = newline - pretty_xml(subelement, newline, level=level + 1) - - if len(sys.argv) < 4: print("Usage: %s [plugin] [output-xml-file] [srcfiles]" % (sys.argv[0])) exit(1) @@ -152,6 +140,4 @@ for one_file in source_files: setting_node.text = "\n" scan_doc_comments(plugin, setting_node, one_file, start_tag, end_tag) -pretty_xml(root_node, "\n") - ET.ElementTree(root_node).write(output) From 7b1e9a5c3d146c7ddd83f552c290fc841388cfda Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Feb 2022 19:04:05 +0100 Subject: [PATCH 32/33] libnm/doc: list route attributes in `man nm-settings-nmcli` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IPv4: routes A list of IPv4 destination addresses, prefix length, optional IPv4 next hop addresses, optional route metric, optional attribute. The valid syntax is: "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". For example "192.0.2.0/24 10.1.1.1 77, 198.51.100.0/24". Various attributes are supported: • "cwnd" - an unsigned 32 bit integer. • "initcwnd" - an unsigned 32 bit integer. • "initrwnd" - an unsigned 32 bit integer. • "lock-cwnd" - a boolean value. • "lock-initcwnd" - a boolean value. • "lock-initrwnd" - a boolean value. • "lock-mtu" - a boolean value. • "lock-window" - a boolean value. • "mtu" - an unsigned 32 bit integer. • "onlink" - a boolean value. • "scope" - an unsigned 8 bit integer. IPv4 only. • "src" - an IPv4 address. • "table" - an unsigned 32 bit integer. The default depends on ipv4.route-table. • "tos" - an unsigned 8 bit integer. IPv4 only. • "type" - one of unicast, local, blackhole, unavailable, prohibit. The default is unicast. • "window" - an unsigned 32 bit integer. For details see also `man ip-route`. Format: a comma separated list of routes IPv6: routes A list of IPv6 destination addresses, prefix length, optional IPv6 next hop addresses, optional route metric, optional attribute. The valid syntax is: "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". Various attributes are supported: • "cwnd" - an unsigned 32 bit integer. • "from" - an IPv6 address with optional prefix. IPv6 only. • "initcwnd" - an unsigned 32 bit integer. • "initrwnd" - an unsigned 32 bit integer. • "lock-cwnd" - a boolean value. • "lock-initcwnd" - a boolean value. • "lock-initrwnd" - a boolean value. • "lock-mtu" - a boolean value. • "lock-window" - a boolean value. • "mtu" - an unsigned 32 bit integer. • "onlink" - a boolean value. • "src" - an IPv6 address. • "table" - an unsigned 32 bit integer. The default depends on ipv6.route-table. • "type" - one of unicast, local, blackhole, unavailable, prohibit. The default is unicast. • "window" - an unsigned 32 bit integer. For details see also `man ip-route`. Format: a comma separated list of routes --- src/libnm-core-impl/nm-setting-ip4-config.c | 70 ++++++++++++++++++++- src/libnm-core-impl/nm-setting-ip6-config.c | 65 +++++++++++++++++++ 2 files changed, 132 insertions(+), 3 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip4-config.c b/src/libnm-core-impl/nm-setting-ip4-config.c index 6c2ba10d6c..76043cd50a 100644 --- a/src/libnm-core-impl/nm-setting-ip4-config.c +++ b/src/libnm-core-impl/nm-setting-ip4-config.c @@ -949,9 +949,73 @@ nm_setting_ip4_config_class_init(NMSettingIP4ConfigClass *klass) * property: routes * format: a comma separated list of routes * description: A list of IPv4 destination addresses, prefix length, optional IPv4 - * next hop addresses, optional route metric, optional attribute. The valid syntax is: - * "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". For example - * "192.0.2.0/24 10.1.1.1 77, 198.51.100.0/24". + * next hop addresses, optional route metric, optional attribute. The valid syntax is: + * "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". For example + * "192.0.2.0/24 10.1.1.1 77, 198.51.100.0/24". + * description-docbook: + * + * A list of IPv4 destination addresses, prefix length, optional IPv4 + * next hop addresses, optional route metric, optional attribute. The valid syntax is: + * "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". + * For example "192.0.2.0/24 10.1.1.1 77, 198.51.100.0/24". + * + * + * Various attributes are supported: + * + * + * "cwnd" - an unsigned 32 bit integer. + * + * + * "initcwnd" - an unsigned 32 bit integer. + * + * + * "initrwnd" - an unsigned 32 bit integer. + * + * + * "lock-cwnd" - a boolean value. + * + * + * "lock-initcwnd" - a boolean value. + * + * + * "lock-initrwnd" - a boolean value. + * + * + * "lock-mtu" - a boolean value. + * + * + * "lock-window" - a boolean value. + * + * + * "mtu" - an unsigned 32 bit integer. + * + * + * "onlink" - a boolean value. + * + * + * "scope" - an unsigned 8 bit integer. IPv4 only. + * + * + * "src" - an IPv4 address. + * + * + * "table" - an unsigned 32 bit integer. The default depends on ipv4.route-table. + * + * + * "tos" - an unsigned 8 bit integer. IPv4 only. + * + * + * "type" - one of unicast, local, blackhole, + * unavailable, prohibit. The default is unicast. + * + * + * "window" - an unsigned 32 bit integer. + * + * + * + * + * For details see also `man ip-route`. + * * ---end--- */ _nm_properties_override_gobj( diff --git a/src/libnm-core-impl/nm-setting-ip6-config.c b/src/libnm-core-impl/nm-setting-ip6-config.c index 99032e97f2..54f3f96b87 100644 --- a/src/libnm-core-impl/nm-setting-ip6-config.c +++ b/src/libnm-core-impl/nm-setting-ip6-config.c @@ -989,6 +989,71 @@ nm_setting_ip6_config_class_init(NMSettingIP6ConfigClass *klass) * default metric for the device. * ---end--- */ + /* ---nmcli--- + * property: routes + * format: a comma separated list of routes + * description-docbook: + * + * A list of IPv6 destination addresses, prefix length, optional IPv6 + * next hop addresses, optional route metric, optional attribute. The valid syntax is: + * "ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]". + * + * + * Various attributes are supported: + * + * + * "cwnd" - an unsigned 32 bit integer. + * + * + * "from" - an IPv6 address with optional prefix. IPv6 only. + * + * + * "initcwnd" - an unsigned 32 bit integer. + * + * + * "initrwnd" - an unsigned 32 bit integer. + * + * + * "lock-cwnd" - a boolean value. + * + * + * "lock-initcwnd" - a boolean value. + * + * + * "lock-initrwnd" - a boolean value. + * + * + * "lock-mtu" - a boolean value. + * + * + * "lock-window" - a boolean value. + * + * + * "mtu" - an unsigned 32 bit integer. + * + * + * "onlink" - a boolean value. + * + * + * "src" - an IPv6 address. + * + * + * "table" - an unsigned 32 bit integer. The default depends on ipv6.route-table. + * + * + * "type" - one of unicast, local, blackhole, + * unavailable, prohibit. The default is unicast. + * + * + * "window" - an unsigned 32 bit integer. + * + * + * + * + * For details see also `man ip-route`. + * + * ---end--- + */ _nm_properties_override_gobj( properties_override, g_object_class_find_property(G_OBJECT_CLASS(setting_class), NM_SETTING_IP_CONFIG_ROUTES), From 948c2b0fb1c6a51b03ac6cfeb79febf19a234f20 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 9 Feb 2022 20:31:47 +0100 Subject: [PATCH 33/33] libnm/doc: describe routing-rules in `man nm-settings-nmcli` --- src/libnm-core-impl/nm-setting-ip4-config.c | 17 +++++++++++++++++ src/libnm-core-impl/nm-setting-ip6-config.c | 17 +++++++++++++++++ src/libnmc-setting/settings-docs.h.in | 2 ++ 3 files changed, 36 insertions(+) diff --git a/src/libnm-core-impl/nm-setting-ip4-config.c b/src/libnm-core-impl/nm-setting-ip4-config.c index 76043cd50a..5b06739c11 100644 --- a/src/libnm-core-impl/nm-setting-ip4-config.c +++ b/src/libnm-core-impl/nm-setting-ip4-config.c @@ -1047,6 +1047,23 @@ nm_setting_ip4_config_class_init(NMSettingIP4ConfigClass *klass) .compare_fcn = _nm_setting_property_compare_fcn_ignore, .from_dbus_fcn = ip4_route_data_set, )); + /* ---nmcli--- + * property: routing-rules + * format: a comma separated list of routing rules + * description: A comma separated list of routing rules for policy routing. + * description-docbook: + * + * A comma separated list of routing rules for policy routing. The format + * is based on ip rule add syntax and mostly compatible. + * One difference is that routing rules in NetworkManager always need a + * fixed priority. + * + * + * Example: priority 5 from 192.167.4.0/24 table 45 + * + * ---end--- + */ + g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties); _nm_setting_class_commit(setting_class, diff --git a/src/libnm-core-impl/nm-setting-ip6-config.c b/src/libnm-core-impl/nm-setting-ip6-config.c index 54f3f96b87..f4623b2819 100644 --- a/src/libnm-core-impl/nm-setting-ip6-config.c +++ b/src/libnm-core-impl/nm-setting-ip6-config.c @@ -1083,6 +1083,23 @@ nm_setting_ip6_config_class_init(NMSettingIP6ConfigClass *klass) .compare_fcn = _nm_setting_property_compare_fcn_ignore, .from_dbus_fcn = ip6_route_data_set, )); + /* ---nmcli--- + * property: routing-rules + * format: a comma separated list of routing rules + * description: A comma separated list of routing rules for policy routing. + * description-docbook: + * + * A comma separated list of routing rules for policy routing. The format + * is based on ip rule add syntax and mostly compatible. + * One difference is that routing rules in NetworkManager always need a + * fixed priority. + * + * + * Example: priority 5 from 1:2:3::5/128 table 45 + * + * ---end--- + */ + g_object_class_install_properties(object_class, _PROPERTY_ENUMS_LAST, obj_properties); _nm_setting_class_commit(setting_class, diff --git a/src/libnmc-setting/settings-docs.h.in b/src/libnmc-setting/settings-docs.h.in index 0bc7d06932..5ba6302d64 100644 --- a/src/libnmc-setting/settings-docs.h.in +++ b/src/libnmc-setting/settings-docs.h.in @@ -253,6 +253,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ROUTE_METRIC N_("The default metric for routes that don't explicitly specify a metric. The default value -1 means that the metric is chosen automatically based on the device type. The metric applies to dynamic routes, manual (static) routes that don't have an explicit metric setting, address prefix routes, and the default route. Note that for IPv6, the kernel accepts zero (0) but coerces it to 1024 (user default). Hence, setting this property to zero effectively mean setting it to 1024. For IPv4, zero is a regular value for the metric.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ROUTE_TABLE N_("Enable policy routing (source routing) and set the routing table used when adding routes. This affects all routes, including device-routes, IPv4LL, DHCP, SLAAC, default-routes and static routes. But note that static routes can individually overwrite the setting by explicitly specifying a non-zero routing table. If the table setting is left at zero, it is eligible to be overwritten via global configuration. If the property is zero even after applying the global configuration value, policy routing is disabled for the address family of this connection. Policy routing disabled means that NetworkManager will add all routes to the main table (except static routes that explicitly configure a different table). Additionally, NetworkManager will not delete any extraneous routes from tables except the main table. This is to preserve backward compatibility for users who manage routing tables outside of NetworkManager.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ROUTES N_("A list of IPv4 destination addresses, prefix length, optional IPv4 next hop addresses, optional route metric, optional attribute. The valid syntax is: \"ip[/prefix] [next-hop] [metric] [attribute=val]...[,ip[/prefix]...]\". For example \"192.0.2.0/24 10.1.1.1 77, 198.51.100.0/24\".") +#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ROUTING_RULES N_("A comma separated list of routing rules for policy routing.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE N_("Configure method for creating the address for use with RFC4862 IPv6 Stateless Address Autoconfiguration. The permitted values are: NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_EUI64 (0) or NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY (1). If the property is set to EUI64, the addresses will be generated using the interface tokens derived from hardware address. This makes the host part of the address to stay constant, making it possible to track host's presence when it changes networks. The address changes when the interface hardware is replaced. The value of stable-privacy enables use of cryptographically secure hash of a secret host-specific key along with the connection's stable-id and the network address as specified by RFC7217. This makes it impossible to use the address track host's presence, and makes the address stable when the network interface hardware is replaced. On D-Bus, the absence of an addr-gen-mode setting equals enabling stable-privacy. For keyfile plugin, the absence of the setting on disk means EUI64 so that the property doesn't change on upgrade from older versions. Note that this setting is distinct from the Privacy Extensions as configured by \"ip6-privacy\" property and it does not affect the temporary addresses configured with this option.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ADDRESSES N_("A list of IPv6 addresses and their prefix length. Multiple addresses can be separated by comma. For example \"2001:db8:85a3::8a2e:370:7334/64, 2001:db8:85a3::5/64\". The addresses are listed in increasing priority, meaning the last address will be the primary address.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DAD_TIMEOUT N_("Timeout in milliseconds used to check for the presence of duplicate IP addresses on the network. If an address conflict is detected, the activation will fail. A zero value means that no duplicate address detection is performed, -1 means the default value (either configuration ipvx.dad-timeout override or zero). A value greater than zero is a timeout in milliseconds. The property is currently implemented only for IPv4.") @@ -279,6 +280,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ROUTE_METRIC N_("The default metric for routes that don't explicitly specify a metric. The default value -1 means that the metric is chosen automatically based on the device type. The metric applies to dynamic routes, manual (static) routes that don't have an explicit metric setting, address prefix routes, and the default route. Note that for IPv6, the kernel accepts zero (0) but coerces it to 1024 (user default). Hence, setting this property to zero effectively mean setting it to 1024. For IPv4, zero is a regular value for the metric.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ROUTE_TABLE N_("Enable policy routing (source routing) and set the routing table used when adding routes. This affects all routes, including device-routes, IPv4LL, DHCP, SLAAC, default-routes and static routes. But note that static routes can individually overwrite the setting by explicitly specifying a non-zero routing table. If the table setting is left at zero, it is eligible to be overwritten via global configuration. If the property is zero even after applying the global configuration value, policy routing is disabled for the address family of this connection. Policy routing disabled means that NetworkManager will add all routes to the main table (except static routes that explicitly configure a different table). Additionally, NetworkManager will not delete any extraneous routes from tables except the main table. This is to preserve backward compatibility for users who manage routing tables outside of NetworkManager.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ROUTES N_("Array of IP routes.") +#define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ROUTING_RULES N_("A comma separated list of routing rules for policy routing.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_TOKEN N_("Configure the token for draft-chown-6man-tokenised-ipv6-identifiers-02 IPv6 tokenized interface identifiers. Useful with eui64 addr-gen-mode.") #define DESCRIBE_DOC_NM_SETTING_MACSEC_ENCRYPT N_("Whether the transmitted traffic must be encrypted.") #define DESCRIBE_DOC_NM_SETTING_MACSEC_MKA_CAK N_("The pre-shared CAK (Connectivity Association Key) for MACsec Key Agreement.")