Hi Grant,
On Tue, May 03, 2011 at 10:47:30AM -0700, Grant Erickson wrote:
> In the Connection Manager, completion of a valid IP configuration
> excites the service state machine to move from the "configuration" to
> the "ready" state.
>
> However, the existing implementation of IP configuration completion
> explicitly attempts to directly manipulate service state, rather than
> hinting at an excitation event.
>
> As a consequence, a late IP configuration completion after the service
> state machine has transitioned from "ready" to "online" can lead to an
> incorrect transition back to the "ready" state. This causes the
> connection count for the technology associated with that service to
> increment again, unnecessarily.
>
> This patch avoids this issue by providing a service object interface
> that simply hints that an IP configuration is complete for a given IP
> type, allowing the service object and its state machine to either hold
> fast in the present state, returning an advisory error or advancing,
> as before.
The fix looks good. But did you try the case where we actually do go from
ONLINE to READY, e.g. when switching from DHCP to static IP, or when a DHCP
renewal gives us new IP settings ? In those cases, we should go back from
ONLINE to READY, and have the portal plugin do the online check again.
If that still works with your patch, I'm fine with it (although it shows
we're starting to hit the service state machine limits...), modulo this
comment:
> +int __connman_service_set_ipconfig_ready(struct connman_service *service,
> +
> enum connman_ipconfig_type type)
> +{
> + int err = 0;
> +
> + DBG("service %p (%s) type %d (%s)",
> + service, service ? service->identifier : NULL,
> + type, __connman_ipconfig_type2string(type));
> +
> + if (service == NULL)
> + return -EINVAL;
> +
> + if (service->state_ipv4 == CONNMAN_SERVICE_STATE_READY ||
> + service->state_ipv4 == CONNMAN_SERVICE_STATE_ONLINE) {
> + err = -EALREADY;
> + } else if (service->state_ipv6 == CONNMAN_SERVICE_STATE_READY ||
> + service->state_ipv6 == CONNMAN_SERVICE_STATE_ONLINE)
> {
> + err = -EALREADY;
I think we want to check the service combine_state instead of the IPv4 one and
then the IPv6 one.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
