ifcfg-rh: merge branch 'th/ifcfg-parse-route-file-cleanup'

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/392

7 files changed, 209 insertions, 79 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); \
+		\
+		g_assert (_content); \
+		\
+		if (_size < 0) { \
+			g_assert (_size == -1); \
+			_size = strlen (_content); \
+		} \
 		\
-		_success = g_file_set_contents ((filename), (content), -1, &_error); \
+		_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
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..9110d68d75 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');
+
+next:
+		if (!eol)
+			return TRUE;
+
+		/* restore original content. */
+		eol[0] = '\n';
 	}
+}
 
-	return TRUE;
+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
@@ -1595,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;
@@ -1832,32 +1856,34 @@ make_ip4_setting (shvarFile *ifcfg,
 	/* Static routes  - route-<name> 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) {
+	if (routes_read) {
+		gs_free char *contents = NULL;
+		gsize len;
+
+		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++) {
-				NMIPRoute *route = NULL;
+				nm_auto_unref_ip_route NMIPRoute *route = NULL;
 
-				if (!read_one_ip4_route (route_ifcfg, i, &route, error)) {
-					svCloseFile (route_ifcfg);
+				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) */
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..2bc32e052e 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,60 @@ 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;
+	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;
-	}
+	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;
+
+	while (TRUE) {
+		const char *line = contents;
+		char *eol;
+		gboolean found = FALSE;
+
+		/* matches regex "^[[:space:]]*ADDRESS[0-9]+=" */
+
+		eol = (char *) strchr (contents, '\n');
+		if (eol) {
+			eol[0] = '\0';
+			contents = &eol[1];
+		}
 
-	if (g_regex_match_simple (pattern, contents, G_REGEX_MULTILINE, 0))
-		ret = TRUE;
+		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] == '=')
+					found = TRUE;
+			}
+		}
 
-gone:
-	g_free (contents);
-	return ret;
+		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;
+	}
 }
 
 gboolean
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);
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 <name>, 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 <name>, return a shvarFile (never fails) */
 shvarFile *svCreateFile (const char *name);
 
 /* Open the file <name>, 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
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 ();
 }