From 88e18c9de80e6fd6d842d7e28dc13fa80ab163a2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 11 Nov 2016 12:20:23 +0100 Subject: [PATCH] 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' --- src/settings/plugins/ifcfg-rh/shvar.c | 45 +++++++++++-------- .../ifcfg-test-write-unknown-4 | 4 ++ .../ifcfg-test-write-unknown-4.expected | 4 ++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 31 ++++++++++--- 4 files changed, 60 insertions(+), 24 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c index b4b3c0c3f..22c942fce 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') { - *to_free = g_strndup (value, i); - return *to_free; - } else { - *to_free = NULL; - return value; + 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; + } + + *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 2a0198d30..a7156e337 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 7f9a2d061..674df840b 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 4518c327e..0a58da6bd 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");