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

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?

>   		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?

>   
>   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