Hi Patrik,
On ke, 2015-02-11 at 13:54 +0200, Patrik Flykt wrote:
> Configure IPv4 and IPv6 configurations only once in
> __connman_network_enable_ipconfig() and use this function when setting
> the network connected. Remove obsolete function and rework the rest
> to centralise configuration state and error reporting.
>
> The variable network->connecting is true as long as the IPv4 or IPv6
> address configuration phase is ongoing.
> ---
>
>
> Hi,
>
> This is a fairly large patch that removes duplicate code in set_connected()
> in favor of using the one in __connman_network_set_ipconfig(). There is
> no change in functionality in this patch.
>
> v2 fixes a glitch with variable naming wrt rebasing
>
>
> Please test,
>
> Patrik
>
>
> src/connman.h | 5 +-
> src/network.c | 218
> +++++++++++++++++-----------------------------------------
> src/service.c | 21 +++---
> 3 files changed, 80 insertions(+), 164 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index 8d4a692..d4765cc 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -587,9 +587,8 @@ int __connman_network_connect(struct connman_network
> *network);
> int __connman_network_disconnect(struct connman_network *network);
> int __connman_network_clear_ipconfig(struct connman_network *network,
> struct connman_ipconfig *ipconfig);
> -int __connman_network_set_ipconfig(struct connman_network *network,
> - struct connman_ipconfig *ipconfig_ipv4,
> - struct connman_ipconfig *ipconfig_ipv6);
> +int __connman_network_enable_ipconfig(struct connman_network *network,
> + struct connman_ipconfig *ipconfig);
>
> const char *__connman_network_get_type(struct connman_network *network);
> const char *__connman_network_get_group(struct connman_network *network);
> diff --git a/src/network.c b/src/network.c
> index 0cef220..253966e 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -153,10 +153,6 @@ static void dhcp_success(struct connman_network *network)
> if (!service)
> goto err;
>
> - connman_network_set_associating(network, false);
> -
> - network->connecting = false;
> -
> ipconfig_ipv4 = __connman_service_get_ip4config(service);
>
> DBG("lease acquired for ipconfig %p", ipconfig_ipv4);
> @@ -188,9 +184,6 @@ static void dhcp_failure(struct connman_network *network)
> if (!service)
> return;
>
> - connman_network_set_associating(network, false);
> - network->connecting = false;
> -
> ipconfig_ipv4 = __connman_service_get_ip4config(service);
>
> DBG("lease lost for ipconfig %p", ipconfig_ipv4);
> @@ -206,55 +199,24 @@ static void dhcp_callback(struct connman_ipconfig
> *ipconfig,
> struct connman_network *network,
> bool success, gpointer data)
> {
> + network->connecting = false;
> +
> if (success)
> dhcp_success(network);
> else
> dhcp_failure(network);
> }
>
> -static int set_connected_fixed(struct connman_network *network)
> -{
> - struct connman_service *service;
> - struct connman_ipconfig *ipconfig_ipv4;
> - int err;
> -
> - DBG("");
> -
> - service = connman_service_lookup_from_network(network);
> -
> - ipconfig_ipv4 = __connman_service_get_ip4config(service);
> -
> - set_configuration(network, CONNMAN_IPCONFIG_TYPE_IPV4);
> -
> - network->connecting = false;
> -
> - connman_network_set_associating(network, false);
> -
> - err = __connman_ipconfig_address_add(ipconfig_ipv4);
> - if (err < 0)
> - goto err;
> -
> - err = __connman_ipconfig_gateway_add(ipconfig_ipv4);
> - if (err < 0)
> - goto err;
> -
> - return 0;
> -
> -err:
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_CONFIGURE_FAIL);
> -
> - return err;
> -}
> -
> -static void set_connected_manual(struct connman_network *network)
> +static int set_connected_manual(struct connman_network *network)
> {
> + int err = 0;
> struct connman_service *service;
> struct connman_ipconfig *ipconfig;
> - int err;
>
> DBG("network %p", network);
>
> + network->connecting = false;
> +
> service = connman_service_lookup_from_network(network);
>
> ipconfig = __connman_service_get_ip4config(service);
> @@ -262,8 +224,6 @@ static void set_connected_manual(struct connman_network
> *network)
> if (!__connman_ipconfig_get_local(ipconfig))
> __connman_service_read_ip4config(service);
>
> - set_configuration(network, CONNMAN_IPCONFIG_TYPE_IPV4);
> -
> err = __connman_ipconfig_address_add(ipconfig);
> if (err < 0)
> goto err;
> @@ -272,16 +232,8 @@ static void set_connected_manual(struct connman_network
> *network)
> if (err < 0)
> goto err;
>
> - network->connecting = false;
> -
> - connman_network_set_associating(network, false);
> -
> - return;
> -
> err:
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_CONFIGURE_FAIL);
> - return;
> + return err;
> }
>
> static int set_connected_dhcp(struct connman_network *network)
> @@ -292,8 +244,6 @@ static int set_connected_dhcp(struct connman_network
> *network)
>
> DBG("network %p", network);
>
> - set_configuration(network, CONNMAN_IPCONFIG_TYPE_IPV4);
> -
> service = connman_service_lookup_from_network(network);
> ipconfig_ipv4 = __connman_service_get_ip4config(service);
>
> @@ -351,6 +301,8 @@ static int manual_ipv6_set(struct connman_network
> *network,
>
> static void stop_dhcpv6(struct connman_network *network)
> {
> + network->connecting = false;
> +
> __connman_dhcpv6_stop(network);
> }
>
> @@ -632,13 +584,13 @@ static void autoconf_ipv6_set(struct connman_network
> *network)
> static void set_connected(struct connman_network *network)
> {
> struct connman_ipconfig *ipconfig_ipv4, *ipconfig_ipv6;
> - enum connman_ipconfig_method ipv4_method, ipv6_method;
> struct connman_service *service;
> - int ret;
>
> if (network->connected)
> return;
>
> + connman_network_set_associating(network, false);
> +
> network->connected = true;
>
> service = connman_service_lookup_from_network(network);
> @@ -649,57 +601,8 @@ static void set_connected(struct connman_network
> *network)
> DBG("service %p ipv4 %p ipv6 %p", service, ipconfig_ipv4,
> ipconfig_ipv6);
>
> - ipv4_method = __connman_ipconfig_get_method(ipconfig_ipv4);
> - ipv6_method = __connman_ipconfig_get_method(ipconfig_ipv6);
> -
> - DBG("method ipv4 %d ipv6 %d", ipv4_method, ipv6_method);
> -
> - switch (ipv6_method) {
> - case CONNMAN_IPCONFIG_METHOD_UNKNOWN:
> - break;
> - case CONNMAN_IPCONFIG_METHOD_OFF:
> - __connman_ipconfig_disable_ipv6(ipconfig_ipv6);
> - case CONNMAN_IPCONFIG_METHOD_DHCP:
> - case CONNMAN_IPCONFIG_METHOD_AUTO:
> - autoconf_ipv6_set(network);
> - break;
> - case CONNMAN_IPCONFIG_METHOD_FIXED:
> - case CONNMAN_IPCONFIG_METHOD_MANUAL:
> - ret = manual_ipv6_set(network, ipconfig_ipv6);
> - if (ret != 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
> - return;
> - }
> - break;
> - }
> -
> - switch (ipv4_method) {
> - case CONNMAN_IPCONFIG_METHOD_UNKNOWN:
> - case CONNMAN_IPCONFIG_METHOD_OFF:
> - case CONNMAN_IPCONFIG_METHOD_AUTO:
> - return;
> - case CONNMAN_IPCONFIG_METHOD_FIXED:
> - if (set_connected_fixed(network) < 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
> - return;
> - }
> - return;
> - case CONNMAN_IPCONFIG_METHOD_MANUAL:
> - set_connected_manual(network);
> - return;
> - case CONNMAN_IPCONFIG_METHOD_DHCP:
> - if (set_connected_dhcp(network) < 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
> - return;
> - }
> - }
> -
> - network->connecting = false;
> -
> - connman_network_set_associating(network, false);
> + __connman_network_enable_ipconfig(network, ipconfig_ipv4);
> + __connman_network_enable_ipconfig(network, ipconfig_ipv6);
> }
>
> static void set_disconnected(struct connman_network *network)
> @@ -1608,26 +1511,6 @@ int __connman_network_disconnect(struct
> connman_network *network)
> return err;
> }
>
> -static int manual_ipv4_set(struct connman_network *network,
> - struct connman_ipconfig *ipconfig)
> -{
> - struct connman_service *service;
> - int err;
> -
> - service = connman_service_lookup_from_network(network);
> - if (!service)
> - return -EINVAL;
> -
> - err = __connman_ipconfig_address_add(ipconfig);
> - if (err < 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_CONFIGURE_FAIL);
> - return err;
> - }
> -
> - return __connman_ipconfig_gateway_add(ipconfig);
> -}
> -
> int __connman_network_clear_ipconfig(struct connman_network *network,
> struct connman_ipconfig *ipconfig)
> {
> @@ -1672,59 +1555,88 @@ int __connman_network_clear_ipconfig(struct
> connman_network *network,
> return 0;
> }
>
> -int __connman_network_set_ipconfig(struct connman_network *network,
> - struct connman_ipconfig *ipconfig_ipv4,
> - struct connman_ipconfig *ipconfig_ipv6)
> +int __connman_network_enable_ipconfig(struct connman_network *network,
> + struct connman_ipconfig *ipconfig)
> {
> + int r = 0;
> + enum connman_ipconfig_type type;
> enum connman_ipconfig_method method;
> - int ret;
>
> - if (!network)
> + if (!network || !ipconfig)
> return -EINVAL;
>
> - if (ipconfig_ipv6) {
> - method = __connman_ipconfig_get_method(ipconfig_ipv6);
> + type = __connman_ipconfig_get_config_type(ipconfig);
> +
> + switch (type) {
> + case CONNMAN_IPCONFIG_TYPE_UNKNOWN:
> + case CONNMAN_IPCONFIG_TYPE_ALL:
> + return -ENOSYS;
> +
> + case CONNMAN_IPCONFIG_TYPE_IPV6:
> + set_configuration(network, type);
> +
> + method = __connman_ipconfig_get_method(ipconfig);
> +
> + DBG("ipv4 ipconfig method %d", method);
Prints above and few lines below should be swapped. This print is for
ipv6 ipconfig method.
>
> switch (method) {
> case CONNMAN_IPCONFIG_METHOD_UNKNOWN:
> break;
> +
> case CONNMAN_IPCONFIG_METHOD_OFF:
> - __connman_ipconfig_disable_ipv6(ipconfig_ipv6);
> + __connman_ipconfig_disable_ipv6(ipconfig);
> + break;
> +
> case CONNMAN_IPCONFIG_METHOD_AUTO:
> autoconf_ipv6_set(network);
> break;
> +
> case CONNMAN_IPCONFIG_METHOD_FIXED:
> case CONNMAN_IPCONFIG_METHOD_MANUAL:
> - ret = manual_ipv6_set(network, ipconfig_ipv6);
> - if (ret != 0) {
> - connman_network_set_error(network,
> - CONNMAN_NETWORK_ERROR_ASSOCIATE_FAIL);
> - return ret;
> - }
> + r = manual_ipv6_set(network, ipconfig);
> break;
> +
> case CONNMAN_IPCONFIG_METHOD_DHCP:
> + r = -ENOSYS;
> break;
> }
> - }
>
> - if (ipconfig_ipv4) {
> - method = __connman_ipconfig_get_method(ipconfig_ipv4);
> + break;
> +
> + case CONNMAN_IPCONFIG_TYPE_IPV4:
> + set_configuration(network, type);
> +
> + method = __connman_ipconfig_get_method(ipconfig);
> +
> + DBG("ipv6 ipconfig method %d", method);
Ditto.
>
> switch (method) {
> case CONNMAN_IPCONFIG_METHOD_UNKNOWN:
> case CONNMAN_IPCONFIG_METHOD_OFF:
> - case CONNMAN_IPCONFIG_METHOD_FIXED:
> + break;
> +
> case CONNMAN_IPCONFIG_METHOD_AUTO:
> - return -EINVAL;
> + r = -ENOSYS;
> + break;
> +
> + case CONNMAN_IPCONFIG_METHOD_FIXED:
> case CONNMAN_IPCONFIG_METHOD_MANUAL:
> - return manual_ipv4_set(network, ipconfig_ipv4);
> + r = set_connected_manual(network);
> + break;
> +
> case CONNMAN_IPCONFIG_METHOD_DHCP:
> - return __connman_dhcp_start(ipconfig_ipv4,
> - network, dhcp_callback, NULL);
> + r = set_connected_dhcp(network);
> + break;
> }
> +
> + break;
> }
>
> - return 0;
> + if (r < 0)
> + connman_network_set_error(network,
> + CONNMAN_NETWORK_ERROR_CONFIGURE_FAIL);
> +
> + return r;
> }
>
> int connman_network_set_ipaddress(struct connman_network *network,
> diff --git a/src/service.c b/src/service.c
> index f5fb868..3ed98ed 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -3412,10 +3412,13 @@ static DBusMessage *set_property(DBusConnection *conn,
>
> if (err < 0) {
> if (is_connected_state(service, state) ||
> - is_connecting_state(service, state))
> - __connman_network_set_ipconfig(service->network,
> - service->ipconfig_ipv4,
> - service->ipconfig_ipv6);
> + is_connecting_state(service, state)) {
> +
> __connman_network_enable_ipconfig(service->network,
> + service->ipconfig_ipv4);
> +
> __connman_network_enable_ipconfig(service->network,
> + service->ipconfig_ipv6);
> + }
> +
> return __connman_error_failed(msg, -err);
> }
>
> @@ -3424,10 +3427,12 @@ static DBusMessage *set_property(DBusConnection *conn,
> else
> ipv6_configuration_changed(service);
>
> - if (is_connecting(service) || is_connected(service))
> - __connman_network_set_ipconfig(service->network,
> - service->ipconfig_ipv4,
> - service->ipconfig_ipv6);
> + if (is_connecting(service) || is_connected(service)) {
> + __connman_network_enable_ipconfig(service->network,
> + service->ipconfig_ipv4);
> + __connman_network_enable_ipconfig(service->network,
> + service->ipconfig_ipv6);
> + }
>
> service_save(service);
> } else
The daemon worked just fine in live environment and valgrind also did
not report any errors, so ACK with minor actions from me.
Cheers,
Jukka
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
