[PATCH] Add inotify monitoring .config file.

Marcel Holtmann marcel at holtmann.org
Mon Jan 3 14:51:56 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.
> 
> P.S
> I will submit another patch to refelect new provisioning on current
> avialable network.
> ---

just a quick hint. Please place notes after the --- mark. Otherwise they
end up in the commit message.

Everything between --- and the diff keyword will be removed. These are
just extra information for the reviewer (like changelog etc.).

>  src/config.c |  157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 154 insertions(+), 3 deletions(-)
> 
> diff --git a/src/config.c b/src/config.c
> index d203935..59f5fe3 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -24,8 +24,13 @@
>  #endif
>  
>  #include <stdio.h>
> +#include <unistd.h>
> +#include <limits.h>
>  #include <string.h>
> +#include <sys/ioctl.h>

What do you need an ioctl for?

>  #include <sys/vfs.h>
> +#include <sys/types.h>
> +#include <sys/inotify.h>
>  #include <glib.h>
>  
>  #include "connman.h"
> @@ -56,6 +61,10 @@ struct connman_config {
>  
>  static GHashTable *config_table = NULL;
>  
> +static int inotify_watch = 0;
> +static GIOChannel *inotify_channel = NULL;
> +static uint channel_watch = 0;

So 0 is actually a valid file descriptor. So that would need to be -1.

I am also not a big fan of mixing inotify watch and GLib watch
definitions here. Maybe using _wd is a bit better for the inotify watch
descriptor.

>  /* Definition of possible strings in the .config files */
>  #define CONFIG_KEY_NAME                "Name"
>  #define CONFIG_KEY_DESC                "Description"
> @@ -340,7 +349,7 @@ static int load_config(struct connman_config *config)
>  	return 0;
>  }
>  
> -static int create_config(const char *ident)
> +static int create_config(const char *ident, connman_bool_t load)
>  {
>  	struct connman_config *config;
>  
> @@ -362,7 +371,8 @@ static int create_config(const char *ident)
>  
>  	connman_info("Adding configuration %s", config->ident);
>  
> -	load_config(config);
> +	if (load == TRUE)
> +		load_config(config);

I think doing this in read_configs() is a bit cleaner.

	if (connman_dbus_validate_ident(ident) == TRUE) {
		if (create_config(ident) == 0)
			load_config(config);
	}
 
> +static gboolean inotify_data(GIOChannel *channel, GIOCondition cond,
> +							gpointer user_data)
> +{
> +	char buffer[4096];
> +	gsize i, ret;
> +	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,
> +					sizeof(buffer) - 1, &ret);

Call it bytes_read or count instead of ret.

> +	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;
> +	}
> +
> +	if (ret <= 0)
> +		return TRUE;

This check is rather pointless here.

> +	i = 0;
> +
> +	while (i < ret) {

Doing while (count > 0) seems a bit more easier. And then you just
decrement count and increment current ptr.

> +		struct inotify_event *event;
> +		const gchar *file = NULL;
> +		GString *str;
> +		gchar *ident;
> +
> +		event = (struct inotify_event *) &buffer[i];
> +		if (event->len)
> +			file = &buffer[i] + sizeof(struct inotify_event);
> +
> +		i += sizeof(struct inotify_event) + event->len;

You do need to check that the inotify_event block fits. Otherwise you
potentially read from invalid memory.

> +		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);
> +
> +		if (connman_dbus_validate_ident(ident) == FALSE)
> +			continue;
> +
> +		if (event->mask & IN_CREATE)
> +			create_config(ident, FALSE);
> +
> +		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);
> +
> +	}
> +
> +	return TRUE;
> +}
> +
> +static int create_watch(void)
> +{
> +	int fd;
> +
> +	fd = inotify_init();
> +

Here we don't do extra empty lines. The fd is compared as a direct
result from the the previous function call.

> +	if (fd < 0)
> +		return -EIO;
> +
> +	inotify_watch = inotify_add_watch(fd, STORAGEDIR,
> +					IN_MODIFY | IN_CREATE | IN_DELETE);
> +	if (inotify_watch < 0) {
> +		connman_error("Creation of STORAGEDIR  watch failed");
> +		close(fd);
> +		return -EIO;
> +	}
> +
> +	inotify_channel = g_io_channel_unix_new(fd);
> +	if (inotify_channel == NULL) {
> +		connman_error("Creation of inotify channel failed");
> +		inotify_rm_watch(fd, inotify_watch);
> +		inotify_watch = 0;
> +
> +		close(fd);
> +		return -EIO;
> +	}
> +
> +	g_io_channel_set_close_on_unref(inotify_channel, TRUE);
> +	g_io_channel_set_encoding(inotify_channel, NULL, NULL);

You to be safe, also g_io_channel_set_buffered(.. FALSE).

> +
> +	channel_watch = g_io_add_watch(inotify_channel,
> +				G_IO_IN | G_IO_HUP | G_IO_NVAL | G_IO_ERR,
> +				inotify_data, NULL);
> +
> +	return 0;
> +}
> +
> +static void remove_watch(void)
> +{
> +	int fd;
> +
> +	if (inotify_channel == NULL)
> +		return;
> +
> +	if (channel_watch != 0)

Pleas do this with > 0.

> +		g_source_remove(channel_watch);
> +
> +	channel_watch = 0;

And stick it all together.

	if (channel_watch > 0) {
		g_source_remove(...
		channel_watch = 0;
	}

> +
> +	fd = g_io_channel_unix_get_fd(inotify_channel);
> +
> +	if (inotify_watch >= 0) {
> +		inotify_rm_watch(fd, inotify_watch);
> +		inotify_watch = 0;
> +	}
> +
> +	g_io_channel_unref(inotify_channel);
> +}
> +
>  int __connman_config_init(void)
>  {
>  	DBG("");
> @@ -413,6 +560,8 @@ int __connman_config_init(void)
>  	config_table = g_hash_table_new_full(g_str_hash, g_str_equal,
>  						NULL, unregister_config);
>  
> +	create_watch();
> +
>  	return read_configs();
>  }
>  
> @@ -422,6 +571,8 @@ void __connman_config_cleanup(void)
>  
>  	g_hash_table_destroy(config_table);
>  	config_table = NULL;
> +
> +	remove_watch();

I think you better remove the watch before destroying the hash table. It
should not matter since we are single threaded, but just in case.

Regards

Marcel





More information about the connman mailing list