Re: [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown

[Daniel Wagner <wagi@monom.org>]

On Thu, Sep 02, 2021 at 06:11:21PM +0300, Jussi Laakkonen wrote:
> Do not allow to add errors for provider that is already set into idle
> state or is in unknown state. This case may happen when networks are
> rapidly changed and VPN did not call the callback connect_cb() until the
> VPN is died and vpn.c:vpn_died() initiates cleanup in the VPN which
> eventually calls connect_cb() with an error.

I wonder if we should set the VPN state immediately when starting the
connection process to some value != unknown.

> ---
>  vpn/vpn-provider.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
> index f30ce04f..90e21e95 100644
> --- a/vpn/vpn-provider.c
> +++ b/vpn/vpn-provider.c
> @@ -2026,6 +2026,21 @@ int vpn_provider_indicate_error(struct vpn_provider *provider,
>  	DBG("provider %p id %s error %d", provider, provider->identifier,
>  									error);
>  
> +	/*
> +	 * Ignore adding of errors when the VPN is idle or not set. Calls may
> +	 * happen in a case when networks are rapidly changed and the call to
> +	 * vpn_died() is done before executing the connect_cb() from the
> +	 * plugin. Then vpn.c:vpn_died() executes the plugin specific died()
> +	 * function which may free the plugin private data, containing also
> +	 * the callback which hasn't yet been called. As a result the provider
> +	 * might already been reset to idle state when the callback is executed
> +	 * resulting in unnecessary reset of the previous successful connect
> +	 * timer and adding of an error for already disconnected VPN.
> +	 */
> +	if (provider->state == VPN_PROVIDER_STATE_IDLE ||
> +				provider->state == VPN_PROVIDER_STATE_UNKNOWN)
> +		return 0;

Checking the unknown state looks here a bit strange. The usual meaning
of unknown is it hasn't been initialized. But then this is exactly
what's happening here I assume.