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

[Jussi Laakkonen <jussi.laakkonen@jolla.com>]

Hi Daniel,

On 9/13/21 9:32 AM, Daniel Wagner wrote:
> 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.
> 

Perhaps. But that would need a bit of more work to make sure the state 
machine is not getting any new issues introduced. Maybe a task for some 
later work to improve that. Unknown is not perhaps the best 
representation as a name for that but it does the trick and works, as of 
now :).

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

Yes, to cover all the cases as we may get these checks for a new VPN.

Cheers,
  Jussi