On 5/4/11 2:56 AM, Samuel Ortiz wrote:
> 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?
No, that's not a case I tested for as yet.
> In those cases, we should go back from
> ONLINE to READY, and have the portal plugin do the online check again.
Makes sense.
> 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.
I'll rerun with combined state.
Thanks for the prompt review and feedback.
Best,
Grant
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
