Re: [PATCH 05/45] service: Explicit VPN connect timeout, ignore in VPN agent wait

Hi Jussi,

 >>> @@ -7322,8 +7322,27 @@ static gboolean connect_timeout(gpointer user_data)
>>>       if (service->network)
>>>           __connman_network_disconnect(service->network);
>>> -    else if (service->provider)
>>> +    else if (service->provider) {
>>> +        /*
>>> +         * Remove timeout when the VPN is waiting for user input in
>>> +         * association state. By default the VPN agent timeout is
>>> +         * 300s whereas default connection timeout is 120s. Provider
>>> +         * will start connect timeout for the service when it enters
>>> +         * configuration state.
>>> +         */
>>> +        const char *statestr = connman_provider_get_string(
>>> +                    service->provider, "State");
>>> +        if (!g_strcmp0(statestr, "association")) {
>>> +            DBG("VPN provider %p is waiting for VPN agent, "
>>> +                        "stop connect timeout",
>>> +                        service->provider);
>>> +            return G_SOURCE_REMOVE;
>>> +        }
>>> +
>>
>> I'm a bit confused why this entire block is needed?  If the service is a VPN, 
>> then the timeout isn't even started, and thus connect_timeout() shouldn't even 
>> be called?
> 
> Yes and the point of this was to extend that the provider.c can initiate the 
> timeout for the VPNs as well to behave similarly to other services. So In 
> ASSOCIATION state there is the 300 timeout for inputting credentials etc. which 
> is then followed by the 120 timeout for connecting in CONFIGURATION state.

Yeah, but right now you're introducing something that is essentially dead code? 
How is this a good idea?  Introduce it in a patch where this is actually needed?

 > > I think this was done because the agent needed a bit longer timeout than the
> connection timeout is for the user to input the credentials and it seemed to 
> obey the 120 timeout instead, so the aim was to make VPNs to have same states as 
> any other service, to synchronize the behavior.
> 

Why would the core have a timeout for this at all?  NoReply timeouts are purely 
a libdbus-1 construct.  You could have a dialog sitting there forever asking 
user to input username/password prior to initiating a connection.  The driver 
should really figure out whether a timeout is needed or not.

Regards,
-Denis