Discussion:
[PATCH 0/6] Implement ClearProperty
(too old to reply)
Jaakko Hannikainen
2015-10-15 07:41:32 UTC
Permalink
This patchset refactors the API SendProperty function to a bit
better shape, and then implements ClearProperty so that it will
1) Prevent clearing the 'Error' field
2) Allow clearing readwrite fields to defaults.

The defaults are as following:
- AutoConnect True
- Nameservers.Configuration []
- Timeservers.Configuration []
- Domains.Configuration []
- Proxy.Configuration {"method": "auto"}

Reported by Francesco Bruno.
Jaakko Hannikainen
2015-10-15 07:41:33 UTC
Permalink
Refactor set_property's AutoConnect to a separate function.
---
src/connman.h | 2 ++
src/service.c | 29 +++++++++++++++++++----------
2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 35eb3f5..d1cb71b 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -759,6 +759,8 @@ bool __connman_service_is_hidden(struct connman_service *service);
bool __connman_service_is_split_routing(struct connman_service *service);
bool __connman_service_index_is_split_routing(int index);
int __connman_service_get_index(struct connman_service *service);
+bool __connman_service_set_autoconnect(struct connman_service *service,
+ bool autoconnect);
void __connman_service_set_hidden(struct connman_service *service);
void __connman_service_set_hostname(struct connman_service *service,
const char *hostname);
diff --git a/src/service.c b/src/service.c
index 196f6b5..9027810 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2454,6 +2454,24 @@ int __connman_service_get_index(struct connman_service *service)
return -1;
}

+bool __connman_service_set_autoconnect(struct connman_service *service,
+ bool autoconnect)
+{
+ if (!service || service->autoconnect == autoconnect)
+ return false;
+
+ service->autoconnect = autoconnect;
+
+ autoconnect_changed(service);
+
+ if (autoconnect)
+ __connman_service_auto_connect(CONNMAN_SERVICE_CONNECT_REASON_AUTO);
+
+ service_save(service);
+
+ return true;
+}
+
void __connman_service_set_hidden(struct connman_service *service)
{
if (!service || service->hidden)
@@ -3229,17 +3247,8 @@ static DBusMessage *set_property(DBusConnection *conn,

dbus_message_iter_get_basic(&value, &autoconnect);

- if (service->autoconnect == autoconnect)
+ if (!__connman_service_set_autoconnect(service, autoconnect))
return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
-
- service->autoconnect = autoconnect;
-
- autoconnect_changed(service);
-
- if (autoconnect)
- __connman_service_auto_connect(CONNMAN_SERVICE_CONNECT_REASON_AUTO);
-
- service_save(service);
} else if (g_str_equal(name, "Nameservers.Configuration")) {
DBusMessageIter entry;
GString *str;
--
2.5.3
Jaakko Hannikainen
2015-10-15 07:41:34 UTC
Permalink
Refactor set_property's Nameservers.Configuration to a separate
function.
---
src/connman.h | 2 +
src/service.c | 131 +++++++++++++++++++++++++++++++---------------------------
2 files changed, 73 insertions(+), 60 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index d1cb71b..8f64e65 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -761,6 +761,8 @@ bool __connman_service_index_is_split_routing(int index);
int __connman_service_get_index(struct connman_service *service);
bool __connman_service_set_autoconnect(struct connman_service *service,
bool autoconnect);
+bool __connman_service_set_nameservers_conf(struct connman_service *service,
+ char **nameservers);
void __connman_service_set_hidden(struct connman_service *service);
void __connman_service_set_hostname(struct connman_service *service,
const char *hostname);
diff --git a/src/service.c b/src/service.c
index 9027810..0ed3bdb 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2472,6 +2472,72 @@ bool __connman_service_set_autoconnect(struct connman_service *service,
return true;
}

+static char **remove_empty_strings(char **strv)
+{
+ int index = 0;
+ char **iter = strv;
+
+ while (*iter) {
+ if (**iter)
+ strv[index++] = *iter;
+ else
+ g_free(*iter);
+ iter++;
+ }
+
+ strv[index] = NULL;
+ return strv;
+}
+
+bool __connman_service_set_nameservers_conf(struct connman_service *service,
+ char **nameservers)
+{
+ int index;
+ const char *gw;
+ char **iter;
+
+ if (!service->nameservers_config && !nameservers)
+ return false;
+
+ index = __connman_service_get_index(service);
+ gw = __connman_ipconfig_get_gateway_from_index(index,
+ CONNMAN_IPCONFIG_TYPE_ALL);
+
+ if (gw && strlen(gw))
+ __connman_service_nameserver_del_routes(service,
+ CONNMAN_IPCONFIG_TYPE_ALL);
+
+ nameserver_remove_all(service, CONNMAN_IPCONFIG_TYPE_ALL);
+ g_strfreev(service->nameservers_config);
+ service->nameservers_config = NULL;
+
+ if (nameservers) {
+ for (iter = nameservers; *iter; iter++)
+ if (connman_inet_check_ipaddress(*iter) <= 0)
+ *iter[0] = '\0';
+
+ nameservers = remove_empty_strings(nameservers);
+ service->nameservers_config = nameservers;
+ }
+
+ nameserver_add_all(service, CONNMAN_IPCONFIG_TYPE_ALL);
+ dns_configuration_changed(service);
+
+ if (__connman_service_is_connected_state(service,
+ CONNMAN_IPCONFIG_TYPE_IPV4))
+ __connman_wispr_start(service,
+ CONNMAN_IPCONFIG_TYPE_IPV4);
+
+ if (__connman_service_is_connected_state(service,
+ CONNMAN_IPCONFIG_TYPE_IPV6))
+ __connman_wispr_start(service,
+ CONNMAN_IPCONFIG_TYPE_IPV6);
+
+ service_save(service);
+
+ return true;
+}
+
void __connman_service_set_hidden(struct connman_service *service)
{
if (!service || service->hidden)
@@ -2941,23 +3007,6 @@ static DBusMessage *get_properties(DBusConnection *conn,
return reply;
}

-static char **remove_empty_strings(char **strv)
-{
- int index = 0;
- char **iter = strv;
-
- while (*iter) {
- if (**iter)
- strv[index++] = *iter;
- else
- g_free(*iter);
- iter++;
- }
-
- strv[index] = NULL;
- return strv;
-}
-
static int update_proxy_configuration(struct connman_service *service,
DBusMessageIter *array)
{
@@ -3252,8 +3301,7 @@ static DBusMessage *set_property(DBusConnection *conn,
} else if (g_str_equal(name, "Nameservers.Configuration")) {
DBusMessageIter entry;
GString *str;
- int index;
- const char *gw;
+ char **nameservers = NULL;

if (__connman_provider_is_immutable(service->provider) ||
service->immutable)
@@ -3266,14 +3314,6 @@ static DBusMessage *set_property(DBusConnection *conn,
if (!str)
return __connman_error_invalid_arguments(msg);

- index = __connman_service_get_index(service);
- gw = __connman_ipconfig_get_gateway_from_index(index,
- CONNMAN_IPCONFIG_TYPE_ALL);
-
- if (gw && strlen(gw))
- __connman_service_nameserver_del_routes(service,
- CONNMAN_IPCONFIG_TYPE_ALL);
-
dbus_message_iter_recurse(&value, &entry);

while (dbus_message_iter_get_arg_type(&entry) == DBUS_TYPE_STRING) {
@@ -3290,43 +3330,14 @@ static DBusMessage *set_property(DBusConnection *conn,
g_string_append(str, val);
}

- nameserver_remove_all(service, CONNMAN_IPCONFIG_TYPE_ALL);
- g_strfreev(service->nameservers_config);
-
- if (str->len > 0) {
- char **nameservers, **iter;
-
+ if (str->len > 0)
nameservers = g_strsplit_set(str->str, " ", 0);

- for (iter = nameservers; *iter; iter++)
- if (connman_inet_check_ipaddress(*iter) <= 0)
- *iter[0] = '\0';
-
- nameservers = remove_empty_strings(nameservers);
- service->nameservers_config = nameservers;
- } else {
- service->nameservers_config = NULL;
- }
-
g_string_free(str, TRUE);

- if (gw && strlen(gw))
- __connman_service_nameserver_add_routes(service, gw);
-
- nameserver_add_all(service, CONNMAN_IPCONFIG_TYPE_ALL);
- dns_configuration_changed(service);
-
- if (__connman_service_is_connected_state(service,
- CONNMAN_IPCONFIG_TYPE_IPV4))
- __connman_wispr_start(service,
- CONNMAN_IPCONFIG_TYPE_IPV4);
-
- if (__connman_service_is_connected_state(service,
- CONNMAN_IPCONFIG_TYPE_IPV6))
- __connman_wispr_start(service,
- CONNMAN_IPCONFIG_TYPE_IPV6);
-
- service_save(service);
+ if (!__connman_service_set_nameservers_conf(service,
+ nameservers))
+ return __connman_error_invalid_arguments(msg);
} else if (g_str_equal(name, "Timeservers.Configuration")) {
DBusMessageIter entry;
GString *str;
--
2.5.3
Jaakko Hannikainen
2015-10-15 07:41:35 UTC
Permalink
Refactor set_property's Timeservers.Configuration to a separate
function.
---
src/connman.h | 2 ++
src/service.c | 46 +++++++++++++++++++++++++++++++---------------
2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 8f64e65..9f9f665 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -763,6 +763,8 @@ bool __connman_service_set_autoconnect(struct connman_service *service,
bool autoconnect);
bool __connman_service_set_nameservers_conf(struct connman_service *service,
char **nameservers);
+bool __connman_service_set_timeservers_conf(struct connman_service *service,
+ char **nameservers);
void __connman_service_set_hidden(struct connman_service *service);
void __connman_service_set_hostname(struct connman_service *service,
const char *hostname);
diff --git a/src/service.c b/src/service.c
index 0ed3bdb..8fefe6a 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2538,6 +2538,31 @@ bool __connman_service_set_nameservers_conf(struct connman_service *service,
return true;
}

+bool __connman_service_set_timeservers_conf(struct connman_service *service,
+ char **timeservers)
+{
+ if (service->immutable) {
+ g_strfreev(timeservers);
+ return false;
+ }
+
+ g_strfreev(service->timeservers_config);
+ service->timeservers_config = NULL;
+
+ if (timeservers) {
+ timeservers = remove_empty_strings(timeservers);
+ service->timeservers_config = timeservers;
+ }
+
+ service_save(service);
+ timeservers_configuration_changed(service);
+
+ if (service == __connman_service_get_default())
+ __connman_timeserver_sync(service);
+
+ return true;
+}
+
void __connman_service_set_hidden(struct connman_service *service)
{
if (!service || service->hidden)
@@ -3341,6 +3366,7 @@ static DBusMessage *set_property(DBusConnection *conn,
} else if (g_str_equal(name, "Timeservers.Configuration")) {
DBusMessageIter entry;
GString *str;
+ char **timeservers = NULL;

if (service->immutable)
return __connman_error_not_supported(msg);
@@ -3368,24 +3394,14 @@ static DBusMessage *set_property(DBusConnection *conn,
g_string_append(str, val);
}

- g_strfreev(service->timeservers_config);
- service->timeservers_config = NULL;
-
- if (str->len > 0) {
- char **timeservers = g_strsplit_set(str->str, " ", 0);
- timeservers = remove_empty_strings(timeservers);
- service->timeservers_config = timeservers;
- } else
- service->timeservers = NULL;
+ if (str->len > 0)
+ timeservers = g_strsplit_set(str->str, " ", 0);

g_string_free(str, TRUE);

- service_save(service);
- timeservers_configuration_changed(service);
-
- if (service == __connman_service_get_default())
- __connman_timeserver_sync(service);
-
+ if (!__connman_service_set_timeservers_conf(service,
+ timeservers))
+ return __connman_error_invalid_arguments(msg);
} else if (g_str_equal(name, "Domains.Configuration")) {
DBusMessageIter entry;
GString *str;
--
2.5.3
Jaakko Hannikainen
2015-10-15 07:41:36 UTC
Permalink
Refactor set_property's Domains.Configuration to a separate function.
---
src/connman.h | 2 ++
src/service.c | 44 +++++++++++++++++++++++++++++++-------------
2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 9f9f665..914c623 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -765,6 +765,8 @@ bool __connman_service_set_nameservers_conf(struct connman_service *service,
char **nameservers);
bool __connman_service_set_timeservers_conf(struct connman_service *service,
char **nameservers);
+bool __connman_service_set_domains_conf(struct connman_service *service,
+ char **domains);
void __connman_service_set_hidden(struct connman_service *service);
void __connman_service_set_hostname(struct connman_service *service,
const char *hostname);
diff --git a/src/service.c b/src/service.c
index 8fefe6a..7929113 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2563,6 +2563,32 @@ bool __connman_service_set_timeservers_conf(struct connman_service *service,
return true;
}

+bool __connman_service_set_domains_conf(struct connman_service *service,
+ char **domains)
+{
+ if (service->immutable) {
+ g_strfreev(domains);
+ return false;
+ }
+
+ searchdomain_remove_all(service);
+ g_strfreev(service->domains);
+ service->domains = NULL;
+
+ if (domains) {
+ domains = remove_empty_strings(domains);
+ service->domains = domains;
+ }
+
+ searchdomain_add_all(service);
+ domain_configuration_changed(service);
+ domain_changed(service);
+
+ service_save(service);
+
+ return true;
+}
+
void __connman_service_set_hidden(struct connman_service *service)
{
if (!service || service->hidden)
@@ -3405,6 +3431,7 @@ static DBusMessage *set_property(DBusConnection *conn,
} else if (g_str_equal(name, "Domains.Configuration")) {
DBusMessageIter entry;
GString *str;
+ char **domains = NULL;

if (service->immutable)
return __connman_error_not_supported(msg);
@@ -3432,23 +3459,14 @@ static DBusMessage *set_property(DBusConnection *conn,
g_string_append(str, val);
}

- searchdomain_remove_all(service);
- g_strfreev(service->domains);
-
- if (str->len > 0) {
- char **domains = g_strsplit_set(str->str, " ", 0);
- domains = remove_empty_strings(domains);
- service->domains = domains;
- } else
- service->domains = NULL;
+ if (str->len > 0)
+ domains = g_strsplit_set(str->str, " ", 0);

g_string_free(str, TRUE);

- searchdomain_add_all(service);
- domain_configuration_changed(service);
- domain_changed(service);
+ if (!__connman_service_set_domains_conf(service, domains))
+ return __connman_error_invalid_arguments(msg);

- service_save(service);
} else if (g_str_equal(name, "Proxy.Configuration")) {
int err;
--
2.5.3
Jaakko Hannikainen
2015-10-15 07:41:37 UTC
Permalink
Refactor set_property's Proxy.Configuration to a separate function.
---
src/connman.h | 3 ++
src/service.c | 169 +++++++++++++++++++++++++++++-----------------------------
2 files changed, 88 insertions(+), 84 deletions(-)

diff --git a/src/connman.h b/src/connman.h
index 914c623..eb83d8a 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -767,6 +767,9 @@ bool __connman_service_set_timeservers_conf(struct connman_service *service,
char **nameservers);
bool __connman_service_set_domains_conf(struct connman_service *service,
char **domains);
+bool __connman_service_set_proxy_conf(struct connman_service *service,
+ enum connman_service_proxy_method method,
+ char *url, char **servers, char **excludes);
void __connman_service_set_hidden(struct connman_service *service);
void __connman_service_set_hostname(struct connman_service *service,
const char *hostname);
diff --git a/src/service.c b/src/service.c
index 7929113..e9107c2 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2589,6 +2589,72 @@ bool __connman_service_set_domains_conf(struct connman_service *service,
return true;
}

+bool __connman_service_set_proxy_conf(struct connman_service *service,
+ enum connman_service_proxy_method method,
+ char *url, char **servers, char **excludes)
+{
+ switch (method) {
+ case CONNMAN_SERVICE_PROXY_METHOD_DIRECT:
+ break;
+ case CONNMAN_SERVICE_PROXY_METHOD_MANUAL:
+ if (!servers)
+ goto error;
+
+ g_strfreev(service->proxies);
+ servers = remove_empty_strings(servers);
+ service->proxies = servers;
+
+ g_strfreev(service->excludes);
+ service->excludes = NULL;
+
+ if (excludes) {
+ excludes = remove_empty_strings(excludes);
+ service->excludes = excludes;
+ }
+
+ break;
+ case CONNMAN_SERVICE_PROXY_METHOD_AUTO:
+ g_free(service->pac);
+
+ if (url && strlen(url) > 0)
+ service->pac = g_strstrip(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;
+ case CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN:
+ goto error;
+ }
+
+ g_free(url);
+
+ if (method != CONNMAN_SERVICE_PROXY_METHOD_MANUAL) {
+ g_strfreev(servers);
+ g_strfreev(excludes);
+ }
+
+ service->proxy_config = method;
+ proxy_configuration_changed(service);
+ __connman_notifier_proxy_changed(service);
+
+ service_save(service);
+
+ return true;
+
+error:
+ g_free(url);
+ g_strfreev(servers);
+ g_strfreev(excludes);
+
+ return false;
+}
+
void __connman_service_set_hidden(struct connman_service *service)
{
if (!service || service->hidden)
@@ -3065,6 +3131,8 @@ static int update_proxy_configuration(struct connman_service *service,
enum connman_service_proxy_method method;
GString *servers_str = NULL;
GString *excludes_str = NULL;
+ char **servers = NULL;
+ char **excludes = NULL;
const char *url = NULL;

method = CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN;
@@ -3079,13 +3147,13 @@ static int update_proxy_configuration(struct connman_service *service,
dbus_message_iter_recurse(&dict, &entry);

if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
- goto error;
+ return -EINVAL;

dbus_message_iter_get_basic(&entry, &key);
dbus_message_iter_next(&entry);

if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
- goto error;
+ return -EINVAL;

dbus_message_iter_recurse(&entry, &variant);

@@ -3095,24 +3163,24 @@ static int update_proxy_configuration(struct connman_service *service,
const char *val;

if (type != DBUS_TYPE_STRING)
- goto error;
+ return -EINVAL;

dbus_message_iter_get_basic(&variant, &val);
method = string2proxymethod(val);
} else if (g_str_equal(key, "URL")) {
if (type != DBUS_TYPE_STRING)
- goto error;
+ return -EINVAL;

dbus_message_iter_get_basic(&variant, &url);
} else if (g_str_equal(key, "Servers")) {
DBusMessageIter str_array;

if (type != DBUS_TYPE_ARRAY)
- goto error;
+ return -EINVAL;

servers_str = g_string_new(NULL);
if (!servers_str)
- goto error;
+ return -ENOMEM;

dbus_message_iter_recurse(&variant, &str_array);

@@ -3130,15 +3198,18 @@ static int update_proxy_configuration(struct connman_service *service,

dbus_message_iter_next(&str_array);
}
+ if (servers_str->len > 0)
+ servers = g_strsplit(servers_str->str, " ", 0);
+ g_string_free(servers_str, TRUE);
} else if (g_str_equal(key, "Excludes")) {
DBusMessageIter str_array;

if (type != DBUS_TYPE_ARRAY)
- goto error;
+ return -EINVAL;

excludes_str = g_string_new(NULL);
if (!excludes_str)
- goto error;
+ return -ENOMEM;

dbus_message_iter_recurse(&variant, &str_array);

@@ -3156,83 +3227,19 @@ static int update_proxy_configuration(struct connman_service *service,

dbus_message_iter_next(&str_array);
}
+ if (excludes_str && excludes_str->len > 1)
+ excludes = g_strsplit(excludes_str->str, " ", 0);
+ g_string_free(excludes_str, TRUE);
}

dbus_message_iter_next(&dict);
}

- switch (method) {
- case CONNMAN_SERVICE_PROXY_METHOD_DIRECT:
- break;
- case CONNMAN_SERVICE_PROXY_METHOD_MANUAL:
- if (!servers_str && !service->proxies)
- goto error;
-
- if (servers_str) {
- g_strfreev(service->proxies);
-
- if (servers_str->len > 0) {
- char **proxies = g_strsplit_set(
- servers_str->str, " ", 0);
- proxies = remove_empty_strings(proxies);
- service->proxies = proxies;
- } else
- service->proxies = NULL;
- }
-
- if (excludes_str) {
- g_strfreev(service->excludes);
-
- if (excludes_str->len > 0) {
- char **excludes = g_strsplit_set(
- excludes_str->str, " ", 0);
- excludes = remove_empty_strings(excludes);
- service->excludes = excludes;
- } else
- service->excludes = NULL;
- }
-
- if (!service->proxies)
- method = CONNMAN_SERVICE_PROXY_METHOD_DIRECT;
-
- break;
- case CONNMAN_SERVICE_PROXY_METHOD_AUTO:
- g_free(service->pac);
-
- if (url && strlen(url) > 0)
- service->pac = g_strstrip(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;
- case CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN:
- goto error;
- }
-
- if (servers_str)
- g_string_free(servers_str, TRUE);
-
- if (excludes_str)
- g_string_free(excludes_str, TRUE);
-
- service->proxy_config = method;
+ if (!__connman_service_set_proxy_conf(service, method, g_strdup(url),
+ servers, excludes))
+ return -EINVAL;

return 0;
-
-error:
- if (servers_str)
- g_string_free(servers_str, TRUE);
-
- if (excludes_str)
- g_string_free(excludes_str, TRUE);
-
- return -EINVAL;
}

int __connman_service_reset_ipconfig(struct connman_service *service,
@@ -3480,12 +3487,6 @@ static DBusMessage *set_property(DBusConnection *conn,

if (err < 0)
return __connman_error_failed(msg, -err);
-
- proxy_configuration_changed(service);
-
- __connman_notifier_proxy_changed(service);
-
- service_save(service);
} else if (g_str_equal(name, "IPv4.Configuration") ||
g_str_equal(name, "IPv6.Configuration")) {
--
2.5.3
Jaakko Hannikainen
2015-10-15 07:41:38 UTC
Permalink
The previous implementation only cleared Error, which as per
documentation is wrong since Error is readonly. Add sane
defaults to all readwrite fields.

Reported by Francesco Bruno.
---
src/service.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/service.c b/src/service.c
index e9107c2..fe20763 100644
--- a/src/service.c
+++ b/src/service.c
@@ -3582,11 +3582,32 @@ static DBusMessage *clear_property(DBusConnection *conn,
dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &name,
DBUS_TYPE_INVALID);

- if (g_str_equal(name, "Error")) {
- set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
-
- g_get_current_time(&service->modified);
- service_save(service);
+ if (service->hidden) {
+ return __connman_error_not_supported(msg);
+ } else if (g_str_equal(name, "AutoConnect")) {
+ __connman_service_set_autoconnect(service, true);
+ } else if (service->immutable) {
+ return __connman_error_not_supported(msg);
+ } else if (g_str_equal(name, "Nameservers.Configuration")) {
+ __connman_service_set_nameservers_conf(service, NULL);
+ } else if (g_str_equal(name, "Timeservers.Configuration")) {
+ __connman_service_set_timeservers_conf(service, NULL);
+ } else if (g_str_equal(name, "Domains.Configuration")) {
+ __connman_service_set_domains_conf(service, NULL);
+ } else if (g_str_equal(name, "Proxy.Configuration")) {
+ __connman_service_set_proxy_conf(service,
+ CONNMAN_SERVICE_PROXY_METHOD_AUTO,
+ NULL, NULL, NULL);
+ } else if (g_str_equal(name, "IPv4.Configuration")) {
+ __connman_service_reset_ipconfig(service,
+ CONNMAN_IPCONFIG_TYPE_IPV4, NULL, NULL);
+ __connman_ipconfig_set_method(service->ipconfig_ipv4,
+ CONNMAN_IPCONFIG_METHOD_DHCP);
+ __connman_network_enable_ipconfig(service->network,
+ service->ipconfig_ipv4);
+ } else if (g_str_equal(name, "IPv6.Configuration")) {
+ __connman_service_reset_ipconfig(service,
+ CONNMAN_IPCONFIG_TYPE_IPV6, NULL, NULL);
} else
return __connman_error_invalid_property(msg);
--
2.5.3
Patrik Flykt
2015-10-20 05:51:14 UTC
Permalink
Hi,

Thanks for these patches. I'm going to add text from the cover letter to
this patch and nitpick on clearing AutoConnect below.
Post by Jaakko Hannikainen
The previous implementation only cleared Error, which as per
documentation is wrong since Error is readonly. Add sane
defaults to all readwrite fields.
Reported by Francesco Bruno.
---
src/service.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/service.c b/src/service.c
index e9107c2..fe20763 100644
--- a/src/service.c
+++ b/src/service.c
@@ -3582,11 +3582,32 @@ static DBusMessage *clear_property(DBusConnection *conn,
dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &name,
DBUS_TYPE_INVALID);
- if (g_str_equal(name, "Error")) {
- set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
-
- g_get_current_time(&service->modified);
- service_save(service);
+ if (service->hidden) {
+ return __connman_error_not_supported(msg);
+ } else if (g_str_equal(name, "AutoConnect")) {
+ __connman_service_set_autoconnect(service, true);
Here I'd say clearing means to unset the value 'true'. I.e. the result
is not that one gets a default value but the cleared one. Nitpicking on
this means that the default value of AutoConnect is really false, it's
only the DefaultAutoConnectTechnologies ones that are set to true.

I'll change this to avoid an extra round-trip.

Cheers,

Patrik
Post by Jaakko Hannikainen
+ } else if (service->immutable) {
+ return __connman_error_not_supported(msg);
+ } else if (g_str_equal(name, "Nameservers.Configuration")) {
+ __connman_service_set_nameservers_conf(service, NULL);
+ } else if (g_str_equal(name, "Timeservers.Configuration")) {
+ __connman_service_set_timeservers_conf(service, NULL);
+ } else if (g_str_equal(name, "Domains.Configuration")) {
+ __connman_service_set_domains_conf(service, NULL);
+ } else if (g_str_equal(name, "Proxy.Configuration")) {
+ __connman_service_set_proxy_conf(service,
+ CONNMAN_SERVICE_PROXY_METHOD_AUTO,
+ NULL, NULL, NULL);
+ } else if (g_str_equal(name, "IPv4.Configuration")) {
+ __connman_service_reset_ipconfig(service,
+ CONNMAN_IPCONFIG_TYPE_IPV4, NULL, NULL);
+ __connman_ipconfig_set_method(service->ipconfig_ipv4,
+ CONNMAN_IPCONFIG_METHOD_DHCP);
+ __connman_network_enable_ipconfig(service->network,
+ service->ipconfig_ipv4);
+ } else if (g_str_equal(name, "IPv6.Configuration")) {
+ __connman_service_reset_ipconfig(service,
+ CONNMAN_IPCONFIG_TYPE_IPV6, NULL, NULL);
} else
return __connman_error_invalid_property(msg);
Marcel Holtmann
2015-10-20 08:11:22 UTC
Permalink
Hi Patrik,
Post by Patrik Flykt
Thanks for these patches. I'm going to add text from the cover letter to
this patch and nitpick on clearing AutoConnect below.
Post by Jaakko Hannikainen
The previous implementation only cleared Error, which as per
documentation is wrong since Error is readonly. Add sane
defaults to all readwrite fields.
Reported by Francesco Bruno.
---
src/service.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/src/service.c b/src/service.c
index e9107c2..fe20763 100644
--- a/src/service.c
+++ b/src/service.c
@@ -3582,11 +3582,32 @@ static DBusMessage *clear_property(DBusConnection *conn,
dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &name,
DBUS_TYPE_INVALID);
- if (g_str_equal(name, "Error")) {
- set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
-
- g_get_current_time(&service->modified);
- service_save(service);
+ if (service->hidden) {
+ return __connman_error_not_supported(msg);
+ } else if (g_str_equal(name, "AutoConnect")) {
+ __connman_service_set_autoconnect(service, true);
Here I'd say clearing means to unset the value 'true'. I.e. the result
is not that one gets a default value but the cleared one. Nitpicking on
this means that the default value of AutoConnect is really false, it's
only the DefaultAutoConnectTechnologies ones that are set to true.
I'll change this to avoid an extra round-trip.
why are we doing this at all. The reason why ClearProperty exists is so that you can clear read-only values like "Error" That is the only thing it is intended for. Nothing else.

For anything that that read-write this is pointless to support. Also if I wanted it to reset to default values, then it would have been called ResetProperty and not ClearProperty. It is just for clearing an error condition that ConnMan entered into where is has no capability to get itself out of. So the one time when user interaction is needed to clear an error.

Regards

Marcel
Patrik Flykt
2015-10-20 08:57:25 UTC
Permalink
Post by Marcel Holtmann
why are we doing this at all. The reason why ClearProperty exists is
so that you can clear read-only values like "Error" That is the only
thing it is intended for. Nothing else.
For anything that that read-write this is pointless to support. Also
if I wanted it to reset to default values, then it would have been
called ResetProperty and not ClearProperty. It is just for clearing an
error condition that ConnMan entered into where is has no capability
to get itself out of. So the one time when user interaction is needed
to clear an error.
So the documentation is less clear on the issue for the ClearProperty
method call. Let's have that one fixed instead according to the above.

Cheers,

Patrik

Loading...