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

Hi Denis,

On 8/8/25 19:08, Denis Kenzior wrote:
> 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.
> 

Ok, now I remember why this was done. Both connmand and vpnd have 
separate and configurable timeouts [1,2]. And by looking at the code, 
any service started will have the connection timeout set [3].

The main reason, with adding the association state to VPN, both can be 
respected and VPNs will have same characteristics regarding timeouts as 
any other service, except that the vpnd side timeout is now respected in 
the code and these two do not overlap. It was a bit puzzling to find 
this out in the first place that why the changes to vpnd timeout value 
has no effect, as, for example, NordVPN credentials are so long and 
since our UI implementation would not bend anymore the timeout for input 
of connmand was not enough for inputting them. And since both had 
separate configuration it did not seem feasible to increase the connmand 
side because of this but make them both work, independently of each 
other. Thus, I think this addresses the issue of these two separate 
timeouts.

Granted, the __connman_service_start_connect_timeout() could have the 
checks that if (vpn && state < CONFIGURATION) ; return; or something.

This part of the patch set was already discussed with earlier maintainer 
[4] but because of reasons I had no time to do the regular, non-RFC 
patch set yet.

BR,
  Jussi

[1] 
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/doc/connman.conf.5.in#n29
[2] 
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/doc/connman-vpn.conf.5.in#n41
[3] 
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/src/service.c#n9924
[4] 
https://lore.kernel.org/connman/[email protected]/