ifcfg-rh: improve handling of empty strings in svUnescape()

- a key
    FOO=''
  would still allocate a temporary GString and return the allocated
  empty string. Don't do that. This saves the g_free() in
  svGetValueString() for this common case. We should return
  an allocated string only if it is necessary. It is not necessary
  for the "" case, and it is inconsistent.

- when returning an empty string, always return the static string "".
  No need to seek to the end of value, and return a pointer to that
  string.
  This happens for example in the case
    FOO= # empty value, but trailing stuff
    FOO=""
    FOO=$'\Uxhallo'

4 files changed, 58 insertions, 22 deletions

diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c
index b4b3c0c3f0..22c942fce2 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.c
+++ b/src/settings/plugins/ifcfg-rh/shvar.c
@@ -324,7 +324,7 @@ const char *
 svUnescape (const char *value, char **to_free)
 {
 	gsize i, j;
-	nm_auto_free_gstring GString *str = NULL;
+	GString *str = NULL;
 	int looks_like_old_svescaped = -1;
 
 	/* we handle bash syntax here (note that ifup has #!/bin/bash.
@@ -569,27 +569,34 @@ loop1_next: ;
 	nm_assert_not_reached ();
 
 out_value:
-	if (str) {
-		*to_free = g_string_free (str, FALSE);
-		str = NULL;
-		return *to_free;
-	} else if (i == 0) {
+	if (i == 0) {
+		nm_assert (!str);
 		*to_free = NULL;
-		/* we could just return "", but I prefer returning a
-		 * pointer into @value for consistency. Thus, seek to the
-		 * end. */
-		while (value[0])
-			value++;
-		return value;
-	} else if (value[i] != '\0') {
+		return "";
+	}
+
+	if (str) {
+		if (str->len == 0 || str->str[0] == '\0') {
+			g_string_free (str, TRUE);
+			*to_free = NULL;
+			return "";
+		} else {
+			*to_free = g_string_free (str, FALSE);
+			return *to_free;
+		}
+	}
+
+	if (value[i] != '\0') {
 		*to_free = g_strndup (value, i);
 		return *to_free;
-	} else {
-		*to_free = NULL;
-		return value;
 	}
 
+	*to_free = NULL;
+	return value;
+
 out_error:
+	if (str)
+		g_string_free (str, TRUE);
 	*to_free = NULL;
 	return NULL;
 }
@@ -848,7 +855,7 @@ svGetValueString (shvarFile *s, const char *key)
 		return NULL;
 	}
 	if (!value[0]) {
-		g_free (to_free);
+		nm_assert (!to_free);
 		return NULL;
 	}
 	return to_free ?: g_strdup (value);
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4 b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4
index 2a0198d30b..a7156e3373 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4
@@ -14,6 +14,10 @@ NAME=l2
 #L2
 NAME=l3
 
+some_key1=''
+some_key2=$'\U0x'
+some_key3=$'x\U0'
+
 #L4
 NAME='
 NAME=l4x
diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4.expected b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4.expected
index 7f9a2d0616..674df840b7 100644
--- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4.expected
+++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-write-unknown-4.expected
@@ -12,6 +12,10 @@
 
 #L2
 
+some_key1=''
+some_key2=$'\U0x'
+some_key3=$'x\U0'
+
 #L4
 NAME=set-by-test1
 #NM: '
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 4518c327ee..0a58da6bd1 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -73,11 +73,19 @@
 		_f; \
 	})
 
-#define _svGetValue_check(sv, key, expected_value) \
+#define _svGetValue_check(f, key, expected_value) \
 	G_STMT_START { \
+		const char *_val; \
 		gs_free char *_to_free = NULL; \
+		gs_free char *_val_string = NULL; \
+		shvarFile *const _f = (f); \
+		const char *const _key = (key); \
 		\
-		g_assert_cmpstr (svGetValue (sv, key, &_to_free), ==, expected_value); \
+		_val_string = svGetValueString (_f, _key); \
+		_val = svGetValue (_f, _key, &_to_free); \
+		g_assert_cmpstr (_val, ==, (expected_value)); \
+		g_assert (   (!_val_string && (!_val || !_val[0])) \
+		          || ( _val_string && nm_streq0 (_val, _val_string))); \
 	} G_STMT_END
 
 #define _writer_update_connection(connection, ifcfg_dir, filename) \
@@ -8106,16 +8114,24 @@ static const char *
 _svUnescape (const char *str, char **to_free)
 {
 	const char *s;
+	gs_free char *str_free = NULL;
 
 	g_assert (str);
 	g_assert (to_free);
 
+	if (str[0] == '\0') {
+		/* avoid static string "" */
+		str = (str_free = g_strdup (str));
+	}
+
 	s = svUnescape (str, to_free);
-	if (*to_free)
+	if (*to_free) {
 		g_assert (s == *to_free);
-	else {
+		g_assert (s[0]);
+	} else {
 		g_assert (   s == NULL
-		          || (s >= str && s <= strchr (str, '\0')));
+		          || (!s[0] && (s <  str || s >  strchr (str, '\0')))
+		          || ( s[0] &&  s >= str && s <= strchr (str, '\0') ));
 	}
 	return s;
 }
@@ -8443,6 +8459,11 @@ test_write_unknown (gconstpointer test_data)
 		svSetValue (sv, "NAME2", "set-by-test2");
 		svSetValue (sv, "NAME3", "set-by-test3");
 
+		_svGetValue_check (sv, "some_key", NULL);
+		_svGetValue_check (sv, "some_key1", "");
+		_svGetValue_check (sv, "some_key2", "");
+		_svGetValue_check (sv, "some_key3", "x");
+
 		_svGetValue_check (sv, "NAME", "set-by-test1");
 		_svGetValue_check (sv, "NAME2", "set-by-test2");
 		_svGetValue_check (sv, "NAME3", "set-by-test3");