Re: [PATCH] core: improve keyfile error handling

[Dan Williams <dcbw redhat com>]

On Sat, 2011-10-01 at 00:22 +0200, Thomas Bechtold wrote:
> * better error messages
>   * fix memory leak in parse_state_file ()
>   * create intermediate parent directories as needed for state file

Pushed, thanks.

Dan

> ---
>  src/main.c      |   30 +++++++++++++-----------------
>  src/nm-config.c |   13 ++++++++++---
>  2 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index 59d7cd1..682cfcd 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -327,8 +327,8 @@ parse_state_file (const char *filename,
>  
>  	state_file = g_key_file_new ();
>  	if (!state_file) {
> -		g_set_error (error, 0, 0,
> -		             "Not enough memory to load state file.");
> +		g_set_error (error, NM_CONFIG_ERROR, NM_CONFIG_ERROR_NO_MEMORY,
> +		             "Not enough memory to load state file %s.", filename);
>  		return FALSE;
>  	}
>  
> @@ -348,11 +348,12 @@ parse_state_file (const char *filename,
>  			/* try to create the directory if it doesn't exist */
>  			dirname = g_path_get_dirname (filename);
>  			errno = 0;
> -			if (mkdir (dirname, 0755) != 0) {
> +			if (g_mkdir_with_parents (dirname, 0755) != 0) {
>  				if (errno != EEXIST) {
>  					g_set_error (error, G_FILE_ERROR, G_FILE_ERROR_ACCES,
> -					             "Error creating state directory %s: %d", dirname, errno);
> +					             "Error creating state directory %s: %s", dirname, strerror(errno));
>  					g_free (dirname);
> +					g_clear_error (&tmp_error);
>  					return FALSE;
>  				}
>  			}
> @@ -368,13 +369,15 @@ parse_state_file (const char *filename,
>  			if (data)
>  				ret = g_file_set_contents (filename, data, len, error);
>  			g_free (data);
> +			g_clear_error (&tmp_error);
>  
>  			return ret;
>  		} else {
> -			g_set_error_literal (error, tmp_error->domain, tmp_error->code, tmp_error->message);
> -			g_clear_error (&tmp_error);
> +			/* the error is not "No such file or directory" - propagate the error */
> +			g_propagate_error (error, tmp_error);
>  		}
>  
> +		g_clear_error (&tmp_error);
>  		/* Otherwise, file probably corrupt or inaccessible */
>  		return FALSE;
>  	}
> @@ -384,34 +387,27 @@ parse_state_file (const char *filename,
>  	 */
>  	net = g_key_file_get_boolean (state_file, "main", "NetworkingEnabled", &tmp_error);
>  	if (tmp_error)
> -		g_set_error_literal (error, tmp_error->domain, tmp_error->code, tmp_error->message);
> +		g_clear_error (&tmp_error);
>  	else
>  		*net_enabled = net;
> -	g_clear_error (&tmp_error);
>  
>  	wifi = g_key_file_get_boolean (state_file, "main", "WirelessEnabled", &tmp_error);
>  	if (tmp_error) {
> -		g_clear_error (error);
> -		g_set_error_literal (error, tmp_error->domain, tmp_error->code, tmp_error->message);
> +		g_clear_error (&tmp_error);
>  	} else
>  		*wifi_enabled = wifi;
> -	g_clear_error (&tmp_error);
>  
>  	wwan = g_key_file_get_boolean (state_file, "main", "WWANEnabled", &tmp_error);
>  	if (tmp_error) {
> -		g_clear_error (error);
> -		g_set_error_literal (error, tmp_error->domain, tmp_error->code, tmp_error->message);
> +		g_clear_error (&tmp_error);
>  	} else
>  		*wwan_enabled = wwan;
> -	g_clear_error (&tmp_error);
>  
>  	wimax = g_key_file_get_boolean (state_file, "main", "WimaxEnabled", &tmp_error);
>  	if (tmp_error) {
> -		g_clear_error (error);
> -		g_set_error_literal (error, tmp_error->domain, tmp_error->code, tmp_error->message);
> +		g_clear_error (&tmp_error);
>  	} else
>  		*wimax_enabled = wimax;
> -	g_clear_error (&tmp_error);
>  
>  	g_key_file_free (state_file);
>  
> diff --git a/src/nm-config.c b/src/nm-config.c
> index f52f06f..7e582f4 100644
> --- a/src/nm-config.c
> +++ b/src/nm-config.c
> @@ -130,14 +130,14 @@ fill_from_file (NMConfig *config,
>  	gboolean success = FALSE;
>  
>  	if (g_file_test (path, G_FILE_TEST_EXISTS) == FALSE) {
> -		g_set_error (error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_NOT_FOUND, "file not found");
> +		g_set_error (error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_NOT_FOUND, "file %s not found", path);
>  		return FALSE;
>  	}
>  
>  	kf = g_key_file_new ();
>  	if (!kf) {
>  		g_set_error (error, NM_CONFIG_ERROR, NM_CONFIG_ERROR_NO_MEMORY,
> -		             "Not enough memory to load config file");
> +		             "Not enough memory to load config file %s", path);
>  		return FALSE;
>  	}
>  
> @@ -219,9 +219,16 @@ nm_config_new (const char *cli_config_path,
>  		         NM_DEFAULT_SYSTEM_CONF_FILE,
>  		         local ? local->code : -1,
>  		         (local && local->message) ? local->message : "unknown");
> +		g_propagate_error (error, local);
> +		nm_config_free (config);
> +		return NULL;
> +	}
> +	else
> +	{
> +		/* ignore error if config file not found */
> +		g_clear_error (&local);
>  	}
>  
> -	g_propagate_error (error, local);
>  	return config;
>  }
>