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