From d944761f835e0cfdb98883401474fdbe23c62233 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 16:34:48 +0100 Subject: [PATCH 1/7] shared/tests: add nmtst_file_set_contents_size() helper --- shared/nm-utils/nm-test-utils.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index fd90c3b5e7..495aa0b35c 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1362,15 +1362,27 @@ nmtst_file_get_contents (const char *filename) return contents; } -#define nmtst_file_set_contents(filename, content) \ +#define nmtst_file_set_contents_size(filename, content, size) \ G_STMT_START { \ GError *_error = NULL; \ gboolean _success; \ + const char *_content = (content); \ + gssize _size = (size); \ \ - _success = g_file_set_contents ((filename), (content), -1, &_error); \ + g_assert (_content); \ + \ + if (_size < 0) { \ + g_assert (_size == -1); \ + _size = strlen (_content); \ + } \ + \ + _success = g_file_set_contents ((filename), _content, _size, &_error); \ nmtst_assert_success (_success, _error); \ } G_STMT_END +#define nmtst_file_set_contents(filename, content) \ + nmtst_file_set_contents_size (filename, content, -1) + /*****************************************************************************/ static inline void From 5aee49aaaee4579e0019242ec9d3b8581f9b88ed Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 16:35:02 +0100 Subject: [PATCH 2/7] ifcfg-rh/tests: add test for utils_has_route_file_new_syntax() --- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) 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 762af6de10..8ce1b0f709 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -10305,6 +10305,42 @@ test_well_known_keys (void) /*****************************************************************************/ +static void +_do_utils_has_route_file_new_syntax_size (gboolean has_new_syntax, + const char *content, + gssize content_len) +{ + nmtst_auto_unlinkfile char *testfile = g_strdup (TEST_SCRATCH_DIR"/utils-has-route-file-new-syntax-test.txt"); + gboolean val; + + nmtst_file_set_contents_size (testfile, content, content_len); + + val = utils_has_route_file_new_syntax (testfile); + + g_assert_cmpint (val, ==, has_new_syntax); +} +#define _do_utils_has_route_file_new_syntax(has_new_syntax, content) \ + _do_utils_has_route_file_new_syntax_size (has_new_syntax, (content), NM_STRLEN (content)) + +static void +test_utils_has_route_file_new_syntax (void) +{ + _do_utils_has_route_file_new_syntax (TRUE, ""); + _do_utils_has_route_file_new_syntax (FALSE, "\0"); + _do_utils_has_route_file_new_syntax (FALSE, "\n"); + _do_utils_has_route_file_new_syntax (FALSE, "ADDRESS=bogus"); + _do_utils_has_route_file_new_syntax (FALSE, "ADDRESS=bogus\0"); + _do_utils_has_route_file_new_syntax (TRUE, "ADDRESS1=b\0ogus\0"); + _do_utils_has_route_file_new_syntax (TRUE, "ADDRESS1=bogus\0"); + _do_utils_has_route_file_new_syntax (TRUE, "\n\n\tADDRESS1=bogus\0"); + _do_utils_has_route_file_new_syntax (FALSE, "\n\n\tADDRESS=bogus\n"); + _do_utils_has_route_file_new_syntax (TRUE, "\n\n\tADDRESS=bogus\n ADDRESS000=\n"); + _do_utils_has_route_file_new_syntax (FALSE, "\n\n\tROUTE1=bogus\n ADDRES=\n"); + _do_utils_has_route_file_new_syntax (FALSE, "\n\n\tADDRESS=bogus\n ADDRESS\000000=\n"); +} + +/*****************************************************************************/ + #define TPATH "/settings/plugins/ifcfg-rh/" #define TEST_IFCFG_WIFI_OPEN_SSID_LONG_QUOTED TEST_IFCFG_DIR"/ifcfg-test-wifi-open-ssid-long-quoted" @@ -10602,6 +10638,7 @@ int main (int argc, char **argv) g_test_add_func (TPATH "tc/read", test_tc_read); g_test_add_func (TPATH "tc/write", test_tc_write); g_test_add_func (TPATH "utils/test_well_known_keys", test_well_known_keys); + g_test_add_func (TPATH "utils/test_utils_has_route_file_new_syntax", test_utils_has_route_file_new_syntax); return g_test_run (); } From da4a3dd24ca79b9cac83790416f02b5c0ae78b2d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 15:11:36 +0100 Subject: [PATCH 3/7] ifcfg-rh: don't use GRegex in utils_has_route_file_new_syntax() It's simple enough to iterate the file content line by line and search for a suitable prefix. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.c | 50 +++++++++++++------ 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c index a6a4d381ec..843d2159a2 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c @@ -260,27 +260,47 @@ utils_get_route_ifcfg (const char *parent, gboolean should_create) gboolean utils_has_route_file_new_syntax (const char *filename) { - char *contents = NULL; - gsize len = 0; - gboolean ret = FALSE; - const char *pattern = "^[[:space:]]*ADDRESS[0-9]+="; + gs_free char *contents_data = NULL; + const char *contents; + gsize len; g_return_val_if_fail (filename != NULL, TRUE); - if (!g_file_get_contents (filename, &contents, &len, NULL)) + if (!g_file_get_contents (filename, &contents_data, &len, NULL)) return TRUE; - if (len <= 0) { - ret = TRUE; - goto gone; + if (len <= 0) + return TRUE; + + contents = contents_data; + + while (TRUE) { + const char *line = contents; + char *eol; + + /* matches regex "^[[:space:]]*ADDRESS[0-9]+=" */ + + eol = (char *) strchr (contents, '\n'); + if (eol) { + eol[0] = '\0'; + contents = &eol[1]; + } + + line = nm_str_skip_leading_spaces (line); + if (NM_STR_HAS_PREFIX (line, "ADDRESS")) { + line += NM_STRLEN ("ADDRESS"); + if (g_ascii_isdigit (line[0])) { + while (g_ascii_isdigit ((++line)[0])) { + /* pass */ + } + if (line[0] == '=') + return TRUE; + } + } + + if (!eol) + return FALSE; } - - if (g_regex_match_simple (pattern, contents, G_REGEX_MULTILINE, 0)) - ret = TRUE; - -gone: - g_free (contents); - return ret; } gboolean From 9f35d9e49e39505b58b5c90d4239d36999c3c944 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 16:58:23 +0100 Subject: [PATCH 4/7] ifcfg-rh: split reading file and parsing in utils_has_route_file_new_syntax() function We will need both variants, so that the caller can read the file only once. Note that also utils_has_route_file_new_syntax_content() will restore the original contents and not modify the input (from point of view of the caller). In practice it will momentarily modify the content. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.c | 21 +++++++++++++++---- .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.h | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c index 843d2159a2..2bc32e052e 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c @@ -261,7 +261,6 @@ gboolean utils_has_route_file_new_syntax (const char *filename) { gs_free char *contents_data = NULL; - const char *contents; gsize len; g_return_val_if_fail (filename != NULL, TRUE); @@ -269,14 +268,20 @@ utils_has_route_file_new_syntax (const char *filename) if (!g_file_get_contents (filename, &contents_data, &len, NULL)) return TRUE; + return utils_has_route_file_new_syntax_content (contents_data, len); +} + +gboolean +utils_has_route_file_new_syntax_content (const char *contents, + gsize len) +{ if (len <= 0) return TRUE; - contents = contents_data; - while (TRUE) { const char *line = contents; char *eol; + gboolean found = FALSE; /* matches regex "^[[:space:]]*ADDRESS[0-9]+=" */ @@ -294,10 +299,18 @@ utils_has_route_file_new_syntax (const char *filename) /* pass */ } if (line[0] == '=') - return TRUE; + found = TRUE; } } + if (eol) { + /* restore the line ending. We don't want to mangle the content from + * POV of the caller. */ + eol[0] = '\n'; + } + + if (found) + return TRUE; if (!eol) return FALSE; } diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h index 29c9df7af7..4aed0dff51 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h @@ -76,6 +76,8 @@ shvarFile *utils_get_keys_ifcfg (const char *parent, gboolean should_create); shvarFile *utils_get_route_ifcfg (const char *parent, gboolean should_create); gboolean utils_has_route_file_new_syntax (const char *filename); +gboolean utils_has_route_file_new_syntax_content (const char *contents, + gsize len); gboolean utils_has_complex_routes (const char *filename, int addr_family); gboolean utils_is_ifcfg_alias_file (const char *alias, const char *ifcfg); From b1fa27f9bd1a7c1452af4220fdf00db8b0a28abf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 17:07:12 +0100 Subject: [PATCH 5/7] ifcfg-rh: split reading file and parsing from read_route_file() function Split the parsing of the file content to a separate function. Next we will load IPv4 files only once, and thus only need to parse the content without reading it. Note that the function temporarily modifies the input buffer, but restores the original content before returning. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 69 +++++++++++++------ 1 file changed, 47 insertions(+), 22 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 32364aed55..c59bdbafe6 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -1345,39 +1345,40 @@ read_one_ip4_route (shvarFile *ifcfg, } static gboolean -read_route_file (int addr_family, - const char *filename, - NMSettingIPConfig *s_ip, - GError **error) +read_route_file_parse (int addr_family, + const char *filename, + const char *contents, + gsize len, + NMSettingIPConfig *s_ip, + GError **error) { - gs_free char *contents = NULL; - char *contents_rest = NULL; - const char *line; - gsize len = 0; gsize line_num; - g_return_val_if_fail (filename, FALSE); - g_return_val_if_fail ( (addr_family == AF_INET && NM_IS_SETTING_IP4_CONFIG (s_ip)) - || (addr_family == AF_INET6 && NM_IS_SETTING_IP6_CONFIG (s_ip)), FALSE); - g_return_val_if_fail (!error || !*error, FALSE); + nm_assert (filename); + nm_assert (addr_family == nm_setting_ip_config_get_addr_family (s_ip)); + nm_assert (!error || !*error); - if ( !g_file_get_contents (filename, &contents, &len, NULL) - || !len) { + if (len <= 0) return TRUE; /* missing/empty = success */ - } line_num = 0; - for (line = strtok_r (contents, "\n", &contents_rest); - line; - line = strtok_r (NULL, "\n", &contents_rest)) { + while (TRUE) { nm_auto_unref_ip_route NMIPRoute *route = NULL; gs_free_error GError *local = NULL; + const char *line = contents; + char *eol; int e; + eol = strchr (contents, '\n'); + if (eol) { + eol[0] = '\0'; + contents = &eol[1]; + } + line_num++; if (parse_route_line_is_comment (line)) - continue; + goto next; e = parse_route_line (line, addr_family, NULL, &route, &local); @@ -1389,14 +1390,38 @@ read_route_file (int addr_family, * entire connection. */ PARSE_WARNING ("ignoring invalid route at \"%s\" (%s:%lu): %s", line, filename, (long unsigned) line_num, local->message); } - continue; + goto next; } if (!nm_setting_ip_config_add_route (s_ip, route)) PARSE_WARNING ("duplicate IPv%c route", addr_family == AF_INET ? '4' : '6'); - } - return TRUE; +next: + if (!eol) + return TRUE; + + /* restore original content. */ + eol[0] = '\n'; + } +} + +static gboolean +read_route_file (int addr_family, + const char *filename, + NMSettingIPConfig *s_ip, + GError **error) +{ + gs_free char *contents = NULL; + gsize len; + + nm_assert (filename); + nm_assert (addr_family == nm_setting_ip_config_get_addr_family (s_ip)); + nm_assert (!error || !*error); + + if (!g_file_get_contents (filename, &contents, &len, NULL)) + return TRUE; /* missing/empty = success */ + + return read_route_file_parse (addr_family, filename, contents, len, s_ip, error); } static void From 16ba286e3002b800a6f525309c126e5e8a7cdb6e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 17:37:24 +0100 Subject: [PATCH 6/7] ifcfg-rh: expose constructor for shvarFile that doesn't read content from file --- src/settings/plugins/ifcfg-rh/shvar.c | 64 ++++++++++++++++----------- src/settings/plugins/ifcfg-rh/shvar.h | 8 ++++ 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index 7cdab0297e..f2a1dd7875 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.c +++ b/src/settings/plugins/ifcfg-rh/shvar.c @@ -71,6 +71,10 @@ struct _shvarFile { /*****************************************************************************/ +static void _line_link_parse (shvarFile *s, const char *value, gsize len); + +/*****************************************************************************/ + #define ASSERT_key_is_well_known(key) \ nm_assert ( ({ \ const char *_key = (key); \ @@ -629,16 +633,33 @@ out_error: /*****************************************************************************/ -static shvarFile * -svFile_new (const char *name) +shvarFile * +svFile_new (const char *name, + int fd, + const char *content) { shvarFile *s; + const char *p; + const char *q; + + nm_assert (name); + nm_assert (fd >= -1); + + s = g_slice_new (shvarFile); + *s = (shvarFile) { + .fileName = g_strdup (name), + .fd = fd, + .lst_head = C_LIST_INIT (s->lst_head), + .lst_idx = g_hash_table_new (nm_pstr_hash, nm_pstr_equal), + }; + + if (content) { + for (p = content; (q = strchr (p, '\n')) != NULL; p = q + 1) + _line_link_parse (s, p, q - p); + if (p[0]) + _line_link_parse (s, p, strlen (p)); + } - s = g_slice_new0 (shvarFile); - s->fd = -1; - s->fileName = g_strdup (name); - c_list_init (&s->lst_head); - s->lst_idx = g_hash_table_new (nm_pstr_hash, nm_pstr_equal); return s; } @@ -839,11 +860,9 @@ do_link: static shvarFile * svOpenFileInternal (const char *name, gboolean create, GError **error) { - shvarFile *s; gboolean closefd = FALSE; int errsv = 0; - gs_free char *arena = NULL; - const char *p, *q; + gs_free char *content = NULL; gs_free_error GError *local = NULL; nm_auto_close int fd = -1; @@ -860,7 +879,7 @@ svOpenFileInternal (const char *name, gboolean create, GError **error) if (fd < 0) { if (create) - return svFile_new (name); + return svFile_new (name, -1, NULL); g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv), "Could not read file '%s': %s", @@ -872,12 +891,12 @@ svOpenFileInternal (const char *name, gboolean create, GError **error) closefd, 10 * 1024 * 1024, NM_UTILS_FILE_GET_CONTENTS_FLAG_NONE, - &arena, + &content, NULL, NULL, &local)) { if (create) - return svFile_new (name); + return svFile_new (name, -1, NULL); g_set_error (error, G_FILE_ERROR, local->domain == G_FILE_ERROR ? local->code : G_FILE_ERROR_FAILED, @@ -886,21 +905,14 @@ svOpenFileInternal (const char *name, gboolean create, GError **error) return NULL; } - s = svFile_new (name); - - for (p = arena; (q = strchr (p, '\n')) != NULL; p = q + 1) - _line_link_parse (s, p, q - p); - if (p[0]) - _line_link_parse (s, p, strlen (p)); - /* closefd is set if we opened the file read-only, so go ahead and * close it, because we can't write to it anyway */ - if (!closefd) { - nm_assert (fd > 0); - s->fd = nm_steal_fd (&fd); - } - - return s; + nm_assert (closefd || fd >= 0); + return svFile_new (name, + !closefd + ? nm_steal_fd (&fd) + : -1, + content); } /* Open the file , return shvarFile on success, NULL on failure */ diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h index ac1dd2c67f..410284f8cb 100644 --- a/src/settings/plugins/ifcfg-rh/shvar.h +++ b/src/settings/plugins/ifcfg-rh/shvar.h @@ -24,12 +24,20 @@ const char *svFileGetName (const shvarFile *s); void _nmtst_svFileSetName (shvarFile *s, const char *fileName); void _nmtst_svFileSetModified (shvarFile *s); +/*****************************************************************************/ + +shvarFile *svFile_new (const char *name, + int fd, + const char *content); + /* Create the file , return a shvarFile (never fails) */ shvarFile *svCreateFile (const char *name); /* Open the file , return shvarFile on success, NULL on failure */ shvarFile *svOpenFile (const char *name, GError **error); +/*****************************************************************************/ + const char *svFindFirstNumberedKey (shvarFile *s, const char *key_prefix); /* Get the value associated with the key, and leave the current pointer From 4fc7c7eec01b75c37c5690e13e62c5585767a5ef Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 Jan 2020 17:24:18 +0100 Subject: [PATCH 7/7] ifcfg-rh: avoid creaing route file twice in make_ip4_setting() Just read the content once. It's wasteful to read the file twice while parsing. But what is worse, the file content can change any time we read it. We make decisions based on what we read, and hence we should only read the file once in the parsing process to get one consistent result. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 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 c59bdbafe6..9110d68d75 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -1620,7 +1620,6 @@ make_ip4_setting (shvarFile *ifcfg, int i; guint32 a; gboolean has_key; - shvarFile *route_ifcfg; gboolean never_default; gint64 i64; int priority; @@ -1857,32 +1856,34 @@ make_ip4_setting (shvarFile *ifcfg, /* Static routes - route- file */ route_path = utils_get_route_path (svFileGetName (ifcfg)); - if (!routes_read) { - /* NOP */ - } else if (utils_has_route_file_new_syntax (route_path)) { - /* Parse route file in new syntax */ - route_ifcfg = utils_get_route_ifcfg (svFileGetName (ifcfg), FALSE); - if (route_ifcfg) { - for (i = 0;; i++) { - NMIPRoute *route = NULL; + if (routes_read) { + gs_free char *contents = NULL; + gsize len; - if (!read_one_ip4_route (route_ifcfg, i, &route, error)) { - svCloseFile (route_ifcfg); + if (!g_file_get_contents (route_path, &contents, &len, NULL)) + len = 0; + + if (utils_has_route_file_new_syntax_content (contents, len)) { + nm_auto_shvar_file_close shvarFile *route_ifcfg = NULL; + + /* Parse route file in new syntax */ + route_ifcfg = svFile_new (route_path, -1, contents); + for (i = 0;; i++) { + nm_auto_unref_ip_route NMIPRoute *route = NULL; + + if (!read_one_ip4_route (route_ifcfg, i, &route, error)) return NULL; - } if (!route) break; if (!nm_setting_ip_config_add_route (s_ip4, route)) PARSE_WARNING ("duplicate IP4 route"); - nm_ip_route_unref (route); } - svCloseFile (route_ifcfg); + } else { + if (!read_route_file_parse (AF_INET, route_path, contents, len, s_ip4, error)) + return NULL; } - } else { - if (!read_route_file (AF_INET, route_path, s_ip4, error)) - return NULL; } /* Legacy value NM used for a while but is incorrect (rh #459370) */