Hi Tomasz,
> include/service.h | 7 +
> src/connman.h | 6 -
> src/ipconfig.c | 108 ---------------
> src/service.c | 377
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 377 insertions(+), 121 deletions(-)
the overall patch looks fine to me. So this is just some style issues
here and there.
> diff --git a/include/service.h b/include/service.h
> index 9de5b9c..1b5c1ec 100644
> --- a/include/service.h
> +++ b/include/service.h
> @@ -84,6 +84,13 @@ enum connman_service_error {
> CONNMAN_SERVICE_ERROR_CONNECT_FAILED = 4,
> };
>
> +enum connman_service_proxy {
> + CONNMAN_SERVICE_PROXY_UNKNOWN = 0,
> + CONNMAN_SERVICE_PROXY_DIRECT = 1,
> + CONNMAN_SERVICE_PROXY_MANUAL = 2,
> + CONNMAN_SERVICE_PROXY_AUTO = 3,
> +};
> +
I know the name gets pretty long, but this should be enum
connman_service_proxy_method and CONNMAN_SERVICE_PROXY_METHOD_*.
Next step we could argue to create a include/proxy.c and just call it
CONNMAN_PROXY_METHOD_*. However I would prefer to get this merged first
before thinking about that.
> struct connman_service;
>
> struct connman_service *connman_service_create(void);
> diff --git a/src/connman.h b/src/connman.h
> index 2f8f5af..cbd4fb5 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -231,12 +231,6 @@ void __connman_ipconfig_append_ipv6config(struct
> connman_ipconfig *ipconfig,
> DBusMessageIter *iter);
> int __connman_ipconfig_set_config(struct connman_ipconfig *ipconfig,
> enum connman_ipconfig_type type, DBusMessageIter *array);
> -void __connman_ipconfig_append_proxy(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *iter);
> -void __connman_ipconfig_append_proxyconfig(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *iter);
> -int __connman_ipconfig_set_proxyconfig(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *array);
> void __connman_ipconfig_append_ethernet(struct connman_ipconfig *ipconfig,
> DBusMessageIter *iter);
> enum connman_ipconfig_method __connman_ipconfig_get_method(
> diff --git a/src/ipconfig.c b/src/ipconfig.c
> index 2b60e2d..8a67b41 100644
> --- a/src/ipconfig.c
> +++ b/src/ipconfig.c
> @@ -74,7 +74,6 @@ struct connman_ipdevice {
> char *ipv4_gateway;
> char *ipv6_gateway;
>
> - char *proxy;
> char *pac;
>
> struct connman_ipconfig *config;
> @@ -305,7 +304,6 @@ static void free_ipdevice(gpointer data)
> free_address_list(ipdevice);
> g_free(ipdevice->ipv4_gateway);
> g_free(ipdevice->ipv6_gateway);
> - g_free(ipdevice->proxy);
> g_free(ipdevice->pac);
>
> g_free(ipdevice->address);
> @@ -1633,112 +1631,6 @@ int __connman_ipconfig_set_config(struct
> connman_ipconfig *ipconfig,
> return 0;
> }
>
> -void __connman_ipconfig_append_proxy(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *iter)
> -{
> - struct connman_ipdevice *ipdevice;
> - const char *method = "direct";
> -
> - ipdevice = g_hash_table_lookup(ipdevice_hash,
> - GINT_TO_POINTER(ipconfig->index));
> - if (ipdevice == NULL)
> - goto done;
> -
> - if (ipdevice->pac == NULL)
> - goto done;
> -
> - method = "auto-config";
> -
> - connman_dbus_dict_append_basic(iter, "URL",
> - DBUS_TYPE_STRING, &ipdevice->pac);
> -
> -done:
> - connman_dbus_dict_append_basic(iter, "Method",
> - DBUS_TYPE_STRING, &method);
> -}
> -
> -void __connman_ipconfig_append_proxyconfig(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *iter)
> -{
> - struct connman_ipdevice *ipdevice;
> - const char *method = "auto";
> -
> - ipdevice = g_hash_table_lookup(ipdevice_hash,
> - GINT_TO_POINTER(ipconfig->index));
> - if (ipdevice == NULL)
> - goto done;
> -
> - if (ipdevice->proxy == NULL)
> - goto done;
> -
> - method = ipdevice->proxy;
> -
> -done:
> - connman_dbus_dict_append_basic(iter, "Method",
> - DBUS_TYPE_STRING, &method);
> -}
> -
> -int __connman_ipconfig_set_proxyconfig(struct connman_ipconfig *ipconfig,
> - DBusMessageIter *array)
> -{
> - struct connman_ipdevice *ipdevice;
> - DBusMessageIter dict;
> - const char *method;
> -
> - DBG("ipconfig %p", ipconfig);
> -
> - ipdevice = g_hash_table_lookup(ipdevice_hash,
> - GINT_TO_POINTER(ipconfig->index));
> - if (ipdevice == NULL)
> - return -ENXIO;
> -
> - if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
> - return -EINVAL;
> -
> - dbus_message_iter_recurse(array, &dict);
> -
> - while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> - DBusMessageIter entry;
> - const char *key;
> - int type;
> -
> - dbus_message_iter_recurse(&dict, &entry);
> -
> - if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
> - return -EINVAL;
> -
> - dbus_message_iter_get_basic(&entry, &key);
> - dbus_message_iter_next(&entry);
> -
> - type = dbus_message_iter_get_arg_type(&entry);
> -
> - if (g_str_equal(key, "Method") == TRUE) {
> - if (type != DBUS_TYPE_STRING)
> - return -EINVAL;
> -
> - dbus_message_iter_get_basic(&entry, &method);
> - if (strlen(method) == 0)
> - method = NULL;
> - }
> -
> - dbus_message_iter_next(&dict);
> - }
> -
> - DBG("method %s", method);
> -
> - if (method == NULL)
> - return -EINVAL;
> -
> - if (g_str_equal(method, "auto") == FALSE &&
> - g_str_equal(method, "direct") == FALSE)
> - return -EINVAL;
> -
> - g_free(ipdevice->proxy);
> - ipdevice->proxy = g_strdup(method);
> -
> - return 0;
> -}
> -
> void __connman_ipconfig_append_ethernet(struct connman_ipconfig *ipconfig,
> DBusMessageIter *iter)
> {
> diff --git a/src/service.c b/src/service.c
> index 6a2b19d..286379b 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -100,6 +100,10 @@ struct connman_service {
> struct connman_stats stats;
> struct connman_stats stats_roaming;
> GHashTable *counter_table;
> + enum connman_service_proxy proxy;
> + char **proxies;
> + char **excludes;
> + char *pac;
> };
Make the names a bit clearer. Call it proxy_method, proxy_servers,
proxy_excludes and proxy_url.
I am all for short names, but I think this makes the code a lot easier
to read in the end.
> static void append_path(gpointer value, gpointer user_data)
> @@ -823,23 +827,132 @@ static void append_domainconfig(DBusMessageIter *iter,
> void *user_data)
> DBUS_TYPE_STRING, &service->domains[i]);
> }
>
> +static const char *proxymethod2string(enum connman_service_proxy method)
> +{
> + switch(method) {
There has to be space between swtich and (method).
> + case CONNMAN_SERVICE_PROXY_DIRECT:
> + return "direct";
> + case CONNMAN_SERVICE_PROXY_MANUAL:
> + return "manual";
> + case CONNMAN_SERVICE_PROXY_AUTO:
> + return "auto";
> + default:
> + break;
> + }
> +
> + return NULL;
> +}
No default please. I want the compiler to complain loudly if we changed
the enum. Just add a case CONNMAN_SERVICE_PROXY_UNKNOWN: break;
statement to it.
> +static void append_proxies(DBusMessageIter *iter, void *user_data)
> +{
> + struct connman_service *service = user_data;
> + int i;
> +
> + if (service->proxies == NULL)
> + return;
> +
> + for (i = 0; service->proxies[i]; i++)
> + dbus_message_iter_append_basic(iter,
> + DBUS_TYPE_STRING, &service->proxies[i]);
> +}
> +
> +static void append_excludes(DBusMessageIter *iter, void *user_data)
> +{
> + struct connman_service *service = user_data;
> + int i;
> +
> + if (service->excludes == NULL)
> + return;
> +
> + for (i = 0; service->excludes[i]; i++)
> + dbus_message_iter_append_basic(iter,
> + DBUS_TYPE_STRING, &service->excludes[i]);
> +}
> +
> static void append_proxy(DBusMessageIter *iter, void *user_data)
> {
> struct connman_service *service = user_data;
> + const char *method = proxymethod2string(CONNMAN_SERVICE_PROXY_DIRECT);
> + const char *pac = NULL;
> +
> + DBG("");
>
> if (is_connected(service) == FALSE)
> return;
>
> + /* DHCP/WPAD provided a pac? */
What is this ? for. Just phrase this properly.
/* Check if DHCP or WPAD provided a PAC URL */
Comments should be verbose and not more cryptic than the code itself ;)
> if (service->ipconfig != NULL)
> - __connman_ipconfig_append_proxy(service->ipconfig, iter);
> + pac = __connman_ipconfig_get_proxy_autoconfig(
> + service->ipconfig);
> +
> + switch (service->proxy) {
> + case CONNMAN_SERVICE_PROXY_UNKNOWN:
> + service->proxy = CONNMAN_SERVICE_PROXY_DIRECT;
> + case CONNMAN_SERVICE_PROXY_DIRECT:
> + goto done;
I mentioned this before, if you do want to do fall through then mark
this properly with a comment. We are doing it at a few places. See how
we have done it before.
If you don't mark it than we assume this is a bug. 2 weeks later you
don't remember that this was intentional.
I don't like the goto here btw. That code can be written without the
goto. Just spent a few minutes to think about it. Would make it a lot
simpler to read ;)
> + case CONNMAN_SERVICE_PROXY_MANUAL:
> + connman_dbus_dict_append_array(iter, "Servers",
> + DBUS_TYPE_STRING, append_proxies,
> + service);
> +
> + connman_dbus_dict_append_array(iter, "Excludes",
> + DBUS_TYPE_STRING, append_excludes,
> + service);
> + break;
> + case CONNMAN_SERVICE_PROXY_AUTO:
> + if (service->pac == NULL && pac == NULL)
> + goto done;
> +
> + if (service->pac != NULL)
> + pac = service->pac;
> +
> + connman_dbus_dict_append_basic(iter, "URL",
> + DBUS_TYPE_STRING, &pac);
> + break;
> + default:
> + goto done;
> + }
No default for enum please. I want the compiler to complain if you
forgot a case statement.
> +
> + method = proxymethod2string(service->proxy);
> +
> +done:
> + connman_dbus_dict_append_basic(iter, "Method",
> + DBUS_TYPE_STRING, &method);
> }
>
> static void append_proxyconfig(DBusMessageIter *iter, void *user_data)
> {
> struct connman_service *service = user_data;
> + const char *method = proxymethod2string(CONNMAN_SERVICE_PROXY_AUTO);
>
> - if (service->ipconfig != NULL)
> - __connman_ipconfig_append_proxyconfig(service->ipconfig, iter);
> + switch (service->proxy) {
> + case CONNMAN_SERVICE_PROXY_DIRECT:
> + break;
> + case CONNMAN_SERVICE_PROXY_MANUAL:
> + if (service->proxies != NULL)
> + connman_dbus_dict_append_array(iter, "Servers",
> + DBUS_TYPE_STRING,
> + append_proxies, service);
> +
> + if (service->excludes != NULL)
> + connman_dbus_dict_append_array(iter, "Excludes",
> + DBUS_TYPE_STRING,
> + append_excludes, service);
> + break;
> + case CONNMAN_SERVICE_PROXY_AUTO:
> + if (service->pac != NULL)
> + connman_dbus_dict_append_basic(iter, "URL",
> + DBUS_TYPE_STRING, &service->pac);
> + break;
> + default:
> + goto done;
> + }
No default please and no goto please.
> + method = proxymethod2string(service->proxy);
> +
> +done:
> + connman_dbus_dict_append_basic(iter, "Method",
> + DBUS_TYPE_STRING, &method);
> }
>
> static void append_provider(DBusMessageIter *iter, void *user_data)
> @@ -1464,6 +1577,196 @@ static DBusMessage *get_properties(DBusConnection
> *conn,
> return reply;
> }
>
> +static enum connman_service_proxy string2proxymethod(const char *method)
> +{
> + if (!g_strcmp0(method, "direct"))
> + return CONNMAN_SERVICE_PROXY_DIRECT;
> + else if (!g_strcmp0(method, "auto"))
> + return CONNMAN_SERVICE_PROXY_AUTO;
> + else if (!g_strcmp0(method, "manual"))
> + return CONNMAN_SERVICE_PROXY_MANUAL;
> + else
> + return CONNMAN_SERVICE_PROXY_UNKNOWN;
> +}
> +
Grouping the string2 and 2string functions together would be a good
idea.
> +static int update_proxy_cleanup(GString *servers_str, GString *excludes_str)
> +{
> + if (servers_str != NULL)
> + g_string_free(servers_str, TRUE);
> +
> + if (excludes_str != NULL)
> + g_string_free(excludes_str, TRUE);
> +
> + return -EINVAL;
> +}
Why always return an error? And isn't g_string_free already doing the
NULL check for you?
And on second thought, I don't see the need for this function. That is
complicated. Just free the strings in the function you used them in.
> +static int update_proxy_configuration(struct connman_service *service,
> + DBusMessageIter *array)
> +{
> + DBusMessageIter dict;
> + enum connman_service_proxy method;
> + GString *servers_str = NULL;
> + GString *excludes_str = NULL;
> + const char *url = NULL;
> +
> + method = CONNMAN_SERVICE_PROXY_UNKNOWN;
> +
> + dbus_message_iter_recurse(array, &dict);
> +
> + while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> + DBusMessageIter entry, variant;
> + const char *key;
> + int type;
> +
> + dbus_message_iter_recurse(&dict, &entry);
> +
> + if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
> + return update_proxy_cleanup(servers_str, excludes_str);
> +
> + dbus_message_iter_get_basic(&entry, &key);
> + dbus_message_iter_next(&entry);
> +
> + if (dbus_message_iter_get_arg_type(&entry) !=
> + DBUS_TYPE_VARIANT)
> + return update_proxy_cleanup(servers_str, excludes_str);
> +
> + dbus_message_iter_recurse(&entry, &variant);
> + type = dbus_message_iter_get_arg_type(&variant);
> +
> + if (g_str_equal(key, "Method") == TRUE) {
> + const char *val;
> +
> + if (type != DBUS_TYPE_STRING)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + dbus_message_iter_get_basic(&variant, &val);
> + method = string2proxymethod(val);
> + } else if (g_str_equal(key, "URL") == TRUE) {
> + if (type != DBUS_TYPE_STRING)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + dbus_message_iter_get_basic(&variant, &url);
> + } else if (g_str_equal(key, "Servers") == TRUE) {
> + DBusMessageIter str_array;
> +
> + if (type != DBUS_TYPE_ARRAY)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + servers_str = g_string_new(NULL);
> + if (servers_str == NULL)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + dbus_message_iter_recurse(&variant, &str_array);
> +
> + while (dbus_message_iter_get_arg_type(&str_array) ==
> + DBUS_TYPE_STRING) {
> + char *val = NULL;
> +
> + dbus_message_iter_get_basic(&str_array, &val);
> +
> + if (servers_str->len > 0)
> + g_string_append_printf(servers_str,
> + " %s", val);
> + else
> + g_string_append(servers_str, val);
> +
> + dbus_message_iter_next(&str_array);
> + }
> + } else if (g_str_equal(key, "Excludes") == TRUE) {
> + DBusMessageIter str_array;
> +
> + if (type != DBUS_TYPE_ARRAY)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + excludes_str = g_string_new(NULL);
> + if (excludes_str == NULL)
> + return update_proxy_cleanup(servers_str,
> + excludes_str);
> +
> + dbus_message_iter_recurse(&variant, &str_array);
> +
> + while (dbus_message_iter_get_arg_type(&str_array) ==
> + DBUS_TYPE_STRING) {
> + char *val = NULL;
> +
> + dbus_message_iter_get_basic(&str_array, &val);
> +
> + if (excludes_str->len > 0)
> + g_string_append_printf(excludes_str,
> + " %s", val);
> + else
> + g_string_append(excludes_str, val);
> +
> + dbus_message_iter_next(&str_array);
> + }
> + }
> +
> + dbus_message_iter_next(&dict);
> + }
> +
> + switch (method) {
> + case CONNMAN_SERVICE_PROXY_DIRECT:
> + break;
> + case CONNMAN_SERVICE_PROXY_MANUAL:
> + if (servers_str == NULL && service->proxies == NULL)
> + return update_proxy_cleanup(servers_str, excludes_str);
> +
> + if (servers_str != NULL) {
> + g_strfreev(service->proxies);
> +
> + if (servers_str->len > 0)
> + service->proxies = g_strsplit_set(
> + servers_str->str, " ", 0);
> + else
> + service->proxies = NULL;
> + }
> +
> + if (excludes_str != NULL) {
> + g_strfreev(service->excludes);
> +
> + if (excludes_str->len > 0)
> + service->excludes = g_strsplit_set(
> + excludes_str->str, " ", 0);
> + else
> + service->excludes = NULL;
> + }
> +
> + if (service->proxies == NULL)
> + method = CONNMAN_SERVICE_PROXY_DIRECT;
> +
> + break;
> + case CONNMAN_SERVICE_PROXY_AUTO:
> + g_free(service->pac);
> +
> + if (url != NULL && strlen(url) > 0)
> + service->pac = g_strdup(url);
> + else
> + service->pac = NULL;
> +
> + /* if we are connected:
> + - if service->pac == NULL
> + - if __connman_ipconfig_get_proxy_autoconfig(
> + service->ipconfig) == NULL
> + --> We should start WPAD */
> +
> + break;
> + default:
> + return update_proxy_cleanup(servers_str, excludes_str);
> + }
> +
> + /* Here we don't take care of error code, just cleanup */
> + update_proxy_cleanup(servers_str, excludes_str);
> +
> + service->proxy = method;
> +
> + return 0;
> +}
> +
> static DBusMessage *set_property(DBusConnection *conn,
> DBusMessage *msg, void *user_data)
> {
> @@ -1668,11 +1971,11 @@ static DBusMessage *set_property(DBusConnection *conn,
> } else if (g_str_equal(name, "Proxy.Configuration") == TRUE) {
> int err;
>
> - if (service->ipconfig == NULL)
> - return __connman_error_invalid_property(msg);
> + if (type != DBUS_TYPE_ARRAY)
> + return __connman_error_invalid_arguments(msg);
> +
> + err = update_proxy_configuration(service, &value);
>
> - err = __connman_ipconfig_set_proxyconfig(service->ipconfig,
> - &value);
> if (err < 0)
> return __connman_error_failed(msg, -err);
>
> @@ -2246,9 +2549,12 @@ static void service_free(gpointer user_data)
>
> g_strfreev(service->nameservers);
> g_strfreev(service->domains);
> + g_strfreev(service->proxies);
> + g_strfreev(service->excludes);
>
> g_free(service->nameserver);
> g_free(service->domainname);
> + g_free(service->pac);
> g_free(service->mcc);
> g_free(service->mnc);
> g_free(service->apn);
> @@ -4001,6 +4307,31 @@ static int service_load(struct connman_service
> *service)
> service->domains = NULL;
> }
>
> + str = g_key_file_get_string(keyfile,
> + service->identifier, "Proxy.Method", NULL);
> + service->proxy = string2proxymethod(str);
> +
> + service->proxies = g_key_file_get_string_list(keyfile,
> + service->identifier, "Proxy.Servers", &length, NULL);
> + if (service->proxies != NULL && length == 0) {
> + g_strfreev(service->proxies);
> + service->proxies = NULL;
> + }
> +
> + service->excludes = g_key_file_get_string_list(keyfile,
> + service->identifier, "Proxy.Excludes", &length, NULL);
> + if (service->excludes != NULL && length == 0) {
> + g_strfreev(service->excludes);
> + service->excludes = NULL;
> + }
> +
> + str = g_key_file_get_string(keyfile,
> + service->identifier, "Proxy.URL", NULL);
> + if (str != NULL) {
> + g_free(service->pac);
> + service->pac = str;
> + }
> +
> done:
> g_key_file_free(keyfile);
>
> @@ -4162,6 +4493,38 @@ update:
> g_key_file_remove_key(keyfile, service->identifier,
> "Domains", NULL);
>
> + str = (gchar *) proxymethod2string(service->proxy);
No crazy casting like this please. If you need this, then you are doing
something else wrong. If you need a const char, then get a const char
variable.
> + if (str != NULL)
> + g_key_file_set_string(keyfile, service->identifier,
> + "Proxy.Method", str);
> +
> + if (service->proxies != NULL) {
> + guint len = g_strv_length(service->proxies);
> +
> + g_key_file_set_string_list(keyfile, service->identifier,
> + "Proxy.Servers",
> + (const gchar **) service->proxies, len);
> + } else
> + g_key_file_remove_key(keyfile, service->identifier,
> + "Proxy.Servers", NULL);
> +
> + if (service->excludes != NULL) {
> + guint len = g_strv_length(service->excludes);
> +
> + g_key_file_set_string_list(keyfile, service->identifier,
> + "Proxy.Excludes",
> + (const gchar **) service->excludes, len);
> + } else
> + g_key_file_remove_key(keyfile, service->identifier,
> + "Proxy.Excludes", NULL);
> +
> + if (service->pac != NULL && strlen(service->pac) > 0)
> + g_key_file_set_string(keyfile, service->identifier,
> + "Proxy.URL", service->pac);
> + else
> + g_key_file_remove_key(keyfile, service->identifier,
> + "Proxy.URL", NULL);
> +
> data = g_key_file_to_data(keyfile, &length, NULL);
>
> if (g_file_set_contents(pathname, data, length, NULL) == FALSE)
As I said, mostly some style issues. I am fine with the rest.
Regards
Marcel
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
