From ccbfda5c0853a0b64661bee2b73b549a9849ecad Mon Sep 17 00:00:00 2001
From: Thomas Haller <thaller@redhat.com>
Date: Tue, 21 Feb 2017 15:26:58 +0100
Subject: [PATCH] ifcfg: complete shvar API with svGetValue_cp() and
 svGetValueStr()

Add svGetValue_cp() and svGetValueStr() for completeness.

Currently, we mostly use svGetValueStr_cp(), which I think is wrong
because for most cases we should instead not ignore empty values -- that
is, svGetValue_cp() would be a better choice.

Also, I think that the non *_cp() API should be preferred in many cases
because it avoids cloning the value in many cases. The API is not
necessarily less favorable either:

    gs_free char *value = NULL;

    value = svGetValue_cp (s, key);
    if (value)
        ...

vs.

    gs_free char *value_to_free = NULL;
    const char *value;

    value = svGetValue (s, key, &value_to_free);
    if (value)
        ...

Add the two missing variants, so that future code can use what fits
best, not following undesired practices because seemingly there is
no alternative.
---
 src/settings/plugins/ifcfg-rh/shvar.c | 74 +++++++++++++++++++++++----
 src/settings/plugins/ifcfg-rh/shvar.h |  5 +-
 2 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c
index d4c1ad49ef..619a54afb8 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.c
+++ b/src/settings/plugins/ifcfg-rh/shvar.c
@@ -902,19 +902,74 @@ _svGetValue (shvarFile *s, const char *key, char **to_free)
 	return NULL;
 }
 
+/* Returns the value for key. The value is either owned by @s
+ * or returned as to_free. This aims to avoid cloning the string.
+ *
+ * - like svGetValue_cp(), but avoids cloning the value if possible.
+ * - like svGetValueStr(), but does not ignore empty string values.
+ */
 const char *
 svGetValue (shvarFile *s, const char *key, char **to_free)
 {
-	g_return_val_if_fail (s != NULL, NULL);
-	g_return_val_if_fail (key != NULL, NULL);
+	g_return_val_if_fail (s, NULL);
+	g_return_val_if_fail (key, NULL);
 	g_return_val_if_fail (to_free, NULL);
 
 	return _svGetValue (s, key, to_free);
 }
 
-/* Get the value associated with the key, and leave the current pointer
- * pointing at the line containing the value.  The char* returned MUST
- * be freed by the caller.
+/* Returns the value for key. The value is either owned by @s
+ * or returned as to_free. This aims to avoid cloning the string.
+ *
+ * - like svGetValue(), but does not return an empty string.
+ * - like svGetValueStr_cp(), but avoids cloning the value if possible.
+ */
+const char *
+svGetValueStr (shvarFile *s, const char *key, char **to_free)
+{
+	const char *value;
+
+	g_return_val_if_fail (s, NULL);
+	g_return_val_if_fail (key, NULL);
+	g_return_val_if_fail (to_free, NULL);
+
+	value = _svGetValue (s, key, to_free);
+	if (!value || !value[0]) {
+		nm_assert (!*to_free);
+		return NULL;
+	}
+	return value;
+}
+
+/* Returns the value for key. The returned value must be freed
+ * by the caller.
+ *
+ * - like svGetValue(), but always returns a copy of the value.
+ * - like svGetValueStr_cp(), but does not ignore an empty string.
+ */
+char *
+svGetValue_cp (shvarFile *s, const char *key)
+{
+	char *to_free;
+	const char *value;
+
+	g_return_val_if_fail (s, NULL);
+	g_return_val_if_fail (key, NULL);
+
+	value = _svGetValue (s, key, &to_free);
+	if (!value) {
+		nm_assert (!to_free);
+		return NULL;
+	}
+	return to_free ?: g_strdup (value);
+}
+
+/* Returns the value for key. The returned value must be freed
+ * by the caller.
+ * If the key is unset or the value an empty string, NULL is returned.
+ *
+ * - like svGetValueStr(), but always returns a copy of the value.
+ * - like svGetValue_cp(), but returns NULL instead of an empty string.
  */
 char *
 svGetValueStr_cp (shvarFile *s, const char *key)
@@ -922,12 +977,11 @@ svGetValueStr_cp (shvarFile *s, const char *key)
 	char *to_free;
 	const char *value;
 
+	g_return_val_if_fail (s, NULL);
+	g_return_val_if_fail (key, NULL);
+
 	value = _svGetValue (s, key, &to_free);
-	if (!value) {
-		nm_assert (!to_free);
-		return NULL;
-	}
-	if (!value[0]) {
+	if (!value || !value[0]) {
 		nm_assert (!to_free);
 		return NULL;
 	}
diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h
index 9f82e25970..5c384bb229 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.h
+++ b/src/settings/plugins/ifcfg-rh/shvar.h
@@ -49,6 +49,9 @@ shvarFile *svOpenFile (const char *name, GError **error);
  * be freed by the caller.
  */
 const char *svGetValue (shvarFile *s, const char *key, char **to_free);
+char *svGetValue_cp (shvarFile *s, const char *key);
+
+const char *svGetValueStr (shvarFile *s, const char *key, char **to_free);
 char *svGetValueStr_cp (shvarFile *s, const char *key);
 
 gint svParseBoolean (const char *value, gint def);
@@ -66,8 +69,8 @@ gint64 svGetValueInt64 (shvarFile *s, const char *key, guint base, gint64 min, g
  * the key=value pair after that line.  Otherwise, prepend the pair
  * to the top of the file.
  */
-void svSetValueStr (shvarFile *s, const char *key, const char *value);
 void svSetValue (shvarFile *s, const char *key, const char *value);
+void svSetValueStr (shvarFile *s, const char *key, const char *value);
 void svSetValueBoolean (shvarFile *s, const char *key, gboolean value);
 void svSetValueInt64 (shvarFile *s, const char *key, gint64 value);