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

Hi Denis,

I'll try to remember this from 3 years back when this was done.

On 8/1/25 22:31, Denis Kenzior wrote:
> Hi Jussi,
> 
> On 7/11/25 9:26 AM, Jussi Laakkonen wrote:
>> Ignore the connect timeout autostarting when connecting a VPN service
>> because initially the VPN is in association state in which the VPN is
>> waiting for the VPN agent. Separate the starting of connect timeout into
>> its own function __connman_service_start_connect_timeout() so provider.c
>> can call it when it enters configuration state.
>>
>> When a VPN is waiting for user input it should not be affected by
>> connect timeout as the connection is not yet attempted. This may happen
>> if VPN resumes to association state when requiring the VPN agent for
>> other, e.g., encrypted private key input after credential input.
>> ---
>>   src/connman.h |  2 ++
>>   src/service.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 49 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/connman.h b/src/connman.h
>> index 7ebda7af..1a8952e4 100644
>> --- a/src/connman.h
>> +++ b/src/connman.h
>> @@ -788,6 +788,8 @@ int __connman_service_connect(struct 
>> connman_service *service,
>>   int __connman_service_disconnect(struct connman_service *service);
>>   void __connman_service_set_active_session(bool enable, GSList *list);
>>   void __connman_service_auto_connect(enum 
>> connman_service_connect_reason reason);
>> +void __connman_service_start_connect_timeout(struct connman_service 
>> *service,
>> +                bool restart);
>>   bool __connman_service_remove(struct connman_service *service);
>>   void __connman_service_set_hidden_data(struct connman_service *service,
>>                   gpointer user_data);
>> diff --git a/src/service.c b/src/service.c
>> index f73f42eb..9fd8fdb1 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -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.

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.

> 
>>           connman_provider_disconnect(service->provider);
>> +    }
>> +
>> +
>>       __connman_stats_service_unregister(service);
>> @@ -7351,7 +7370,27 @@ static gboolean connect_timeout(gpointer 
>> user_data)
>>                   CONNMAN_SERVICE_CONNECT_REASON_USER)
>>           do_auto_connect(service, CONNMAN_SERVICE_CONNECT_REASON_AUTO);
>> -    return FALSE;
>> +    return G_SOURCE_REMOVE;
>> +}
>> +
>> +void __connman_service_start_connect_timeout(struct connman_service 
>> *service,
>> +                                bool restart)
>> +{
>> +    DBG("");
>> +
>> +    if (!service)
>> +        return;
>> +
>> +    if (!restart && service->timeout)
>> +        return;
>> +
>> +    if (restart && service->timeout) {
>> +        DBG("cancel running connect timeout");
>> +        g_source_remove(service->timeout);
>> +    }
>> +
>> +    service->timeout = g_timeout_add_seconds(CONNECT_TIMEOUT,
>> +                connect_timeout, service);
>>   }
> 
> Given the above, I question the need for this function?

This is needed for keeping the behavior of services in sync, not to 
separate VPNs.

> 
>>   static DBusMessage *connect_service(DBusConnection *conn,
>> @@ -9975,9 +10014,12 @@ int __connman_service_connect(struct 
>> connman_service *service,
>>           return 0;
>>       if (err == -EINPROGRESS) {
>> -        if (service->timeout == 0)
>> -            service->timeout = g_timeout_add_seconds(
>> -                CONNECT_TIMEOUT, connect_timeout, service);
>> +        /*
>> +         * VPN will start connect timeout when it enters CONFIGURATION
>> +         * state.
>> +         */
>> +        if (service->type != CONNMAN_SERVICE_TYPE_VPN)
>> +            __connman_service_start_connect_timeout(service, false);
>>           return -EINPROGRESS;
>>       }
> 
> Regards,
> -Denis

BR,
  Jussi