Hi Fabien,
> Connman reads the *.config files in STORAGE_DIR during its boot but
> it looks only for parameters it is interrested in.
> The configuration parameters syntax is complex and it is very
> simple to make a mistake without getting connman warnings. This
> leads the user to think that connman understands the configuration
> file although it doesn't.
>
> This patch is the code that adds warnings to the logs if connman
> reads unknown parameters from the *.config files.
> ---
> src/config.c | 105
> ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/src/config.c b/src/config.c
> index bdbb704..0d69017 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -56,6 +56,45 @@ struct connman_config {
>
> static GHashTable *config_table = NULL;
>
> +/* Definition of possible strings in the .config files */
> +#define CONFIG_KEY_NAME "Name"
> +#define CONFIG_KEY_DESC "Description"
> +
> +#define SERVICE_KEY_TYPE "Type"
> +#define SERVICE_KEY_NAME "Name"
> +#define SERVICE_KEY_SSID "SSID"
> +#define SERVICE_KEY_EAP "EAP"
> +#define SERVICE_KEY_CA_CERT "CACertFile"
> +#define SERVICE_KEY_CL_CERT "ClientCertFile"
> +#define SERVICE_KEY_PRV_KEY "PrivateKeyFile"
> +#define SERVICE_KEY_PRV_KEY_PASS "PrivateKeyPassphrase"
> +#define SERVICE_KEY_PRV_KEY_PASS_TYPE "PrivateKeyPassphraseType"
> +#define SERVICE_KEY_IDENTITY "Identity"
> +#define SERVICE_KEY_PHASE2 "Phase2"
> +#define SERVICE_KEY_PASSPHRASE "Passphrase"
> +
> +gchar *config_possible_keys[] = {
> + CONFIG_KEY_NAME,
> + CONFIG_KEY_DESC,
> + NULL,
> +};
> +
> +gchar *service_possible_keys[] = {
> + SERVICE_KEY_TYPE,
> + SERVICE_KEY_NAME,
> + SERVICE_KEY_SSID,
> + SERVICE_KEY_EAP,
> + SERVICE_KEY_CA_CERT,
> + SERVICE_KEY_CL_CERT,
> + SERVICE_KEY_PRV_KEY,
> + SERVICE_KEY_PRV_KEY_PASS,
> + SERVICE_KEY_PRV_KEY_PASS_TYPE,
> + SERVICE_KEY_IDENTITY,
> + SERVICE_KEY_PHASE2,
> + SERVICE_KEY_PASSPHRASE,
> + NULL,
> +};
please make these static const char.
The usage of gchar is not encouraged at all. Even if we initially made
that mistake. gchar is wrong and a bad idea.
> static void unregister_config(gpointer data)
> {
> struct connman_config *config = data;
> @@ -92,6 +131,36 @@ static void unregister_service(gpointer data)
> g_free(service);
> }
>
> +void check_keys(GKeyFile *keyfile, const char *group, gchar **possible_keys)
> +{
Why is this not static. Please use bootstrap-configure to enable our set
of compiler warnings. And it would tell you.
> + gchar **avail_keys = NULL;
> + gsize nb_avail_keys;
> + int index_avail, index_possible;
> + GError *error = NULL;
> +
> + avail_keys = g_key_file_get_keys(keyfile, group, &nb_avail_keys,
> + &error);
There is just no point to initialize avail_keys to NULL if you assign it
some instructions later. In general I don't wanna see variable
initialization at all. They are hiding real issues. And the compiler
will warn us if we accidentally used it uninitialized.
> + if (error) {
> + connman_warn("Unable to check keys validity for [%s]", group);
> + return;
> + }
You do have to free the error here.
However I say we just don't care at all here and just give NULL for
error. If avail_keys return NULL, we just silently return from the
function. No need to print a warning for an empty configuration file.
> + for (index_avail = 0 ; index_avail < nb_avail_keys; index_avail++) {
> + for (index_possible = 0; possible_keys[index_possible] ;
> + index_possible++) {
> + if (g_strcmp0(avail_keys[index_avail],
> + possible_keys[index_possible]) == 0)
> + break;
> + }
Don't use index_avail and index_possible. While I want descriptive
variable names, this is rather pointlessly descriptor. An incrementor
variable should be something like i or n or something really short.
And I would just create a small helper function that checks if given
string is part of that array. Makes the code a lot more readable. And
especially since you have two arrays anyway it can be nicely re-used.
> + if (possible_keys[index_possible] == NULL)
> + connman_warn("Unknown configuration key %s in [%s]",
> + avail_keys[index_avail], group);
> + }
> +
> + g_strfreev(avail_keys);
> +}
> +
> static int load_service(GKeyFile *keyfile, const char *group,
> struct connman_config *config)
> {
> @@ -105,6 +174,9 @@ static int load_service(GKeyFile *keyfile, const char
> *group,
> if (strlen(ident) < 1)
> return -EINVAL;
>
> + /* Verify that provided keys are good */
> + check_keys(keyfile, group, service_possible_keys);
> +
> service = g_hash_table_lookup(config->service_table, ident);
> if (service == NULL) {
> service = g_try_new0(struct connman_config_service, 1);
> @@ -114,19 +186,20 @@ static int load_service(GKeyFile *keyfile, const char
> *group,
> service->ident = g_strdup(ident);
> }
>
> - str = g_key_file_get_string(keyfile, group, "Type", NULL);
> + str = g_key_file_get_string(keyfile, group, SERVICE_KEY_TYPE, NULL);
This clutters the patch a bit. Just send an initial patch that changes
the direct string usage into using defines.
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
