From e7816a25083b5d642b6895adac68e93763852e3d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Nov 2019 15:19:44 +0100 Subject: [PATCH 1/4] ifcfg-rh: fix accepting onlink flag also for IPv6 routes In the past, kernel (and NetworkManager) did not support the onlink flags for IPv6 routes. That is no longer the case. Fixes: f5e8bbc8e082 ('libnm,core: enable "onlink" flags also for IPv6 routes') --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index c4c4b344d0..fad5be8164 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -854,8 +854,7 @@ parse_route_line (const char *line, .int_base_16 = TRUE, .ignore = (addr_family != AF_INET), }, [PARSE_LINE_ATTR_ROUTE_ONLINK] = { .key = NM_IP_ROUTE_ATTRIBUTE_ONLINK, - .type = PARSE_LINE_TYPE_FLAG, - .ignore = (addr_family != AF_INET), }, + .type = PARSE_LINE_TYPE_FLAG, }, [PARSE_LINE_ATTR_ROUTE_WINDOW] = { .key = NM_IP_ROUTE_ATTRIBUTE_WINDOW, .type = PARSE_LINE_TYPE_UINT32_WITH_LOCK, }, [PARSE_LINE_ATTR_ROUTE_CWND] = { .key = NM_IP_ROUTE_ATTRIBUTE_CWND, From 7cadc5e46593bbc49cb9d75ee0900fd6f2314a15 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Nov 2019 13:13:48 +0100 Subject: [PATCH 2/4] libnm: lookup route attributes attribute spec via binary search --- libnm-core/nm-core-internal.h | 9 +++++++ libnm-core/nm-setting-ip-config.c | 29 +++++++++------------ libnm-core/nm-setting-ip-config.h | 16 ++++++------ libnm-core/nm-utils.c | 42 +++++++++++++++++++++++++++++++ libnm-core/tests/test-general.c | 28 +++++++++++++++++++++ 5 files changed, 99 insertions(+), 25 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index bc0bf39426..06456e6577 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -874,4 +874,13 @@ const char *nm_utils_wifi_freq_to_band (guint32 freq); gboolean _nm_utils_iaid_verify (const char *str, gint64 *out_value); +/*****************************************************************************/ + +gboolean _nmtst_variant_attribute_spec_assert_sorted (const NMVariantAttributeSpec *const*array, + gsize len); + +const NMVariantAttributeSpec *_nm_variant_attribute_spec_find_binary_search (const NMVariantAttributeSpec *const*array, + gsize len, + const char *name); + #endif diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index f966a53743..0d7e9b0530 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -1197,21 +1197,21 @@ nm_ip_route_set_attribute (NMIPRoute *route, const char *name, GVariant *value) } static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TABLE, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_SRC, G_VARIANT_TYPE_STRING, .v4 = TRUE, .v6 = TRUE, .str_type = 'a', ), - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_FROM, G_VARIANT_TYPE_STRING, .v6 = TRUE, .str_type = 'p', ), - 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_ONLINK, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_WINDOW, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_CWND, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_FROM, G_VARIANT_TYPE_STRING, .v6 = TRUE, .str_type = 'p', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_INITCWND, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_INITRWND, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_MTU, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), - NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_INITRWND, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_MTU, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_MTU, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_ONLINK, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_SRC, G_VARIANT_TYPE_STRING, .v4 = TRUE, .v6 = TRUE, .str_type = 'a', ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TABLE, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), + 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_WINDOW, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), NULL, }; @@ -1250,21 +1250,16 @@ nm_ip_route_attribute_validate (const char *name, gboolean *known, GError **error) { - const NMVariantAttributeSpec *const *iter; - const NMVariantAttributeSpec *spec = NULL; + const NMVariantAttributeSpec *spec; 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); - for (iter = ip_route_attribute_spec; *iter; iter++) { - if (nm_streq (name, (*iter)->name)) { - spec = *iter; - break; - } - } - + spec = _nm_variant_attribute_spec_find_binary_search (ip_route_attribute_spec, + G_N_ELEMENTS (ip_route_attribute_spec) - 1, + name); if (!spec) { NM_SET_OUT (known, FALSE); g_set_error_literal (error, diff --git a/libnm-core/nm-setting-ip-config.h b/libnm-core/nm-setting-ip-config.h index 440ebe5a6c..feea89db22 100644 --- a/libnm-core/nm-setting-ip-config.h +++ b/libnm-core/nm-setting-ip-config.h @@ -127,21 +127,21 @@ gboolean nm_ip_route_attribute_validate (const char *name, gboolean *known, GError **error); -#define NM_IP_ROUTE_ATTRIBUTE_TABLE "table" -#define NM_IP_ROUTE_ATTRIBUTE_SRC "src" -#define NM_IP_ROUTE_ATTRIBUTE_FROM "from" -#define NM_IP_ROUTE_ATTRIBUTE_TOS "tos" -#define NM_IP_ROUTE_ATTRIBUTE_ONLINK "onlink" -#define NM_IP_ROUTE_ATTRIBUTE_WINDOW "window" #define NM_IP_ROUTE_ATTRIBUTE_CWND "cwnd" +#define NM_IP_ROUTE_ATTRIBUTE_FROM "from" #define NM_IP_ROUTE_ATTRIBUTE_INITCWND "initcwnd" #define NM_IP_ROUTE_ATTRIBUTE_INITRWND "initrwnd" -#define NM_IP_ROUTE_ATTRIBUTE_MTU "mtu" -#define NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW "lock-window" #define NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND "lock-cwnd" #define NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND "lock-initcwnd" #define NM_IP_ROUTE_ATTRIBUTE_LOCK_INITRWND "lock-initrwnd" #define NM_IP_ROUTE_ATTRIBUTE_LOCK_MTU "lock-mtu" +#define NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW "lock-window" +#define NM_IP_ROUTE_ATTRIBUTE_MTU "mtu" +#define NM_IP_ROUTE_ATTRIBUTE_ONLINK "onlink" +#define NM_IP_ROUTE_ATTRIBUTE_SRC "src" +#define NM_IP_ROUTE_ATTRIBUTE_TABLE "table" +#define NM_IP_ROUTE_ATTRIBUTE_TOS "tos" +#define NM_IP_ROUTE_ATTRIBUTE_WINDOW "window" /*****************************************************************************/ diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index c825bb12ba..347ef84093 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -5555,6 +5555,48 @@ attribute_unescape (const char *start, const char *end) return ret; } +gboolean +_nmtst_variant_attribute_spec_assert_sorted (const NMVariantAttributeSpec *const*array, + gsize len) +{ + gsize i; + + g_assert (array); + g_assert (len > 0); + g_assert_cmpint(len, ==, NM_PTRARRAY_LEN (array)); + + for (i = 0; i < len; i++) { + nm_assert (array[i]->name); + nm_assert (array[i]->name[0]); + if (i > 0) + nm_assert (strcmp (array[i - 1]->name, array[i]->name) < 0); + } + nm_assert (!array[i]); + + return TRUE; +} + +const NMVariantAttributeSpec * +_nm_variant_attribute_spec_find_binary_search (const NMVariantAttributeSpec *const*array, + gsize len, + const char *name) +{ + gssize idx; + + G_STATIC_ASSERT_EXPR (G_STRUCT_OFFSET (NMVariantAttributeSpec, name) == 0); + + idx = nm_utils_ptrarray_find_binary_search ((gconstpointer *) array, + len, + &name, + nm_strcmp_p_with_data, + NULL, + NULL, + NULL); + if (idx < 0) + return NULL; + return array[idx]; +} + /** * nm_utils_parse_variant_attributes: * @string: the input string diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 40eabc1208..cbc1a2f2ff 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -7986,6 +7986,33 @@ test_route_attributes_format (void) /*****************************************************************************/ +static void +test_variant_attribute_spec (void) +{ + const NMVariantAttributeSpec *const *const specs_list[] = { + nm_ip_route_get_variant_attribute_spec (), + }; + int i_specs; + + for (i_specs = 0; i_specs < G_N_ELEMENTS (specs_list); i_specs++) { + const NMVariantAttributeSpec *const *const specs = specs_list[i_specs]; + gsize len; + gsize i; + + g_assert (specs); + + len = NM_PTRARRAY_LEN (specs); + g_assert_cmpint (len, >, 0u); + + _nmtst_variant_attribute_spec_assert_sorted (specs, len); + for (i = 0; i < len; i++) + g_assert (specs[i] == _nm_variant_attribute_spec_find_binary_search (specs, len, specs[i]->name)); + g_assert (!_nm_variant_attribute_spec_find_binary_search (specs, len, "bogus")); + } +} + +/*****************************************************************************/ + static gboolean do_test_nm_set_out_called (int *call_count) { @@ -8448,6 +8475,7 @@ int main (int argc, char **argv) g_test_add_func ("/core/general/nm-set-out", test_nm_set_out); g_test_add_func ("/core/general/route_attributes/parse", test_route_attributes_parse); g_test_add_func ("/core/general/route_attributes/format", test_route_attributes_format); + g_test_add_func ("/core/general/test_variant_attribute_spec", test_variant_attribute_spec); g_test_add_func ("/core/general/get_start_time_for_pid", test_get_start_time_for_pid); g_test_add_func ("/core/general/test_nm_va_args_macros", test_nm_va_args_macros); From b8c00780085d56cfcd0b489653aef9b23938dffc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Nov 2019 15:18:32 +0100 Subject: [PATCH 3/4] ifcfg-rh: separately handle static information during parsing ip-route commandline There is an "info" part and a part with the data that we parsed. Don't track the static and mutable data in the same variable. Also, this allows to mark the static part as "const static". --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 194 ++++++++++-------- 1 file changed, 110 insertions(+), 84 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index fad5be8164..66658bde75 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -739,25 +739,36 @@ parse_route_line_is_comment (const char *line) /*****************************************************************************/ +typedef enum { + PARSE_LINE_AF_FLAG_FOR_IPV4 = 0x01, + PARSE_LINE_AF_FLAG_FOR_IPV6 = 0x02, +} ParseLineAFFlag; + typedef struct { const char *key; /* the element is not available in this case. */ - bool disabled:1; + ParseLineAFFlag disabled:3; + + bool disabled_with_options_route:1; /* whether the element is to be ignored. Ignord is different from * "disabled", because we still parse the option, but don't use it. */ - bool ignore:1; + ParseLineAFFlag ignore:3; bool int_base_16:1; + /* the type, one of PARSE_LINE_TYPE_* */ + char type; + +} ParseLineInfo; + +typedef struct { + /* whether the command line option was found, and @v is * initialized. */ bool has:1; - /* the type, one of PARSE_LINE_TYPE_* */ - char type; - union { guint8 uint8; guint32 uint32; @@ -772,7 +783,7 @@ typedef struct { } addr; } v; -} ParseLineInfo; +} ParseLineData; enum { /* route attributes */ @@ -833,26 +844,18 @@ parse_route_line (const char *line, NMIPRoute **out_route, GError **error) { - nm_auto_unref_ip_route NMIPRoute *route = NULL; - gs_free const char **words_free = NULL; - const char *const*words; - const char *s; - gsize i_words; - guint i; - char buf1[256]; - char buf2[256]; - ParseLineInfo infos[] = { + static const ParseLineInfo parse_infos[] = { [PARSE_LINE_ATTR_ROUTE_TABLE] = { .key = NM_IP_ROUTE_ATTRIBUTE_TABLE, .type = PARSE_LINE_TYPE_UINT32, }, [PARSE_LINE_ATTR_ROUTE_SRC] = { .key = NM_IP_ROUTE_ATTRIBUTE_SRC, .type = PARSE_LINE_TYPE_ADDR, }, [PARSE_LINE_ATTR_ROUTE_FROM] = { .key = NM_IP_ROUTE_ATTRIBUTE_FROM, .type = PARSE_LINE_TYPE_ADDR_WITH_PREFIX, - .disabled = (addr_family != AF_INET6), }, + .disabled = PARSE_LINE_AF_FLAG_FOR_IPV4, }, [PARSE_LINE_ATTR_ROUTE_TOS] = { .key = NM_IP_ROUTE_ATTRIBUTE_TOS, .type = PARSE_LINE_TYPE_UINT8, .int_base_16 = TRUE, - .ignore = (addr_family != AF_INET), }, + .ignore = PARSE_LINE_AF_FLAG_FOR_IPV6, }, [PARSE_LINE_ATTR_ROUTE_ONLINK] = { .key = NM_IP_ROUTE_ATTRIBUTE_ONLINK, .type = PARSE_LINE_TYPE_FLAG, }, [PARSE_LINE_ATTR_ROUTE_WINDOW] = { .key = NM_IP_ROUTE_ATTRIBUTE_WINDOW, @@ -868,19 +871,31 @@ parse_route_line (const char *line, [PARSE_LINE_ATTR_ROUTE_TO] = { .key = "to", .type = PARSE_LINE_TYPE_ADDR_WITH_PREFIX, - .disabled = (options_route != NULL), }, + .disabled_with_options_route = TRUE, }, [PARSE_LINE_ATTR_ROUTE_VIA] = { .key = "via", .type = PARSE_LINE_TYPE_ADDR, - .disabled = (options_route != NULL), }, + .disabled_with_options_route = TRUE, }, [PARSE_LINE_ATTR_ROUTE_METRIC] = { .key = "metric", .type = PARSE_LINE_TYPE_UINT32, - .disabled = (options_route != NULL), }, + .disabled_with_options_route = TRUE, }, [PARSE_LINE_ATTR_ROUTE_DEV] = { .key = "dev", .type = PARSE_LINE_TYPE_IFNAME, - .ignore = TRUE, - .disabled = (options_route != NULL), }, + .ignore = PARSE_LINE_AF_FLAG_FOR_IPV4 | PARSE_LINE_AF_FLAG_FOR_IPV6, + .disabled_with_options_route = TRUE, }, }; + nm_auto_unref_ip_route NMIPRoute *route = NULL; + gs_free const char **words_free = NULL; + const char *const*words; + const char *s; + gsize i_words; + guint i; + char buf1[256]; + char buf2[256]; + ParseLineData parse_datas[G_N_ELEMENTS (parse_infos)] = { }; + const ParseLineAFFlag af_flag = (addr_family == AF_INET) + ? PARSE_LINE_AF_FLAG_FOR_IPV4 + : PARSE_LINE_AF_FLAG_FOR_IPV6; nm_assert (line); nm_assert_addr_family (addr_family); @@ -908,19 +923,22 @@ parse_route_line (const char *line, for (i_words = 0; words[i_words]; ) { const gsize i_words0 = i_words; const char *const w = words[i_words0]; - ParseLineInfo *info; + const ParseLineInfo *p_info; + ParseLineData *p_data; gboolean unqualified_addr = FALSE; - for (i = 0; i < G_N_ELEMENTS (infos); i++) { - info = &infos[i]; + for (i = 0; i < G_N_ELEMENTS (parse_infos); i++) { + p_info = &parse_infos[i]; + p_data = &parse_datas[i]; - if (info->disabled) + if ( (p_info->disabled & af_flag) + || (p_info->disabled_with_options_route && options_route)) continue; - if (!nm_streq (w, info->key)) + if (!nm_streq (w, p_info->key)) continue; - if (info->has) { + if (p_data->has) { /* iproute2 for most arguments allows specifying them multiple times. * Let's not do that. */ g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, @@ -928,8 +946,8 @@ parse_route_line (const char *line, return -EINVAL; } - info->has = TRUE; - switch (info->type) { + p_data->has = TRUE; + switch (p_info->type) { case PARSE_LINE_TYPE_UINT8: i_words++; goto parse_line_type_uint8; @@ -957,10 +975,13 @@ parse_route_line (const char *line, } /* "to" is also accepted unqualified... (once) */ - info = &infos[PARSE_LINE_ATTR_ROUTE_TO]; - if (!info->has && !info->disabled) { + p_info = &parse_infos[PARSE_LINE_ATTR_ROUTE_TO]; + p_data = &parse_datas[PARSE_LINE_ATTR_ROUTE_TO]; + if ( !p_data->has + && !(p_info->disabled & af_flag) + && !(p_info->disabled_with_options_route && options_route)) { unqualified_addr = TRUE; - info->has = TRUE; + p_data->has = TRUE; goto parse_line_type_addr; } @@ -972,11 +993,11 @@ parse_line_type_uint8: s = words[i_words]; if (!s) goto err_word_missing_argument; - info->v.uint8 = _nm_utils_ascii_str_to_int64 (s, - info->int_base_16 ? 16 : 10, - 0, - G_MAXUINT8, - 0);; + p_data->v.uint8 = _nm_utils_ascii_str_to_int64 (s, + p_info->int_base_16 ? 16 : 10, + 0, + G_MAXUINT8, + 0);; if (errno) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Argument for \"%s\" is not a valid number", w); @@ -990,17 +1011,17 @@ parse_line_type_uint32_with_lock: s = words[i_words]; if (!s) goto err_word_missing_argument; - if (info->type == PARSE_LINE_TYPE_UINT32_WITH_LOCK) { + if (p_info->type == PARSE_LINE_TYPE_UINT32_WITH_LOCK) { if (nm_streq (s, "lock")) { s = words[++i_words]; if (!s) goto err_word_missing_argument; - info->v.uint32_with_lock.lock = TRUE; + p_data->v.uint32_with_lock.lock = TRUE; } else - info->v.uint32_with_lock.lock = FALSE; - info->v.uint32_with_lock.uint32 = _nm_utils_ascii_str_to_int64 (s, 10, 0, G_MAXUINT32, 0);; + p_data->v.uint32_with_lock.lock = FALSE; + p_data->v.uint32_with_lock.uint32 = _nm_utils_ascii_str_to_int64 (s, 10, 0, G_MAXUINT32, 0);; } else { - info->v.uint32 = _nm_utils_ascii_str_to_int64 (s, 10, 0, G_MAXUINT32, 0); + p_data->v.uint32 = _nm_utils_ascii_str_to_int64 (s, 10, 0, G_MAXUINT32, 0); } if (errno) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, @@ -1025,16 +1046,16 @@ parse_line_type_addr_with_prefix: { int prefix = -1; - if (info->type == PARSE_LINE_TYPE_ADDR) { + if (p_info->type == PARSE_LINE_TYPE_ADDR) { if (!nm_utils_parse_inaddr_bin (addr_family, s, NULL, - &info->v.addr.addr)) { - if ( info == &infos[PARSE_LINE_ATTR_ROUTE_VIA] + &p_data->v.addr.addr)) { + if ( p_info == &parse_infos[PARSE_LINE_ATTR_ROUTE_VIA] && nm_streq (s, "(null)")) { /* Due to a bug, would older versions of NM write "via (null)" * (rh#1452648). Workaround that, and accept it.*/ - memset (&info->v.addr.addr, 0, sizeof (info->v.addr.addr)); + memset (&p_data->v.addr.addr, 0, sizeof (p_data->v.addr.addr)); } else { if (unqualified_addr) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, @@ -1049,15 +1070,15 @@ parse_line_type_addr_with_prefix: } } } else { - nm_assert (info->type == PARSE_LINE_TYPE_ADDR_WITH_PREFIX); - if ( info == &infos[PARSE_LINE_ATTR_ROUTE_TO] + nm_assert (p_info->type == PARSE_LINE_TYPE_ADDR_WITH_PREFIX); + if ( p_info == &parse_infos[PARSE_LINE_ATTR_ROUTE_TO] && nm_streq (s, "default")) { - memset (&info->v.addr.addr, 0, sizeof (info->v.addr.addr)); + memset (&p_data->v.addr.addr, 0, sizeof (p_data->v.addr.addr)); prefix = 0; } else if (!nm_utils_parse_inaddr_prefix_bin (addr_family, s, NULL, - &info->v.addr.addr, + &p_data->v.addr.addr, &prefix)) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Argument for \"%s\" is not ADDR/PREFIX format", w); @@ -1065,10 +1086,10 @@ parse_line_type_addr_with_prefix: } } if (prefix == -1) - info->v.addr.has_plen = FALSE; + p_data->v.addr.has_plen = FALSE; else { - info->v.addr.has_plen = TRUE; - info->v.addr.plen = prefix; + p_data->v.addr.has_plen = TRUE; + p_data->v.addr.plen = prefix; } } i_words++; @@ -1086,70 +1107,75 @@ next: route = options_route; nm_ip_route_ref (route); } else { - ParseLineInfo *info_to = &infos[PARSE_LINE_ATTR_ROUTE_TO]; - ParseLineInfo *info_via = &infos[PARSE_LINE_ATTR_ROUTE_VIA]; - ParseLineInfo *info_metric = &infos[PARSE_LINE_ATTR_ROUTE_METRIC]; + ParseLineData *data_to = &parse_datas[PARSE_LINE_ATTR_ROUTE_TO]; + ParseLineData *data_via = &parse_datas[PARSE_LINE_ATTR_ROUTE_VIA]; + ParseLineData *data_metric = &parse_datas[PARSE_LINE_ATTR_ROUTE_METRIC]; guint prefix; - if (!info_to->has) { + if (!data_to->has) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Missing destination prefix"); return -EINVAL; } - prefix = info_to->v.addr.has_plen - ? info_to->v.addr.plen + prefix = data_to->v.addr.has_plen + ? data_to->v.addr.plen : (addr_family == AF_INET ? 32 : 128); route = nm_ip_route_new_binary (addr_family, - &info_to->v.addr.addr, + &data_to->v.addr.addr, prefix, - info_via->has ? &info_via->v.addr.addr : NULL, - info_metric->has ? (gint64) info_metric->v.uint32 : (gint64) -1, + data_via->has ? &data_via->v.addr.addr : NULL, + data_metric->has ? (gint64) data_metric->v.uint32 : (gint64) -1, error); - info_to->has = FALSE; - info_via->has = FALSE; - info_metric->has = FALSE; + data_to->has = FALSE; + data_via->has = FALSE; + data_metric->has = FALSE; if (!route) return -EINVAL; } - for (i = 0; i < G_N_ELEMENTS (infos); i++) { - ParseLineInfo *info = &infos[i]; + for (i = 0; i < G_N_ELEMENTS (parse_infos); i++) { + const ParseLineInfo *p_info = &parse_infos[i]; + ParseLineData *p_data = &parse_datas[i]; - if (!info->has) + if (!p_data->has) continue; - if (info->ignore || info->disabled) + + if ( (p_info->ignore & af_flag) + || (p_info->disabled & af_flag) + || (p_info->disabled_with_options_route && options_route)) continue; - switch (info->type) { + + switch (p_info->type) { case PARSE_LINE_TYPE_UINT8: nm_ip_route_set_attribute (route, - info->key, - g_variant_new_byte (info->v.uint8)); + p_info->key, + g_variant_new_byte (p_data->v.uint8)); break; case PARSE_LINE_TYPE_UINT32: nm_ip_route_set_attribute (route, - info->key, - g_variant_new_uint32 (info->v.uint32)); + p_info->key, + g_variant_new_uint32 (p_data->v.uint32)); break; case PARSE_LINE_TYPE_UINT32_WITH_LOCK: - if (info->v.uint32_with_lock.lock) { + if (p_data->v.uint32_with_lock.lock) { nm_ip_route_set_attribute (route, - nm_sprintf_buf (buf1, "lock-%s", info->key), + nm_sprintf_buf (buf1, "lock-%s", p_info->key), g_variant_new_boolean (TRUE)); } nm_ip_route_set_attribute (route, - info->key, - g_variant_new_uint32 (info->v.uint32_with_lock.uint32)); + p_info->key, + g_variant_new_uint32 (p_data->v.uint32_with_lock.uint32)); break; case PARSE_LINE_TYPE_ADDR: case PARSE_LINE_TYPE_ADDR_WITH_PREFIX: nm_ip_route_set_attribute (route, - info->key, + p_info->key, g_variant_new_printf ("%s%s", - inet_ntop (addr_family, &info->v.addr.addr, buf1, sizeof (buf1)), - info->v.addr.has_plen - ? nm_sprintf_buf (buf2, "/%u", (unsigned) info->v.addr.plen) + inet_ntop (addr_family, &p_data->v.addr.addr, buf1, sizeof (buf1)), + p_data->v.addr.has_plen + ? nm_sprintf_buf (buf2, "/%u", (unsigned) p_data->v.addr.plen) : "")); break; case PARSE_LINE_TYPE_FLAG: @@ -1158,7 +1184,7 @@ next: * of this attribute, hence, the file format cannot encode * that configuration. */ nm_ip_route_set_attribute (route, - info->key, + p_info->key, g_variant_new_boolean (TRUE)); break; default: From b9f1beb06e624cc3d6bb0bb8723347392376e200 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Nov 2019 13:18:52 +0100 Subject: [PATCH 4/4] all: add support for "scope" attribute for IPv4 routes - systemd-networkd and initscripts both support it. - it seems suggested to configure routes with scope "link" on AWS. - the scope is only supported for IPv4 routes. Kernel ignores the attribute for IPv6 routes. - we don't support the aliases like "link" or "global". Instead only the numeric value is supported. This is different from systemd-networkd, which accepts names like "global" and "link", but no numerical values. I think restricting ourself only to the aliases unnecessarily limits what is possible on netlink. The alternative would be to allow aliases and numbers both, but that causes multiple ways to define something and has thus downsides. So, only numeric values. - when setting rtm_scope to RT_SCOPE_NOWHERE (0, the default), kernel will coerce that to RT_SCOPE_LINK. This ambiguity of nowhere vs. link is a problem, but we don't do anything about it. - The other problem is, that when deleting a route with scope RT_SCOPE_NOWHERE, this acts as a wild care and removes the first route that matches (given the other route attributes). That means, NetworkManager has no meaningful way to delete a route with scope zero, there is always the danger that we might delete the wrong route. But this is nothing new to this patch. The problem existed already previously, except that NetworkManager could only add routes with scope nowhere (i.e. link). --- NEWS | 1 + libnm-core/nm-setting-ip-config.c | 1 + libnm-core/nm-setting-ip-config.h | 1 + src/nm-ip4-config.c | 7 +++- src/platform/nm-platform.h | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 39 +++++++++++++++++++ .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 2 + .../route-test-wired-static-routes | 4 +- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 2 + 9 files changed, 55 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index b33d76d8be..daead33e0b 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ USE AT YOUR OWN RISK. NOT RECOMMENDED FOR PRODUCTION USE! type instances would have been created by NMClient for a while now. * DHCP: switch "internal" DHCPv4 plugin from code based on systemd to use nettools' n-dhcp4 library. +* Add support for "scope" attribute for IPv4 routes. * libnm: heavily internal rework NMClient. This slims down libnm and makes the implementation more efficient. NMClient should work now well with a separate GMainContext. diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index 0d7e9b0530..cd833e09fc 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -1208,6 +1208,7 @@ static const NMVariantAttributeSpec *const ip_route_attribute_spec[] = { NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_MTU, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_ONLINK, G_VARIANT_TYPE_BOOLEAN, .v4 = TRUE, .v6 = TRUE, ), + NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_SCOPE, G_VARIANT_TYPE_BYTE, .v4 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_SRC, G_VARIANT_TYPE_STRING, .v4 = TRUE, .v6 = TRUE, .str_type = 'a', ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TABLE, G_VARIANT_TYPE_UINT32, .v4 = TRUE, .v6 = TRUE, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE (NM_IP_ROUTE_ATTRIBUTE_TOS, G_VARIANT_TYPE_BYTE, .v4 = TRUE, ), diff --git a/libnm-core/nm-setting-ip-config.h b/libnm-core/nm-setting-ip-config.h index feea89db22..07581976db 100644 --- a/libnm-core/nm-setting-ip-config.h +++ b/libnm-core/nm-setting-ip-config.h @@ -138,6 +138,7 @@ gboolean nm_ip_route_attribute_validate (const char *name, #define NM_IP_ROUTE_ATTRIBUTE_LOCK_WINDOW "lock-window" #define NM_IP_ROUTE_ATTRIBUTE_MTU "mtu" #define NM_IP_ROUTE_ATTRIBUTE_ONLINK "onlink" +#define NM_IP_ROUTE_ATTRIBUTE_SCOPE "scope" #define NM_IP_ROUTE_ATTRIBUTE_SRC "src" #define NM_IP_ROUTE_ATTRIBUTE_TABLE "table" #define NM_IP_ROUTE_ATTRIBUTE_TOS "tos" diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c index 30bd5c6790..cd14fb8f94 100644 --- a/src/nm-ip4-config.c +++ b/src/nm-ip4-config.c @@ -862,8 +862,13 @@ _nm_ip_config_merge_route_attributes (int addr_family, GET_ATTR (NM_IP_ROUTE_ATTRIBUTE_TABLE, table, UINT32, uint32, 0); r->table_coerced = nm_platform_route_table_coerce (table ?: (route_table ?: RT_TABLE_MAIN)); - if (addr_family == AF_INET) + if (addr_family == AF_INET) { + guint8 scope; + GET_ATTR (NM_IP_ROUTE_ATTRIBUTE_TOS, r4->tos, BYTE, byte, 0); + GET_ATTR (NM_IP_ROUTE_ATTRIBUTE_SCOPE, scope, BYTE, byte, RT_SCOPE_NOWHERE); + r4->scope_inv = nm_platform_route_scope_inv (scope); + } GET_ATTR (NM_IP_ROUTE_ATTRIBUTE_ONLINK, onlink, BOOLEAN, boolean, FALSE); diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 0340136c86..4bd8e34d1e 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -496,7 +496,7 @@ struct _NMPlatformIP4Route { /* The bitwise inverse of the route scope rtm_scope. It is inverted so that the * default value (RT_SCOPE_NOWHERE) is zero. Use nm_platform_route_scope_inv() - * to convert back and forth between the inverese representation and the + * to convert back and forth between the inverse representation and the * real value. * * rtm_scope is part of the primary key for IPv4 routes. When deleting a route, diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 66658bde75..5f61d4fb0f 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "nm-glib-aux/nm-secret-utils.h" #include "nm-connection.h" @@ -791,6 +792,7 @@ enum { PARSE_LINE_ATTR_ROUTE_SRC, PARSE_LINE_ATTR_ROUTE_FROM, PARSE_LINE_ATTR_ROUTE_TOS, + PARSE_LINE_ATTR_ROUTE_SCOPE, PARSE_LINE_ATTR_ROUTE_ONLINK, PARSE_LINE_ATTR_ROUTE_WINDOW, PARSE_LINE_ATTR_ROUTE_CWND, @@ -814,6 +816,7 @@ enum { #define PARSE_LINE_TYPE_ADDR_WITH_PREFIX 'p' #define PARSE_LINE_TYPE_IFNAME 'i' #define PARSE_LINE_TYPE_FLAG 'f' +#define PARSE_LINE_TYPE_ROUTE_SCOPE 'S' /** * parse_route_line: @@ -856,6 +859,9 @@ parse_route_line (const char *line, .type = PARSE_LINE_TYPE_UINT8, .int_base_16 = TRUE, .ignore = PARSE_LINE_AF_FLAG_FOR_IPV6, }, + [PARSE_LINE_ATTR_ROUTE_SCOPE] = { .key = NM_IP_ROUTE_ATTRIBUTE_SCOPE, + .type = PARSE_LINE_TYPE_ROUTE_SCOPE, + .ignore = PARSE_LINE_AF_FLAG_FOR_IPV6, }, [PARSE_LINE_ATTR_ROUTE_ONLINK] = { .key = NM_IP_ROUTE_ATTRIBUTE_ONLINK, .type = PARSE_LINE_TYPE_FLAG, }, [PARSE_LINE_ATTR_ROUTE_WINDOW] = { .key = NM_IP_ROUTE_ATTRIBUTE_WINDOW, @@ -969,6 +975,9 @@ parse_route_line (const char *line, case PARSE_LINE_TYPE_FLAG: i_words++; goto next; + case PARSE_LINE_TYPE_ROUTE_SCOPE: + i_words++; + goto parse_line_type_route_scope; default: nm_assert_not_reached (); } @@ -989,6 +998,35 @@ parse_route_line (const char *line, "Unrecognized argument (\"to\" is duplicate or \"%s\" is garbage)", w); return -EINVAL; +parse_line_type_route_scope: + s = words[i_words]; + if (!s) + goto err_word_missing_argument; + if (nm_streq (s, "global")) + p_data->v.uint8 = RT_SCOPE_UNIVERSE; + else if (nm_streq (s, "nowhere")) + p_data->v.uint8 = RT_SCOPE_NOWHERE; + else if (nm_streq (s, "host")) + p_data->v.uint8 = RT_SCOPE_HOST; + else if (nm_streq (s, "link")) + p_data->v.uint8 = RT_SCOPE_LINK; + else if (nm_streq (s, "site")) + p_data->v.uint8 = RT_SCOPE_SITE; + else { + p_data->v.uint8 = _nm_utils_ascii_str_to_int64 (s, + 0, + 0, + G_MAXUINT8, + 0);; + if (errno) { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, + "Argument for \"%s\" is not a valid number", w); + return -EINVAL; + } + } + i_words++; + goto next; + parse_line_type_uint8: s = words[i_words]; if (!s) @@ -1149,6 +1187,7 @@ next: switch (p_info->type) { case PARSE_LINE_TYPE_UINT8: + case PARSE_LINE_TYPE_ROUTE_SCOPE: nm_ip_route_set_attribute (route, p_info->key, g_variant_new_byte (p_data->v.uint8)); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index c457b810ea..ea2cef803c 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -2084,6 +2084,8 @@ get_route_attributes_string (NMIPRoute *route, int family) /* we also have a corresponding attribute with the numeric value. The * lock setting is handled above. */ } + } else if (nm_streq (names[i], NM_IP_ROUTE_ATTRIBUTE_SCOPE)) { + g_string_append_printf (str, "%s %u", names[i], (unsigned) g_variant_get_byte (attr)); } else if (nm_streq (names[i], NM_IP_ROUTE_ATTRIBUTE_TOS)) { g_string_append_printf (str, "%s 0x%02x", names[i], (unsigned) g_variant_get_byte (attr)); } else if (nm_streq (names[i], NM_IP_ROUTE_ATTRIBUTE_TABLE)) { diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/route-test-wired-static-routes b/src/settings/plugins/ifcfg-rh/tests/network-scripts/route-test-wired-static-routes index 8d6aaac2f6..5d02c62ef3 100644 --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/route-test-wired-static-routes +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/route-test-wired-static-routes @@ -6,10 +6,10 @@ ADDRESS1=44.55.66.77 NETMASK1=255.255.255.255 GATEWAY1=192.168.1.7 METRIC1=3 -OPTIONS1="mtu lock 9000 cwnd 12 src 1.1.1.1 tos 0x28 window 30000 initcwnd lock 13 initrwnd 14" +OPTIONS1="mtu lock 9000 cwnd 12 src 1.1.1.1 tos 0x28 window 30000 scope 10 initcwnd lock 13 initrwnd 14" ADDRESS2=44.55.66.78 NETMASK2=255.255.255.255 GATEWAY2=192.168.1.8 METRIC2=3 -OPTIONS2="mtu lock 9000 cwnd 12 src 1.1.1.1 tos 0x28 onlink window 30000 initcwnd lock 13 initrwnd 14" +OPTIONS2="mtu lock 9000 cwnd 12 src 1.1.1.1 tos 0x28 onlink window 30000 initcwnd lock 13 initrwnd 14 scope link" diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 459a78eca0..d3651978f2 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -1334,6 +1334,7 @@ test_read_wired_static_routes (void) nmtst_assert_route_attribute_boolean (ip4_route, NM_IP_ROUTE_ATTRIBUTE_LOCK_MTU, TRUE); nmtst_assert_route_attribute_boolean (ip4_route, NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND, TRUE); nmtst_assert_route_attribute_string (ip4_route, NM_IP_ROUTE_ATTRIBUTE_SRC, "1.1.1.1"); + nmtst_assert_route_attribute_byte (ip4_route, NM_IP_ROUTE_ATTRIBUTE_SCOPE, 10); ip4_route = nm_setting_ip_config_get_route (s_ip4, 2); g_assert (ip4_route); @@ -1351,6 +1352,7 @@ test_read_wired_static_routes (void) nmtst_assert_route_attribute_boolean (ip4_route, NM_IP_ROUTE_ATTRIBUTE_LOCK_INITCWND, TRUE); nmtst_assert_route_attribute_string (ip4_route, NM_IP_ROUTE_ATTRIBUTE_SRC, "1.1.1.1"); nmtst_assert_route_attribute_boolean (ip4_route, NM_IP_ROUTE_ATTRIBUTE_ONLINK, TRUE); + nmtst_assert_route_attribute_byte (ip4_route, NM_IP_ROUTE_ATTRIBUTE_SCOPE, 253); g_object_unref (connection); }