ifcfg-rh: return proper error messages from svOpenFile() and svWriteFile()

src/settings/plugins/ifcfg-rh/plugin.c

@@ -646,7 +646,7 @@ plugin_get_hostname (SCPluginIfcfg *plugin)
 		return hostname;
 	}
 
-	network = svOpenFile (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 = svOpenFile (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);
 	}
 

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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (filename);
+	ifcfg = svOpenFile (filename, NULL);
 	if (!ifcfg)
 		return NULL;
 
@@ -5077,12 +5077,9 @@ connection_from_file (const char *filename,
 		return NULL;
 	}
 
-	parsed = svOpenFile (filename);
+	parsed = svOpenFile (filename, error);
-	if (!parsed) {
+	if (!parsed)
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-		             "Couldn't parse file '%s'", filename);
 		return NULL;
-	}
 
 	if (!svTrueValue (parsed, "NM_CONTROLLED", TRUE)) {
 		g_assert (out_unhandled != NULL);

src/settings/plugins/ifcfg-rh/shvar.c

@@ -39,10 +39,11 @@
  * (actually, return a structure anyway) if it doesn't exist.
  */
 static shvarFile *
-svOpenFileInternal (const char *name, gboolean create)
+svOpenFileInternal (const char *name, gboolean create, GError **error)
 {
 	shvarFile *s = NULL;
 	gboolean closefd = FALSE;
+	int errsv;
 
 	s = g_slice_new0 (shvarFile);
 
@@ -53,7 +54,9 @@ svOpenFileInternal (const char *name, gboolean create)
 	if (!create || s->fd == -1) {
 		/* try read-only */
 		s->fd = open (name, O_RDONLY); /* NOT O_CREAT */
-		if (s->fd != -1)
+		if (s->fd == -1)
+			errsv = errno;
+		else
 			closefd = TRUE;
 	}
 	s->fileName = g_strdup (name);
@@ -63,8 +66,10 @@ svOpenFileInternal (const char *name, gboolean create)
 		char *arena, *p, *q;
 		ssize_t nread, total = 0;
 
-		if (fstat (s->fd, &buf) < 0)
+		if (fstat (s->fd, &buf) < 0) {
+			errsv = errno;
 			goto bail;
+		}
 		arena = g_malloc (buf.st_size + 1);
 		arena[buf.st_size] = '\0';
 
@@ -72,8 +77,10 @@ svOpenFileInternal (const char *name, gboolean create)
 			nread = read (s->fd, arena + total, buf.st_size - total);
 			if (nread == -1 && errno == EINTR)
 				continue;
-			if (nread <= 0)
+			if (nread <= 0) {
+				errsv = errno;
 				goto bail;
+			}
 			total += nread;
 		}
 
@@ -101,14 +108,18 @@ svOpenFileInternal (const char *name, gboolean create)
 		close (s->fd);
 	g_free (s->fileName);
 	g_slice_free (shvarFile, s);
+
+	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 *
-svOpenFile (const char *name)
+svOpenFile (const char *name, GError **error)
 {
-	return svOpenFileInternal (name, FALSE);
+	return svOpenFileInternal (name, FALSE, error);
 }
 
 /* Create a new file structure, returning actual data if the file exists,
@@ -117,7 +128,7 @@ svOpenFile (const char *name)
 shvarFile *
 svCreateFile (const char *name)
 {
-	return svOpenFileInternal (name, TRUE);
+	return svOpenFileInternal (name, TRUE, NULL);
 }
 
 /* remove escaped characters in place */
@@ -360,7 +371,7 @@ svSetValue (shvarFile *s, const char *key, const char *value, gboolean verbatim)
  * open() syscall.
  */
 gboolean
-svWriteFile (shvarFile *s, int mode)
+svWriteFile (shvarFile *s, int mode, GError **error)
 {
 	FILE *f;
 	int tmpfd;
@@ -368,14 +379,32 @@ svWriteFile (shvarFile *s, int mode)
 	if (s->modified) {
 		if (s->fd == -1)
 			s->fd = open (s->fileName, O_WRONLY | O_CREAT, mode);
-		if (s->fd == -1)
+		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)
+		}
+		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)
+		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) {
@@ -390,10 +419,10 @@ svWriteFile (shvarFile *s, int mode)
 
 
 /* Close the file descriptor (if open) and free the shvarFile. */
-gboolean
+void
 svCloseFile (shvarFile *s)
 {
-	g_return_val_if_fail (s != NULL, FALSE);
+	g_return_if_fail (s != NULL);
 
 	if (s->fd != -1)
 		close (s->fd);
@@ -401,6 +430,4 @@ svCloseFile (shvarFile *s)
 	g_free (s->fileName);
 	g_list_free_full (s->lineList, g_free); /* implicitly frees s->current */
 	g_slice_free (shvarFile, s);
-
-	return TRUE;
 }

src/settings/plugins/ifcfg-rh/shvar.h

@@ -49,7 +49,7 @@ struct _shvarFile {
 shvarFile *svCreateFile (const char *name);
 
 /* Open the file <name>, return shvarFile on success, NULL on failure */
-shvarFile *svOpenFile (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
@@ -77,12 +77,10 @@ void svSetValue (shvarFile *s, const char *key, const char *value, gboolean verb
  * re-writing an existing file, and is passed unchanged to the
  * open() syscall.
  */
-gboolean svWriteFile (shvarFile *s, int mode);
+gboolean svWriteFile (shvarFile *s, int mode, GError **error);
 
-/* Close the file descriptor (if open) and free the shvarFile.
+/* Close the file descriptor (if open) and free the shvarFile. */
- * Returns FALSE on error and TRUE on success.
+void svCloseFile (shvarFile *s);
- */
-gboolean svCloseFile (shvarFile *s);
 
 /* Return a new escaped string */
 char *svEscape (const char *s);

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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (testfile);
+	ifcfg = svOpenFile (testfile, &error);
-	ASSERT (ifcfg != NULL,
+	g_assert_no_error (error);
-	        "wifi-open-write-reread", "failed to load %s as shvarfile", testfile);
+	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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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 = svOpenFile (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);

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 = svOpenFile (path);
+		ifcfg = svOpenFile (path, NULL);
 
 	g_free (path);
 	return ifcfg;

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)) {
+	if (!svWriteFile (keyfile, 0600, &error)) {
-		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: could not update key file '%s'",
+		PLUGIN_WARN (IFCFG_PLUGIN_NAME, "    warning: %s", error->message);
-		             keyfile->fileName);
+		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)) {
+		if (!svWriteFile (routefile, 0644, error)) {
-			g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-			             "Could not update route file '%s'", routefile->fileName);
 			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 = svOpenFile (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)) {
+	if (!svWriteFile (ifcfg, 0644, error))
-		g_set_error (error, IFCFG_PLUGIN_ERROR, 0,
-		             "Can't write connection '%s'", ifcfg->fileName);
 		goto out;
-	}
 
 	/* Only return the filename if this was a newly written ifcfg */
 	if (out_filename && !filename)