Re: [PATCH] keyfile plugin patches

2013-03-07 Thread Dan Williams
On Wed, 2013-03-06 at 18:01 -0500, Colin Walters wrote:
 Hopefully the first patch for error handling is something we can do in
 the rest of the codebase too, because it's s much better.

Pushed, thanks.

Dan

___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list


[PATCH] keyfile plugin patches

2013-03-06 Thread Colin Walters
Hopefully the first patch for error handling is something we can do in
the rest of the codebase too, because it's s much better.


From de1e3e809d06529afc0eb95ba38b374e1487924b Mon Sep 17 00:00:00 2001
From: Colin Walters walt...@verbum.org
Date: Wed, 6 Mar 2013 14:00:41 -0500
Subject: [PATCH 1/2] keyfile: Use goto out style error handling

Just code cleanup: This is much less error-prone than manual nesting,
and will mesh very well with future changes to use the libgsystem
cleanup macros.
---
 src/settings/plugins/keyfile/plugin.c |  146 ++---
 1 files changed, 81 insertions(+), 65 deletions(-)

diff --git a/src/settings/plugins/keyfile/plugin.c b/src/settings/plugins/keyfile/plugin.c
index e7e350b..a50f8c5 100644
--- a/src/settings/plugins/keyfile/plugin.c
+++ b/src/settings/plugins/keyfile/plugin.c
@@ -412,48 +412,53 @@ get_unmanaged_specs (NMSystemConfigInterface *config)
 	GKeyFile *key_file;
 	GSList *specs = NULL;
 	GError *error = NULL;
+	char *str;
 
 	if (!priv-conf_file)
 		return NULL;
 
 	key_file = g_key_file_new ();
-	if (g_key_file_load_from_file (key_file, priv-conf_file, G_KEY_FILE_NONE, error)) {
-		char *str;
-
-		str = g_key_file_get_value (key_file, keyfile, unmanaged-devices, NULL);
-		if (str) {
-			char **udis;
-			int i;
-
-			udis = g_strsplit (str, ;, -1);
-			g_free (str);
-
-			for (i = 0; udis[i] != NULL; i++) {
-/* Verify unmanaged specification and add it to the list */
-if (strlen (udis[i])  4  !strncmp (udis[i], mac:, 4)  ether_aton (udis[i] + 4)) {
-	char *p = udis[i];
-
-	/* To accept uppercase MACs in configuration file, we have to convert values to lowercase here.
-	 * Unmanaged MACs in specs are always in lowercase. */
-	while (*p) {
-*p = g_ascii_tolower (*p);
-p++;
-}
-	specs = g_slist_append (specs, udis[i]);
-} else {
-	g_warning (Error in file '%s': invalid unmanaged-devices entry: '%s', priv-conf_file, udis[i]);
-	g_free (udis[i]);
+	if (!g_key_file_load_from_file (key_file, priv-conf_file, G_KEY_FILE_NONE, error)) {
+		g_prefix_error (error, Error parsing file '%s': , priv-conf_file);
+		goto out;
+	}
+	
+	str = g_key_file_get_value (key_file, keyfile, unmanaged-devices, NULL);
+	if (str) {
+		char **udis;
+		int i;
+
+		udis = g_strsplit (str, ;, -1);
+		g_free (str);
+
+		for (i = 0; udis[i] != NULL; i++) {
+			/* Verify unmanaged specification and add it to the list */
+			if (strlen (udis[i])  4  !strncmp (udis[i], mac:, 4)  ether_aton (udis[i] + 4)) {
+char *p = udis[i];
+
+/* To accept uppercase MACs in configuration file, we have to convert values to lowercase here.
+ * Unmanaged MACs in specs are always in lowercase. */
+while (*p) {
+	*p = g_ascii_tolower (*p);
+	p++;
 }
+specs = g_slist_append (specs, udis[i]);
+			} else {
+g_warning (Error in file '%s': invalid unmanaged-devices entry: '%s', priv-conf_file, udis[i]);
+g_free (udis[i]);
 			}
-
-			g_free (udis); /* Yes, g_free, not g_strfreev because we need the strings in the list */
 		}
-	} else {
-		g_warning (Error parsing file '%s': %s, priv-conf_file, error-message);
-		g_error_free (error);
+
+		g_free (udis); /* Yes, g_free, not g_strfreev because we need the strings in the list */
 	}
 
-	g_key_file_free (key_file);
+ out:
+	if (error) {
+		g_warning (%s, error-message);
+		g_error_free (error);
+	}
+	if (key_file)
+		g_key_file_free (key_file);
 
 	return specs;
 }
@@ -470,14 +475,20 @@ plugin_get_hostname (SCPluginKeyfile *plugin)
 		return NULL;
 
 	key_file = g_key_file_new ();
-	if (g_key_file_load_from_file (key_file, priv-conf_file, G_KEY_FILE_NONE, error))
-		hostname = g_key_file_get_value (key_file, keyfile, hostname, NULL);
-	else {
-		g_warning (Error parsing file '%s': %s, priv-conf_file, error-message);
-		g_error_free (error);
+	if (!g_key_file_load_from_file (key_file, priv-conf_file, G_KEY_FILE_NONE, error)) {
+		g_prefix_error (error, Error parsing file '%s': , priv-conf_file);
+		goto out;
 	}
 
-	g_key_file_free (key_file);
+	hostname = g_key_file_get_value (key_file, keyfile, hostname, NULL);
+
+ out:
+	if (error) {
+		g_warning (%s, error-message);
+		g_error_free (error);
+	}
+	if (key_file)
+		g_key_file_free (key_file);
 
 	return hostname;
 }
@@ -485,45 +496,50 @@ plugin_get_hostname (SCPluginKeyfile *plugin)
 static gboolean
 plugin_set_hostname (SCPluginKeyfile *plugin, const char *hostname)
 {
+	gboolean ret = FALSE;
 	SCPluginKeyfilePrivate *priv = SC_PLUGIN_KEYFILE_GET_PRIVATE (plugin);
-	GKeyFile *key_file;
+	GKeyFile *key_file = NULL;
 	GError *error = NULL;
-	gboolean result = FALSE;
+	char *data = NULL;
+	gsize len;
 
 	if (!priv-conf_file) {
-		g_warning (Error saving hostname: no config file);
-		return FALSE;
+		g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+	 Error saving hostname: no config file);
+		goto out;
 	}
+	
+	g_free (priv-hostname);
+	priv-hostname = g_strdup