Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [RFC] service: Move service to state ready if
defaultroute is removed (Naveen Singh)
2. [TEST 2/4] connection: Use the sorted service list to find
the default gateway (Patrik Flykt)
3. [TEST 3/4] connection: Use services to compare gateway
ordering (Patrik Flykt)
4. [TEST 0/4] Clean up gateway selection (Patrik Flykt)
5. [TEST 1/4] service: Add function to compare the order between
two services (Patrik Flykt)
6. [TEST 4/4] connection: Make tests to ensure correct gateway
selection (Patrik Flykt)
----------------------------------------------------------------------
Message: 1
Date: Wed, 13 Jan 2016 15:39:32 -0800
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: [RFC] service: Move service to state ready if
defaultroute is removed
Message-ID:
<CAGTDzK=ppiypz96bdrtvcs94mrnb+wptyhk4v00bawv_b+r...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
On Wed, Jan 13, 2016 at 4:57 AM, Patrik Flykt <[email protected]>
wrote:
> When the default route is removed, it is more than likely that the
> corresponding service cannot be in state online anymore. Downgrade
> the affected service state. This in turn will trigger a new online
> check and resolve the state back to online if at all possible.
> ---
>
> This might fix the issue with dhcp_failure() reported by Naveen Singh.
>
> Please test!
>
> Patrik
>
>
> src/service.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/service.c b/src/service.c
> index abf0899..3fa5ae5 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -6380,12 +6380,27 @@ static void service_ip_release(struct
> connman_ipconfig *ipconfig,
> settings_changed(service, ipconfig);
> }
>
> -static void service_route_changed(struct connman_ipconfig *ipconfig,
> +static void service_defaultroute_set(struct connman_ipconfig *ipconfig,
> const char *ifname)
> {
> struct connman_service *service =
> __connman_ipconfig_get_data(ipconfig);
>
> - DBG("%s route changed", ifname);
> + DBG("service %p %s defaultroute set", service, ifname);
> +
> + settings_changed(service, ipconfig);
> +}
> +
> +static void service_defaultroute_unset(struct connman_ipconfig *ipconfig,
> + const char *ifname)
> +{
> + struct connman_service *service =
> __connman_ipconfig_get_data(ipconfig);
> +
> + DBG("service %p %s defaultroute unset", service, ifname);
> +
> + if (service->state == CONNMAN_SERVICE_STATE_ONLINE)
> + __connman_service_ipconfig_indicate_state(service,
> + CONNMAN_SERVICE_STATE_READY,
> +
> __connman_ipconfig_get_config_type(ipconfig));
>
When the default route is removed why we are changing the state to READY.
Should not it be CONFIGURE?
>
> settings_changed(service, ipconfig);
> }
> @@ -6397,8 +6412,8 @@ static const struct connman_ipconfig_ops service_ops
> = {
> .lower_down = service_lower_down,
> .ip_bound = service_ip_bound,
> .ip_release = service_ip_release,
> - .route_set = service_route_changed,
> - .route_unset = service_route_changed,
> + .route_set = service_defaultroute_set,
> + .route_unset = service_defaultroute_unset,
> };
>
> static struct connman_ipconfig *create_ip4config(struct connman_service
> *service,
>
In stead of changing this why do not we use service_ip_bound and
service_ip_release to do the specific state transitions. These functions
are the entry point. Right now the service state transition to READY is
happening from __connman_connection_gateway_add which we should remove.
This is the sequence that I am proposing:
1) In service_ip_bound transition from configuration to ready
2) In service_ip_release transition from ready to configuration
There is a small check we need to perform specially when we are going from
169 address to (DHCP) address. We would like to release the link local IP
first if it is currently in the link local state and then set the IP. This
would allow service_ip_release to be called first and then
service_ip_bound.
Regards
Naveen
--
> 2.1.4
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20160113/3f4b9bd6/attachment-0001.html>
------------------------------
Message: 2
Date: Thu, 14 Jan 2016 10:35:45 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [TEST 2/4] connection: Use the sorted service list to find
the default gateway
Message-ID:
<[email protected]>
The service list is always sorted and thus the service that should have
the default gateway is first in the list. The first service in the list
has coincidentally also the largest 'order' value.
---
src/connection.c | 22 +++++-----------------
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/src/connection.c b/src/connection.c
index aa4e1c0..fbd0448 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -558,25 +558,13 @@ static void unset_default_gateway(struct gateway_data
*data,
static struct gateway_data *find_default_gateway(void)
{
- struct gateway_data *found = NULL;
- unsigned int order = 0;
- GHashTableIter iter;
- gpointer value, key;
-
- g_hash_table_iter_init(&iter, gateway_hash);
-
- while (g_hash_table_iter_next(&iter, &key, &value)) {
- struct gateway_data *data = value;
-
- if (!found || data->order > order) {
- found = data;
- order = data->order;
+ struct connman_service *service;
- DBG("default %p order %d", found, order);
- }
- }
+ service = __connman_service_get_default();
+ if (!service)
+ return NULL;
- return found;
+ return g_hash_table_lookup(gateway_hash, service);
}
static bool choose_default_gateway(struct gateway_data *data,
--
2.1.4
------------------------------
Message: 3
Date: Thu, 14 Jan 2016 10:35:46 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [TEST 3/4] connection: Use services to compare gateway
ordering
Message-ID:
<[email protected]>
Use the service pointer in the gateway data to find out if the candidate
gateway should be chosen over the other gateway.
---
src/connection.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/connection.c b/src/connection.c
index fbd0448..4eb5566 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -577,37 +577,35 @@ static bool choose_default_gateway(struct gateway_data
*data,
* this one as default. If the other one is already active
* we mark this one as non default.
*/
- if (data->ipv4_gateway) {
- if (candidate->ipv4_gateway &&
- !candidate->ipv4_gateway->active) {
+ if (data->ipv4_gateway && candidate->ipv4_gateway) {
+
+ if (!candidate->ipv4_gateway->active) {
DBG("ipv4 downgrading %p", candidate);
unset_default_gateway(candidate,
CONNMAN_IPCONFIG_TYPE_IPV4);
}
- if (candidate->ipv4_gateway &&
- candidate->ipv4_gateway->active &&
- candidate->order > data->order) {
+
+ if (candidate->ipv4_gateway->active &&
+ __connman_service_compare(candidate->service,
+ data->service) < 0) {
DBG("ipv4 downgrading this %p", data);
- unset_default_gateway(data,
- CONNMAN_IPCONFIG_TYPE_IPV4);
+ unset_default_gateway(data, CONNMAN_IPCONFIG_TYPE_IPV4);
downgraded = true;
}
}
- if (data->ipv6_gateway) {
- if (candidate->ipv6_gateway &&
- !candidate->ipv6_gateway->active) {
+ if (data->ipv6_gateway && candidate->ipv6_gateway) {
+ if (!candidate->ipv6_gateway->active) {
DBG("ipv6 downgrading %p", candidate);
unset_default_gateway(candidate,
CONNMAN_IPCONFIG_TYPE_IPV6);
}
- if (candidate->ipv6_gateway &&
- candidate->ipv6_gateway->active &&
- candidate->order > data->order) {
+ if (candidate->ipv6_gateway->active &&
+ __connman_service_compare(candidate->service,
+ data->service) < 0) {
DBG("ipv6 downgrading this %p", data);
- unset_default_gateway(data,
- CONNMAN_IPCONFIG_TYPE_IPV6);
+ unset_default_gateway(data, CONNMAN_IPCONFIG_TYPE_IPV6);
downgraded = true;
}
}
--
2.1.4
------------------------------
Message: 4
Date: Thu, 14 Jan 2016 10:35:43 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [TEST 0/4] Clean up gateway selection
Message-ID:
<[email protected]>
Hi,
In order to clean up gateway selection, the gateway code should use the
sorted list of services instead of sorting the gateways by itself.
The 'order' value in the gateway_data structure follows the service list,
i.e. the default service gets an 'order' value of 1, a VPN without split
routing a value of 10 and the rest a value of 0. This means the gateway
'order' variable gets its values so that the sorting order is the same
between gateways and services in the service list. With that, there are
now two entities that are being sorted, which in turn has caused code
duplication and a non-trivial implementation to decipher for the gateway
selection.
To change the current behavior, a function comparing the ordering between
two services is introduced in patch #1. This function is then used in
patch #3, throwing out the old code as a result. Patch #3 also combines
consecutive if statements.
Patch #2 gets the default service and uses that to look up the corresponding
gateway_data structure instead of iterating over the hash table.
Patch #4 is important, it restores the old functionality so that tests
can be performed on the newly introduced behavior wrt the old one. Should
they differ, a warning is printed out.
After these steps, it is possible to remove the 'order' variable from the
gateway_data struct, I will send the full patch set doing that shortly.
If you are interested in the well-being of ConnMan and apply this patch
set, you MUST report any warnings printed out by these changes to the
mailing list! One thing that we cannot afford is to introduce funny bugs
due to a modification like this.
Cheers,
Patrik
Patrik Flykt (4):
service: Add function to compare the order between two services
connection: Use the sorted service list to find the default gateway
connection: Use services to compare gateway ordering
connection: Make tests to ensure correct gateway selection
src/connection.c | 70 ++++++++++++++++++++++++++++++++++++++++++--------------
src/connman.h | 3 +++
src/service.c | 6 +++++
3 files changed, 62 insertions(+), 17 deletions(-)
--
2.1.4
------------------------------
Message: 5
Date: Thu, 14 Jan 2016 10:35:44 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [TEST 1/4] service: Add function to compare the order between
two services
Message-ID:
<[email protected]>
This function compares the order between two services and returns
-1, 0 or 1 respectively.
---
src/connman.h | 3 +++
src/service.c | 6 ++++++
2 files changed, 9 insertions(+)
diff --git a/src/connman.h b/src/connman.h
index 35eb3f5..e41d5bf 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -656,6 +656,9 @@ int __connman_service_load_modifiable(struct
connman_service *service);
void __connman_service_list_struct(DBusMessageIter *iter);
+int __connman_service_compare(struct connman_service *a,
+ struct connman_service *b);
+
struct connman_service *__connman_service_lookup_from_index(int index);
struct connman_service *__connman_service_lookup_from_ident(const char
*identifier);
struct connman_service *__connman_service_create_from_network(struct
connman_network *network);
diff --git a/src/service.c b/src/service.c
index abf0899..030b353 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4785,6 +4785,12 @@ static void service_list_sort(void)
}
}
+int __connman_service_compare(struct connman_service *a,
+ struct connman_service *b)
+{
+ return service_compare(a, b);
+}
+
/**
* connman_service_get_type:
* @service: service structure
--
2.1.4
------------------------------
Message: 6
Date: Thu, 14 Jan 2016 10:35:47 +0200
From: Patrik Flykt <[email protected]>
To: [email protected]
Subject: [TEST 4/4] connection: Make tests to ensure correct gateway
selection
Message-ID:
<[email protected]>
As gateway selection is a critical piece of ConnMan, this patch restores
old functionality and compares its results with the new code. If there are
any differences, a warning will be pritnted out.
---
src/connection.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/src/connection.c b/src/connection.c
index 4eb5566..717011d 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -556,14 +556,44 @@ static void unset_default_gateway(struct gateway_data
*data,
data->ipv4_gateway->gateway);
}
+static struct gateway_data *find_default_gateway_old(void)
+{
+ struct gateway_data *found = NULL;
+ unsigned int order = 0;
+ GHashTableIter iter;
+ gpointer value, key;
+
+ g_hash_table_iter_init(&iter, gateway_hash);
+
+ while (g_hash_table_iter_next(&iter, &key, &value)) {
+ struct gateway_data *data = value;
+
+ if (!found || data->order > order) {
+ found = data;
+ order = data->order;
+
+ DBG("default %p order %d", found, order);
+ }
+ }
+
+ return found;
+}
+
static struct gateway_data *find_default_gateway(void)
{
struct connman_service *service;
+ struct gateway_data *new_gw_result, *old_gw_result;
service = __connman_service_get_default();
if (!service)
return NULL;
+ new_gw_result = g_hash_table_lookup(gateway_hash, service);
+ old_gw_result = find_default_gateway_old();
+ if (new_gw_result != old_gw_result)
+ connman_warn("Gateway differences new %p old %p",
+ new_gw_result, old_gw_result);
+
return g_hash_table_lookup(gateway_hash, service);
}
@@ -586,6 +616,16 @@ static bool choose_default_gateway(struct gateway_data
*data,
}
if (candidate->ipv4_gateway->active &&
+ candidate->order > data->order &&
+ __connman_service_compare(candidate->service,
+ data->service) != -1)
+ connman_warn("Order different new %d old %s than",
+ __connman_service_compare(candidate->service,
+ data->service),
+ candidate->order > data->order ?
+ "greater": "smaller or equal");
+
+ if (candidate->ipv4_gateway->active &&
__connman_service_compare(candidate->service,
data->service) < 0) {
DBG("ipv4 downgrading this %p", data);
@@ -602,6 +642,16 @@ static bool choose_default_gateway(struct gateway_data
*data,
}
if (candidate->ipv6_gateway->active &&
+ candidate->order > data->order &&
+ __connman_service_compare(candidate->service,
+ data->service) != -1)
+ connman_warn("Order different new %d old %s than",
+ __connman_service_compare(candidate->service,
+ data->service),
+ candidate->order > data->order ?
+ "greater": "smaller or equal");
+
+ if (candidate->ipv6_gateway->active &&
__connman_service_compare(candidate->service,
data->service) < 0) {
DBG("ipv6 downgrading this %p", data);
--
2.1.4
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 3, Issue 9
*************************************
