Hi Fabien,
On Mon, Aug 30, 2010 at 05:33:13PM +0200, Fabien Marotte wrote:
> 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.
Sure, but we're still not fixing the missing keys issues here.
In conjunction with this patch I'd like to see a patch adding warning when the
corresponding service fields are missing. That would be all from supplicant.c
from now.
Some additional comments:
> ---
> src/config.c | 103
> ++++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 89 insertions(+), 14 deletions(-)
>
> diff --git a/src/config.c b/src/config.c
> index bdbb704..11e470e 100644
> --- a/src/config.c
> +++ b/src/config.c
> @@ -56,6 +56,43 @@ struct connman_config {
>
> static GHashTable *config_table = NULL;
>
> +/***** Definition of possible string in the .config files *****/
Please use the /* xxx */ format.
> +#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,
> +};
> +
> +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,
> +};
Why not having this array NULL terminated ? This would be consistent with
g_key_file_get_keys() return value, and would make your check_keys() prototype
simpler.
> + if (index_possible == nb_possible_keys)
> + connman_warn("Unknown configuration key %s in [%s]",
You have one useless extra space here: ^
> - str = g_key_file_get_string(keyfile, group, "Passphrase", NULL);
> + str = g_key_file_get_string(keyfile, group, SERVICE_KEY_PASSPHRASE,
> NULL);
>
This line is too long.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
