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
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
