[PATCH v3] Add inotify monitoring .config file.

Marcel Holtmann marcel at holtmann.org
Mon Jan 3 17:21:31 PST 2011


Hi Mohamed,

> Reflect new and modify *.config to connman config list. with
> patch any modified or added .config file will be read by connman
> and add these configuration for new provisioning.
> ---
>  src/config.c |  174 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 170 insertions(+), 4 deletions(-)
> 
> diff --git a/src/config.c b/src/config.c
> index d203935..980182f 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -24,8 +24,10 @@
>  #endif
>  
>  #include <stdio.h>
> +#include <unistd.h>
>  #include <string.h>
>  #include <sys/vfs.h>
> +#include <sys/inotify.h>
>  #include <glib.h>
>  
>  #include "connman.h"
> @@ -56,6 +58,11 @@ struct connman_config {
>  
>  static GHashTable *config_table = NULL;
>  
> +static int inotify_wd = -1;
> +
> +static GIOChannel *inotify_channel = NULL;
> +static uint channel_watch = 0;

Just call it inotify_watch here. Makes it a bit more clear.

>  /* Definition of possible strings in the .config files */
>  #define CONFIG_KEY_NAME                "Name"
>  #define CONFIG_KEY_DESC                "Description"
> @@ -362,8 +369,6 @@ static int create_config(const char *ident)
>  
>  	connman_info("Adding configuration %s", config->ident);
>  
> -	load_config(config);
> -
>  	return 0;
>  }
>  
> @@ -395,8 +400,14 @@ static int read_configs(void)
>  			ident = g_string_free(str, FALSE);
>  
>  			if (connman_dbus_validate_ident(ident) == TRUE)
> -				create_config(ident);
> -
> +				if (create_config(ident) == 0) {
> +					struct connman_config *config;
> +
> +					config = g_hash_table_lookup(
> +							config_table, ident);
> +					if (config != NULL)
> +						load_config(config);
> +				}

As a general rule you should put a { } around the if statement. I know
that the compiler gets this right, but for humans it is harder to read.

Also why not modify create_config in a way that it return the struct
connman_config. That would avoid the extra lookup.

>  			g_free(ident);
>  		}
>  
> @@ -406,6 +417,157 @@ static int read_configs(void)
>  	return 0;
>  }
>  
> +static gboolean inotify_data(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	char buffer[4096];

This buffer is a bit of overhead. sizeof(inotify_event) + MAX_PATHNAME
should be enough. So something around 256 bytes is plenty.

> +	char *next_event;
> +	gsize bytes_read;
> +	GIOError err;
> +
> +	if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> +		channel_watch = 0;
> +		return FALSE;
> +	}
> +
> +	err = g_io_channel_read(channel, (gchar *) buffer,

This gchar cast is not needed since your buffer is already char.

> +					sizeof(buffer) - 1, &bytes_read);
> +
> +	if (err != G_IO_ERROR_NONE) {
> +		if (err == G_IO_ERROR_AGAIN)
> +			return TRUE;
> +
> +		connman_error("Reading from inotify channel failed");
> +		channel_watch = 0;
> +		return FALSE;
> +	}
> +
> +	next_event = buffer;
> +
> +	while (bytes_read > 0) {
> +		struct inotify_event *event;
> +		gchar *file;
> +		GString *str;
> +		gchar *ident;
> +		gsize len;
> +
> +		event = (struct inotify_event *) next_event;
> +		if (event->len)
> +			file = next_event + sizeof(struct inotify_event);
> +		else
> +			file = NULL;
> +
> +		len = sizeof(struct inotify_event) + event->len;
> +
> +		/* check if inotify_event block fit */
> +		if (len > bytes_read)
> +			break;
> +
> +		next_event += len;
> +		bytes_read -= len;
> +
> +		if (file == NULL)
> +			continue;
> +
> +		if (g_str_has_suffix(file, ".config") == FALSE)
> +			continue;
> +
> +		ident = g_strrstr(file, ".config");
> +		if (ident == NULL)
> +			continue;
> +
> +		str = g_string_new_len(file, ident - file);
> +		if (str == NULL)
> +			continue;
> +
> +		ident = g_string_free(str, FALSE);

This is a bit pointless here. You are using a GString with no real need
for a GString. You do own the event buffer here and could just put a
'\0' at the position of the '.'. And then use that string directly.

> +		if (connman_dbus_validate_ident(ident) == FALSE) {
> +			g_free(ident);
> +			continue;
> +		}
> +
> +		if (event->mask & IN_CREATE)
> +			create_config(ident);
> +
> +		if (event->mask & IN_MODIFY) {
> +			struct connman_config *config;
> +
> +			config = g_hash_table_lookup(config_table, ident);
> +			if (config != NULL) {
> +				g_hash_table_remove_all(config->service_table);
> +				load_config(config);
> +			}
> +		}
> +
> +		if (event->mask & IN_DELETE)
> +			g_hash_table_remove(config_table, ident);
> +
> +		g_free(ident);
> +	}
> +
> +	return TRUE;
> +}

I might have to go through it once more, but the rest seems fine to me.

Regards

Marcel





More information about the connman mailing list