fixes/improvements to ifcfg-rh shvar code

https://bugzilla.gnome.org/show_bug.cgi?id=726653

7 files changed, 343 insertions, 360 deletions

diff --git a/src/settings/plugins/ifcfg-rh/plugin.c b/src/settings/plugins/ifcfg-rh/plugin.c
index 1f9ed4726d..0bf16758b5 100644
--- a/src/settings/plugins/ifcfg-rh/plugin.c
+++ b/src/settings/plugins/ifcfg-rh/plugin.c
@@ -646,7 +646,7 @@ plugin_get_hostname (SCPluginIfcfg *plugin)
 		return hostname;
 	}
 
-	network = svNewFile (SC_NETWORK_FILE);
+	network = svOpenFile (SC_NETWORK_FILE, NULL);
 	if (!network) {
 		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "Could not get hostname: failed to read " SC_NETWORK_FILE);
 		return NULL;
@@ -708,10 +708,10 @@ plugin_set_hostname (SCPluginIfcfg *plugin, const char *hostname)
 	g_free (hostname_eol);
 
 	/* Remove "HOSTNAME" from SC_NETWORK_FILE, if present */
-	network = svNewFile (SC_NETWORK_FILE);
+	network = svOpenFile (SC_NETWORK_FILE, NULL);
 	if (network) {
 		svSetValue (network, "HOSTNAME", NULL, FALSE);
-		svWriteFile (network, 0644);
+		svWriteFile (network, 0644, NULL);
 		svCloseFile (network);
 	}
 
diff --git a/src/settings/plugins/ifcfg-rh/reader.c b/src/settings/plugins/ifcfg-rh/reader.c
index 70e58e9e03..3b0379c200 100644
--- a/src/settings/plugins/ifcfg-rh/reader.c
+++ b/src/settings/plugins/ifcfg-rh/reader.c
@@ -682,7 +682,7 @@ read_full_ip4_address (shvarFile *ifcfg,
 		gboolean read_success;
 
 		/* If no gateway in the ifcfg, try /etc/sysconfig/network instead */
-		network_ifcfg = svNewFile (network_file);
+		network_ifcfg = svOpenFile (network_file, NULL);
 		if (network_ifcfg) {
 			read_success = read_ip4_address (network_ifcfg, "GATEWAY", &tmp, error);
 			svCloseFile (network_ifcfg);
@@ -1066,7 +1066,7 @@ parse_full_ip6_address (shvarFile *ifcfg,
 	}
 	if (!value) {
 		/* If no gateway in the ifcfg, try global /etc/sysconfig/network instead */
-		network_ifcfg = svNewFile (network_file);
+		network_ifcfg = svOpenFile (network_file, NULL);
 		if (network_ifcfg) {
 			value = svGetValue (network_ifcfg, "IPV6_DEFAULTGW", FALSE);
 			svCloseFile (network_ifcfg);
@@ -1281,7 +1281,7 @@ make_ip4_setting (shvarFile *ifcfg,
 	never_default = !svTrueValue (ifcfg, "DEFROUTE", TRUE);
 
 	/* Then check if GATEWAYDEV; it's global and overrides DEFROUTE */
-	network_ifcfg = svNewFile (network_file);
+	network_ifcfg = svOpenFile (network_file, NULL);
 	if (network_ifcfg) {
 		char *gatewaydev;
 
@@ -1645,7 +1645,7 @@ make_ip6_setting (shvarFile *ifcfg,
 	 * they are global and override IPV6_DEFROUTE
 	 * When both are set, the device specified in IPV6_DEFAULTGW takes preference.
 	 */
-	network_ifcfg = svNewFile (network_file);
+	network_ifcfg = svOpenFile (network_file, NULL);
 	if (network_ifcfg) {
 		char *ipv6_defaultgw, *ipv6_defaultdev;
 		char *default_dev = NULL;
@@ -1680,7 +1680,7 @@ make_ip6_setting (shvarFile *ifcfg,
 	str_value = svGetValue (ifcfg, "IPV6INIT", FALSE);
 	ipv6init = svTrueValue (ifcfg, "IPV6INIT", FALSE);
 	if (!str_value) {
-		network_ifcfg = svNewFile (network_file);
+		network_ifcfg = svOpenFile (network_file, NULL);
 		if (network_ifcfg) {
 			ipv6init = svTrueValue (network_ifcfg, "IPV6INIT", FALSE);
 			svCloseFile (network_ifcfg);
@@ -4991,7 +4991,7 @@ uuid_from_file (const char *filename)
 	if (!ifcfg_name)
 		return NULL;
 
-	ifcfg = svNewFile (filename);
+	ifcfg = svOpenFile (filename, NULL);
 	if (!ifcfg)
 		return NULL;
 
@@ -5077,12 +5077,9 @@ connection_from_file (const char *filename,
 		return NULL;
 	}
 
-	parsed = svNewFile (filename);
-	if (!parsed) {
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-		             "Couldn't parse file '%s'", filename);
+	parsed = svOpenFile (filename, error);
+	if (!parsed)
 		return NULL;
-	}
 
 	if (!svTrueValue (parsed, "NM_CONTROLLED", TRUE)) {
 		g_assert (out_unhandled != NULL);
diff --git a/src/settings/plugins/ifcfg-rh/shvar.c b/src/settings/plugins/ifcfg-rh/shvar.c
index cf03c6dab4..4ba8a631f1 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.c
+++ b/src/settings/plugins/ifcfg-rh/shvar.c
@@ -1,15 +1,10 @@
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 /*
  * shvar.c
  *
  * Implementation of non-destructively reading/writing files containing
  * only shell variable declarations and full-line comments.
  *
- * Includes explicit inheritance mechanism intended for use with
- * Red Hat Linux ifcfg-* files.  There is no protection against
- * inheritance loops; they will generally cause stack overflows.
- * Furthermore, they are only intended for one level of inheritance;
- * the value setting algorithm assumes this.
- *
  * Copyright 1999,2000 Red Hat, Inc.
  *
  * This is free software; you can redistribute it and/or modify it
@@ -28,6 +23,7 @@
  *
  */
 
+#include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -39,76 +35,100 @@
 #include "shvar.h"
 
 /* Open the file <name>, returning a shvarFile on success and NULL on failure.
-   Add a wrinkle to let the caller specify whether or not to create the file
-   (actually, return a structure anyway) if it doesn't exist. */
+ * Add a wrinkle to let the caller specify whether or not to create the file
+ * (actually, return a structure anyway) if it doesn't exist.
+ */
 static shvarFile *
-svOpenFile(const char *name, gboolean create)
+svOpenFileInternal (const char *name, gboolean create, GError **error)
 {
-    shvarFile *s = NULL;
-    int closefd = 0;
-
-    s = g_malloc0(sizeof(shvarFile));
-
-    s->fd = -1;
-    if (create)
-	s->fd = open(name, O_RDWR); /* NOT O_CREAT */
- 
-    if (!create || s->fd == -1) {
-	/* try read-only */
-	s->fd = open(name, O_RDONLY); /* NOT O_CREAT */
-	if (s->fd != -1) closefd = 1;
-    }
-    s->fileName = g_strdup(name);
-
-    if (s->fd != -1) {
-	struct stat buf;
-	char *p, *q;
-
-	if (fstat(s->fd, &buf) < 0) goto bail;
-	s->arena = g_malloc0(buf.st_size + 1);
-
-	if (read(s->fd, s->arena, buf.st_size) < 0) goto bail;
-
-	/* we'd use g_strsplit() here, but we want a list, not an array */
-	for(p = s->arena; (q = strchr(p, '\n')) != NULL; p = q + 1) {
-		s->lineList = g_list_append(s->lineList, g_strndup(p, q - p));
+	shvarFile *s = NULL;
+	gboolean closefd = FALSE;
+	int errsv;
+
+	s = g_slice_new0 (shvarFile);
+
+	s->fd = -1;
+	if (create)
+		s->fd = open (name, O_RDWR); /* NOT O_CREAT */
+
+	if (!create || s->fd == -1) {
+		/* try read-only */
+		s->fd = open (name, O_RDONLY); /* NOT O_CREAT */
+		if (s->fd == -1)
+			errsv = errno;
+		else
+			closefd = TRUE;
 	}
+	s->fileName = g_strdup (name);
+
+	if (s->fd != -1) {
+		struct stat buf;
+		char *arena, *p, *q;
+		ssize_t nread, total = 0;
 
-	/* 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) {
-	    close(s->fd);
-	    s->fd = -1;
+		if (fstat (s->fd, &buf) < 0) {
+			errsv = errno;
+			goto bail;
+		}
+		arena = g_malloc (buf.st_size + 1);
+		arena[buf.st_size] = '\0';
+
+		while (total < buf.st_size) {
+			nread = read (s->fd, arena + total, buf.st_size - total);
+			if (nread == -1 && errno == EINTR)
+				continue;
+			if (nread <= 0) {
+				errsv = errno;
+				goto bail;
+			}
+			total += nread;
+		}
+
+		/* we'd use g_strsplit() here, but we want a list, not an array */
+		for (p = arena; (q = strchr (p, '\n')) != NULL; p = q + 1)
+			s->lineList = g_list_append (s->lineList, g_strndup (p, q - p));
+		g_free (arena);
+
+		/* 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) {
+			close (s->fd);
+			s->fd = -1;
+		}
+
+		return s;
 	}
 
-        return s;
-    }
+	if (create)
+		return s;
 
-    if (create) {
-        return s;
-    }
+ bail:
+	if (s->fd != -1)
+		close (s->fd);
+	g_free (s->fileName);
+	g_slice_free (shvarFile, s);
 
-bail:
-    if (s->fd != -1) close(s->fd);
-    g_free (s->arena);
-    g_free (s->fileName);
-    g_free (s);
-    return NULL;
+	g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv),
+	             "Could not read file '%s': %s",
+	             name, errsv ? strerror (errsv) : "Unknown error");
+	return NULL;
 }
 
 /* Open the file <name>, return shvarFile on success, NULL on failure */
 shvarFile *
-svNewFile(const char *name)
+svOpenFile (const char *name, GError **error)
 {
-    return svOpenFile(name, FALSE);
+	return svOpenFileInternal (name, FALSE, error);
 }
 
 /* Create a new file structure, returning actual data if the file exists,
- * and a suitable starting point if it doesn't. */
+ * and a suitable starting point if it doesn't.
+ */
 shvarFile *
-svCreateFile(const char *name)
+svCreateFile (const char *name)
 {
-    return svOpenFile(name, TRUE);
+	return svOpenFileInternal (name, TRUE, NULL);
 }
 
 /* remove escaped characters in place */
@@ -118,7 +138,7 @@ svUnescape (char *s)
 	size_t len, idx_rd = 0, idx_wr = 0;
 	char c;
 
-	len = strlen(s);
+	len = strlen (s);
 	if (len < 2) {
 		if (s[0] == '\\')
 			s[0] = '\0';
@@ -155,7 +175,8 @@ svUnescape (char *s)
 	}
 
 	/* idx_rd points to the first escape. Walk the string and shift the
-	 * characters from idx_rd to idx_wr. */
+	 * characters from idx_rd to idx_wr.
+	 */
 	while ((c = s[idx_rd++])) {
 		if (c == '\\') {
 			if (s[idx_rd] == '\0') {
@@ -176,38 +197,46 @@ svUnescape (char *s)
 static const char escapees[] = "\"'\\$~`";		/* must be escaped */
 static const char spaces[] = " \t|&;()<>";		/* only require "" */
 static const char newlines[] = "\n\r";			/* will be removed */
+
 char *
-svEscape(const char *s) {
-    char *new;
-    int i, j, mangle = 0, space = 0, newline = 0;
-    int newlen, slen;
-
-    slen = strlen(s);
-
-    for (i = 0; i < slen; i++) {
-	if (strchr(escapees, s[i])) mangle++;
-	if (strchr(spaces, s[i])) space++;
-	if (strchr(newlines, s[i])) newline++;
-    }
-    if (!mangle && !space && !newline) return strdup(s);
-
-    newlen = slen + mangle - newline + 3;	/* 3 is extra ""\0 */
-    new = g_malloc0(newlen);
-
-    j = 0;
-    new[j++] = '"';
-    for (i = 0; i < slen; i++) {
-	if (strchr(newlines, s[i]))
-	    continue;
-	if (strchr(escapees, s[i])) {
-	    new[j++] = '\\';
+svEscape (const char *s)
+{
+	char *new;
+	int i, j, mangle = 0, space = 0, newline = 0;
+	int newlen, slen;
+
+	slen = strlen (s);
+
+	for (i = 0; i < slen; i++) {
+		if (strchr (escapees, s[i]))
+			mangle++;
+		if (strchr (spaces, s[i]))
+			space++;
+		if (strchr (newlines, s[i]))
+			newline++;
+	}
+	if (!mangle && !space && !newline)
+		return strdup (s);
+
+	newlen = slen + mangle - newline + 3;	/* 3 is extra ""\0 */
+	new = g_malloc (newlen);
+
+	j = 0;
+	new[j++] = '"';
+	for (i = 0; i < slen; i++) {
+		if (strchr (newlines, s[i]))
+			continue;
+		if (strchr (escapees, s[i])) {
+			new[j++] = '\\';
+		}
+		new[j++] = s[i];
 	}
-	new[j++] = s[i];
-    }
-    new[j++] = '"';
-    g_assert(j == slen + mangle - newline + 2); /* j is the index of the '\0' */
+	new[j++] = '"';
+	new[j++] = '\0'
+;
+	g_assert (j == slen + mangle - newline + 3);
 
-    return new;
+	return new;
 }
 
 /* Get the value associated with the key, and leave the current pointer
@@ -215,216 +244,190 @@ svEscape(const char *s) {
  * be freed by the caller.
  */
 char *
-svGetValue(shvarFile *s, const char *key, gboolean verbatim)
+svGetValue (shvarFile *s, const char *key, gboolean verbatim)
 {
-    char *value = NULL;
-    char *line;
-    char *keyString;
-    int len;
-
-    g_assert(s);
-    g_assert(key);
-
-    keyString = g_malloc0(strlen(key) + 2);
-    strcpy(keyString, key);
-    keyString[strlen(key)] = '=';
-    len = strlen(keyString);
-
-    for (s->current = s->lineList; s->current; s->current = s->current->next) {
-	line = s->current->data;
-	if (!strncmp(keyString, line, len)) {
-	    value = g_strdup(line + len);
-	    if (!verbatim)
-	      svUnescape(value);
-	    break;
+	char *value = NULL;
+	char *line;
+	char *keyString;
+	int len;
+
+	g_return_val_if_fail (s != NULL, NULL);
+	g_return_val_if_fail (key != NULL, NULL);
+
+	keyString = g_strdup_printf ("%s=", key);
+	len = strlen (keyString);
+
+	for (s->current = s->lineList; s->current; s->current = s->current->next) {
+		line = s->current->data;
+		if (!strncmp (keyString, line, len)) {
+			value = g_strdup (line + len);
+			if (!verbatim)
+				svUnescape (value);
+			break;
+		}
 	}
-    }
-    g_free(keyString);
+	g_free (keyString);
 
-    if (value) {
-	if (value[0]) {
-	    return value;
+	if (value && value[0]) {
+		return value;
 	} else {
-	    g_free(value);
-	    return NULL;
+		g_free (value);
+		return NULL;
 	}
-    }
-    if (s->parent) value = svGetValue(s->parent, key, verbatim);
-    return value;
 }
 
-/* return 1 if <key> resolves to any truth value (e.g. "yes", "y", "true")
- * return 0 if <key> resolves to any non-truth value (e.g. "no", "n", "false")
+/* return TRUE if <key> resolves to any truth value (e.g. "yes", "y", "true")
+ * return FALSE if <key> resolves to any non-truth value (e.g. "no", "n", "false")
  * return <default> otherwise
  */
-int
-svTrueValue(shvarFile *s, const char *key, int def)
+gboolean
+svTrueValue (shvarFile *s, const char *key, gboolean def)
 {
-    char *tmp;
-    int returnValue = def;
-
-    tmp = svGetValue(s, key, FALSE);
-    if (!tmp) return returnValue;
-
-    if ( (!strcasecmp("yes", tmp)) ||
-	 (!strcasecmp("true", tmp)) ||
-	 (!strcasecmp("t", tmp)) ||
-	 (!strcasecmp("y", tmp)) ) returnValue = 1;
-    else
-    if ( (!strcasecmp("no", tmp)) ||
-	 (!strcasecmp("false", tmp)) ||
-	 (!strcasecmp("f", tmp)) ||
-	 (!strcasecmp("n", tmp)) ) returnValue = 0;
-
-    g_free (tmp);
-    return returnValue;
+	char *tmp;
+	gboolean returnValue = def;
+
+	tmp = svGetValue (s, key, FALSE);
+	if (!tmp)
+		return returnValue;
+
+	if (   !g_ascii_strcasecmp ("yes", tmp)
+	    || !g_ascii_strcasecmp ("true", tmp)
+	    || !g_ascii_strcasecmp ("t", tmp)
+	    || !g_ascii_strcasecmp ("y", tmp))
+		returnValue = TRUE;
+	else if (   !g_ascii_strcasecmp ("no", tmp)
+	         || !g_ascii_strcasecmp ("false", tmp)
+	         || !g_ascii_strcasecmp ("f", tmp)
+	         || !g_ascii_strcasecmp ("n", tmp))
+		returnValue = FALSE;
+
+	g_free (tmp);
+	return returnValue;
 }
 
 
 /* Set the variable <key> equal to the value <value>.
  * If <key> does not exist, and the <current> pointer is set, append
- * the key=value pair after that line.  Otherwise, prepend the pair
- * to the top of the file.  Here's the algorithm, as the C code
- * seems to be rather dense:
- *
- * if (value == NULL), then:
- *     if val2 (parent): change line to key= or append line key=
- *     if val1 (this)  : delete line
- *     else noop
- * else use this table:
- *                                val2
- *             NULL              value               other
- * v   NULL    append line       noop                append line
- * a
- * l   value   noop              noop                noop
- * 1
- *     other   change line       delete line         change line
- *
- * No changes are ever made to the parent config file, only to the
- * specific file passed on the command line.
- *
+ * the key=value pair after that line.  Otherwise, append the pair
+ * to the bottom of the file.
  */
 void
-svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim)
+svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim)
 {
-    char *newval = NULL, *val1 = NULL, *val2 = NULL;
-    char *keyValue;
-
-    g_assert(s);
-    g_assert(key);
-    /* value may be NULL */
-
-    if (value)
-        newval = verbatim ? g_strdup(value) : svEscape(value);
-    keyValue = g_strdup_printf("%s=%s", key, newval ? newval : "");
-
-    val1 = svGetValue(s, key, FALSE);
-    if (val1 && newval && !strcmp(val1, newval)) goto bail;
-    if (s->parent) val2 = svGetValue(s->parent, key, FALSE);
-
-    if (!newval || !newval[0]) {
-	/* delete value somehow */
-	if (val2) {
-	    /* change/append line to get key= */
-	    if (s->current) s->current->data = keyValue;
-	    else s->lineList = g_list_append(s->lineList, keyValue);
-	    s->modified = 1;
-	    goto end;
-	} else if (val1) {
-	    /* delete line */
-	    s->lineList = g_list_remove_link(s->lineList, s->current);
-	    g_list_free_1(s->current);
-	    s->modified = 1;
+	char *newval = NULL, *oldval = NULL;
+	char *keyValue;
+
+	g_return_if_fail (s != NULL);
+	g_return_if_fail (key != NULL);
+	/* value may be NULL */
+
+	if (value)
+		newval = verbatim ? g_strdup (value) : svEscape (value);
+	keyValue = g_strdup_printf ("%s=%s", key, newval ? newval : "");
+
+	oldval = svGetValue (s, key, FALSE);
+
+	if (!newval || !newval[0]) {
+		/* delete value */
+		if (oldval) {
+			/* delete line */
+			s->lineList = g_list_remove_link (s->lineList, s->current);
+			g_list_free_1 (s->current);
+			s->modified = TRUE;
+		}
+		goto bail; /* do not need keyValue */
+	}
+
+	if (!oldval) {
+		/* append line */
+		s->lineList = g_list_append (s->lineList, keyValue);
+		s->modified = TRUE;
+		goto end;
 	}
-	goto bail; /* do not need keyValue */
-    }
-
-    if (!val1) {
-	if (val2 && !strcmp(val2, newval)) goto end;
-	/* append line */
-	s->lineList = g_list_append(s->lineList, keyValue);
-	s->modified = 1;
+
+	if (strcmp (oldval, newval) != 0) {
+		/* change line */
+		if (s->current)
+			s->current->data = keyValue;
+		else
+			s->lineList = g_list_append (s->lineList, keyValue);
+		s->modified = TRUE;
+	}
+
+ end:
+	g_free (newval);
+	g_free (oldval);
+	return;
+
+ bail:
+	g_free (keyValue);
 	goto end;
-    }
-
-    /* deal with a whole line of noops */
-    if (val1 && !strcmp(val1, newval)) goto end;
-
-    /* At this point, val1 && val1 != value */
-    if (val2 && !strcmp(val2, newval)) {
-	/* delete line */
-	s->lineList = g_list_remove_link(s->lineList, s->current);
-	g_list_free_1(s->current);
-	s->modified = 1;
-	goto bail; /* do not need keyValue */
-    } else {
-	/* change line */
-	if (s->current) s->current->data = keyValue;
-	else s->lineList = g_list_append(s->lineList, keyValue);
-	s->modified = 1;
-    }
-
-end:
-    g_free(newval);
-    g_free(val1);
-    g_free(val2);
-    return;
-
-bail:
-    g_free (keyValue);
-    goto end;
 }
 
-/* Write the current contents iff modified.  Returns -1 on error
- * and 0 on success.  Do not write if no values have been modified.
+/* Write the current contents iff modified.  Returns FALSE on error
+ * and TRUE on success.  Do not write if no values have been modified.
  * The mode argument is only used if creating the file, not if
  * re-writing an existing file, and is passed unchanged to the
  * open() syscall.
  */
-int
-svWriteFile(shvarFile *s, int mode)
+gboolean
+svWriteFile (shvarFile *s, int mode, GError **error)
 {
-    FILE *f;
-    int tmpfd;
-
-    if (s->modified) {
-	if (s->fd == -1)
-	    s->fd = open(s->fileName, O_WRONLY|O_CREAT, mode);
-	if (s->fd == -1)
-	    return -1;
-	if (ftruncate(s->fd, 0) < 0)
-	    return -1;
-
-	tmpfd = dup(s->fd);
-	if (tmpfd == -1)
-		return -1;
-	f = fdopen(tmpfd, "w");
-	fseek(f, 0, SEEK_SET);
-	for (s->current = s->lineList; s->current; s->current = s->current->next) {
-	    char *line = s->current->data;
-	    fprintf(f, "%s\n", line);
+	FILE *f;
+	int tmpfd;
+
+	if (s->modified) {
+		if (s->fd == -1)
+			s->fd = open (s->fileName, O_WRONLY | O_CREAT, mode);
+		if (s->fd == -1) {
+			int errsv = errno;
+
+			g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv),
+			             "Could not open file '%s' for writing: %s",
+			             s->fileName, strerror (errsv));
+			return FALSE;
+		}
+		if (ftruncate (s->fd, 0) < 0) {
+			int errsv = errno;
+
+			g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv),
+			             "Could not overwrite file '%s': %s",
+			             s->fileName, strerror (errsv));
+			return FALSE;
+		}
+
+		tmpfd = dup (s->fd);
+		if (tmpfd == -1) {
+			int errsv = errno;
+
+			g_set_error (error, G_FILE_ERROR, g_file_error_from_errno (errsv),
+			             "Internal error writing file '%s': %s",
+			             s->fileName, strerror (errsv));
+			return FALSE;
+		}
+		f = fdopen (tmpfd, "w");
+		fseek (f, 0, SEEK_SET);
+		for (s->current = s->lineList; s->current; s->current = s->current->next) {
+			char *line = s->current->data;
+			fprintf (f, "%s\n", line);
+		}
+		fclose (f);
 	}
-	fclose(f);
-    }
 
-    return 0;
+	return TRUE;
 }
 
- 
-/* Close the file descriptor (if open) and delete the shvarFile.
- * Returns -1 on error and 0 on success.
- */
-int
-svCloseFile(shvarFile *s)
-{
 
-    g_assert(s);
+/* Close the file descriptor (if open) and free the shvarFile. */
+void
+svCloseFile (shvarFile *s)
+{
+	g_return_if_fail (s != NULL);
 
-    if (s->fd != -1) close(s->fd);
+	if (s->fd != -1)
+		close (s->fd);
 
-    g_free(s->arena);
-    g_free(s->fileName);
-    g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */
-    g_free(s);
-    return 0;
+	g_free (s->fileName);
+	g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */
+	g_slice_free (shvarFile, s);
 }
diff --git a/src/settings/plugins/ifcfg-rh/shvar.h b/src/settings/plugins/ifcfg-rh/shvar.h
index 4b650d2a9a..4cbf1a31a7 100644
--- a/src/settings/plugins/ifcfg-rh/shvar.h
+++ b/src/settings/plugins/ifcfg-rh/shvar.h
@@ -1,3 +1,4 @@
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 /*
  * shvar.h
  *
@@ -32,79 +33,61 @@
 
 #include <glib.h>
 
-#ifdef __cplusplus
-extern "C" {
-#endif /* __cplusplus */
+G_BEGIN_DECLS
 
 typedef struct _shvarFile shvarFile;
 struct _shvarFile {
-	char		*fileName;	/* read-only */
-	int		fd;		/* read-only */
-	char		*arena;		/* ignore */
-	GList		*lineList;	/* read-only */
-	GList		*current;	/* set implicitly or explicitly,
-					   points to element of lineList */
-	shvarFile	*parent;	/* set explicitly */
-	int		modified;	/* ignore */
+	char      *fileName;    /* read-only */
+	int        fd;          /* read-only */
+	GList     *lineList;    /* read-only */
+	GList     *current;     /* set implicitly or explicitly, points to element of lineList */
+	gboolean   modified;    /* ignore */
 };
 
 
-/* Create the file <name>, return shvarFile on success, NULL on failure */
-shvarFile *
-svCreateFile(const char *name);
+/* 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 *
-svNewFile(const char *name);
+shvarFile *svOpenFile (const char *name, GError **error);
 
 /* 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.
  */
-char *
-svGetValue(shvarFile *s, const char *key, gboolean verbatim);
+char *svGetValue (shvarFile *s, const char *key, gboolean verbatim);
 
-/* return 1 if <key> resolves to any truth value (e.g. "yes", "y", "true")
- * return 0 if <key> resolves to any non-truth value (e.g. "no", "n", "false")
+/* return TRUE if <key> resolves to any truth value (e.g. "yes", "y", "true")
+ * return FALSE if <key> resolves to any non-truth value (e.g. "no", "n", "false")
  * return <def> otherwise
  */
-int
-svTrueValue(shvarFile *s, const char *key, int def);
+gboolean svTrueValue (shvarFile *s, const char *key, gboolean def);
 
 /* Set the variable <key> equal to the value <value>.
  * If <key> does not exist, and the <current> pointer is set, append
  * the key=value pair after that line.  Otherwise, prepend the pair
  * to the top of the file.
  */
-void
-svSetValue(shvarFile *s, const char *key, const char *value, gboolean verbatim);
+void svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim);
 
 
-/* Write the current contents iff modified.  Returns -1 on error
- * and 0 on success.  Do not write if no values have been modified.
+/* Write the current contents iff modified.  Returns FALSE on error
+ * and TRUE on success.  Do not write if no values have been modified.
  * The mode argument is only used if creating the file, not if
  * re-writing an existing file, and is passed unchanged to the
  * open() syscall.
  */
-int
-svWriteFile(shvarFile *s, int mode);
+gboolean svWriteFile (shvarFile *s, int mode, GError **error);
 
-/* Close the file descriptor (if open) and delete the shvarFile.
- * Returns -1 on error and 0 on success.
- */
-int
-svCloseFile(shvarFile *s);
+/* Close the file descriptor (if open) and free the shvarFile. */
+void svCloseFile (shvarFile *s);
 
 /* Return a new escaped string */
-char *
-svEscape(const char *s);
+char *svEscape (const char *s);
 
 /* Unescape a string in-place */
-void
-svUnescape(char *s);
+void svUnescape (char *s);
 
-#ifdef __cplusplus
-}
-#endif /* __cplusplus */
+G_END_DECLS
 
 #endif /* ! _SHVAR_H */
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 f45d788ece..082d6fd585 100644
--- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
+++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c
@@ -6106,12 +6106,13 @@ test_write_wifi_hidden (void)
 	                                 TEST_SCRATCH_DIR "/network-scripts/",
 	                                 &testfile,
 	                                 &error);
-	f = svNewFile (testfile);
-	g_assert (f);
-
 	g_assert_no_error (error);
 	g_assert (success);
 
+	f = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
+	g_assert (f);
+
 	/* re-read the file to check that what key was written. */
 	val = svGetValue (f, "SSID_HIDDEN", FALSE);
 	g_assert (val);
@@ -7337,11 +7338,16 @@ test_write_wired_static_ip6_only_gw (gconstpointer user_data)
 	                               NULL, NULL,
 	                               &error,
 	                               &ignore_error);
+	g_assert_no_error (error);
+	g_assert (reread);
+	g_assert (nm_connection_verify (reread, &error));
+	g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT));
 
 	{
 		/* re-read the file to check that what key was written. */
-		shvarFile *ifcfg = svNewFile (testfile);
+		shvarFile *ifcfg = svOpenFile (testfile, &error);
 
+		g_assert_no_error (error);
 		g_assert (ifcfg);
 		written_ifcfg_gateway = svGetValue (ifcfg, "IPV6_DEFAULTGW", FALSE);
 		svCloseFile (ifcfg);
@@ -7349,11 +7355,6 @@ test_write_wired_static_ip6_only_gw (gconstpointer user_data)
 
 	unlink (testfile);
 
-	g_assert_no_error (error);
-	g_assert (reread);
-	g_assert (nm_connection_verify (reread, &error));
-	g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT));
-
 	/* access the gateway from the loaded connection. */
 	s_ip6 = nm_connection_get_setting_ip6_config (reread);
 	g_assert (s_ip6 && nm_setting_ip6_config_get_num_addresses (s_ip6)==1);
@@ -8380,11 +8381,12 @@ test_write_wifi_open (void)
 	                               &route6file,
 	                               &error,
 	                               &ignore_error);
+	g_assert_no_error (error);
 
 	/* Now make sure that the ESSID item isn't double-quoted (rh #606518) */
-	ifcfg = svNewFile (testfile);
-	ASSERT (ifcfg != NULL,
-	        "wifi-open-write-reread", "failed to load %s as shvarfile", testfile);
+	ifcfg = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
+	g_assert (ifcfg != NULL);
 
 	tmp = svGetValue (ifcfg, "ESSID", TRUE);
 	ASSERT (tmp != NULL,
@@ -10869,7 +10871,8 @@ test_write_wifi_dynamic_wep_leap (void)
 	 * did not get written.  Check first that the auth alg is not set to "LEAP"
 	 * and next that the only IEEE 802.1x EAP method is "LEAP".
 	 */
-	ifcfg = svNewFile (testfile);
+	ifcfg = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
 	g_assert (ifcfg);
 	tmp = svGetValue (ifcfg, "SECURITYMODE", FALSE);
 	g_assert_cmpstr (tmp, ==, NULL);
@@ -11487,7 +11490,8 @@ test_write_wired_ctc_dhcp (void)
 	g_assert (testfile != NULL);
 
 	/* Ensure the CTCPROT item gets written out as it's own option */
-	ifcfg = svNewFile (testfile);
+	ifcfg = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
 	g_assert (ifcfg);
 
 	tmp = svGetValue (ifcfg, "CTCPROT", TRUE);
@@ -13864,9 +13868,10 @@ test_write_fcoe_mode (gconstpointer user_data)
 	g_assert (testfile);
 
 	{
-		shvarFile *ifcfg = svNewFile (testfile);
+		shvarFile *ifcfg = svOpenFile (testfile, &error);
 		char *written_mode;
 
+		g_assert_no_error (error);
 		g_assert (ifcfg);
 		written_mode = svGetValue (ifcfg, "DCB_APP_FCOE_MODE", FALSE);
 		svCloseFile (ifcfg);
@@ -13975,12 +13980,13 @@ test_write_team_master (void)
 	                                 TEST_SCRATCH_DIR "/network-scripts/",
 	                                 &testfile,
 	                                 &error);
-	f = svNewFile (testfile);
-	g_assert (f);
-
 	g_assert_no_error (error);
 	g_assert (success);
 
+	f = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
+	g_assert (f);
+
 	/* re-read the file to check that what key was written. */
 	val = svGetValue (f, "DEVICETYPE", FALSE);
 	g_assert (val);
@@ -14092,12 +14098,13 @@ test_write_team_port (void)
 	                                 TEST_SCRATCH_DIR "/network-scripts/",
 	                                 &testfile,
 	                                 &error);
-	f = svNewFile (testfile);
-	g_assert (f);
-
 	g_assert_no_error (error);
 	g_assert (success);
 
+	f = svOpenFile (testfile, &error);
+	g_assert_no_error (error);
+	g_assert (f);
+
 	/* re-read the file to check that what key was written. */
 	val = svGetValue (f, "TYPE", FALSE);
 	g_assert (!val);
diff --git a/src/settings/plugins/ifcfg-rh/utils.c b/src/settings/plugins/ifcfg-rh/utils.c
index 5abb4282e5..11b8a52e8d 100644
--- a/src/settings/plugins/ifcfg-rh/utils.c
+++ b/src/settings/plugins/ifcfg-rh/utils.c
@@ -296,7 +296,7 @@ utils_get_extra_ifcfg (const char *parent, const char *tag, gboolean should_crea
 		ifcfg = svCreateFile (path);
 
 	if (!ifcfg)
-		ifcfg = svNewFile (path);
+		ifcfg = svOpenFile (path, NULL);
 
 	g_free (path);
 	return ifcfg;
diff --git a/src/settings/plugins/ifcfg-rh/writer.c b/src/settings/plugins/ifcfg-rh/writer.c
index 5e962c57cd..1f0e6a9dda 100644
--- a/src/settings/plugins/ifcfg-rh/writer.c
+++ b/src/settings/plugins/ifcfg-rh/writer.c
@@ -97,6 +97,7 @@ set_secret (shvarFile *ifcfg,
             gboolean verbatim)
 {
 	shvarFile *keyfile;
+	GError *error = NULL;
 	
 	/* Clear the secret from the ifcfg and the associated "keys" file */
 	svSetValue (ifcfg, key, NULL, FALSE);
@@ -118,9 +119,9 @@ set_secret (shvarFile *ifcfg,
 	if (flags == NM_SETTING_SECRET_FLAG_NONE)
 		svSetValue (keyfile, key, value, verbatim);
 
-	if (svWriteFile (keyfile, 0600)) {
-		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: could not update key file '%s'",
-		             keyfile->fileName);
+	if (!svWriteFile (keyfile, 0600, &error)) {
+		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: %s", error->message);
+		g_clear_error (&error);
 		svCloseFile (keyfile);
 		goto error;
 	}
@@ -2118,9 +2119,7 @@ write_ip4_setting (NMConnection *connection, shvarFile *ifcfg, GError **error)
 			g_free (gw_key);
 			g_free (metric_key);
 		}
-		if (svWriteFile (routefile, 0644)) {
-			g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-			             "Could not update route file '%s'", routefile->fileName);
+		if (!svWriteFile (routefile, 0644, error)) {
 			svCloseFile (routefile);
 			goto out;
 		}
@@ -2529,7 +2528,10 @@ write_connection (NMConnection *connection,
 
 	if (filename) {
 		/* For existing connections, 'filename' should be full path to ifcfg file */
-		ifcfg = svNewFile (filename);
+		ifcfg = svOpenFile (filename, error);
+		if (!ifcfg)
+			return FALSE;
+
 		ifcfg_name = g_strdup (filename);
 	} else {
 		char *escaped;
@@ -2565,12 +2567,6 @@ write_connection (NMConnection *connection,
 		ifcfg = svCreateFile (ifcfg_name);
 	}
 
-	if (!ifcfg) {
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-		             "Failed to open/create ifcfg file '%s'", ifcfg_name);
-		goto out;
-	}
-
 	type = nm_setting_connection_get_connection_type (s_con);
 	if (!type) {
 		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
@@ -2641,11 +2637,8 @@ write_connection (NMConnection *connection,
 
 	write_connection_setting (s_con, ifcfg);
 
-	if (svWriteFile (ifcfg, 0644)) {
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-		             "Can't write connection '%s'", ifcfg->fileName);
+	if (!svWriteFile (ifcfg, 0644, error))
 		goto out;
-	}
 
 	/* Only return the filename if this was a newly written ifcfg */
 	if (out_filename && !filename)