connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Jussi Laakkonen <jussi.laakkonen@jolla.com>
To: Daniel Wagner <wagi@monom.org>
Cc: connman@lists.linux.dev
Subject: Re: [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown
Date: Mon, 13 Sep 2021 12:05:47 +0300	[thread overview]
Message-ID: <6747e0f9-5c38-ac97-b759-9f69d0aa6154@jolla.com> (raw)
In-Reply-To: <20210913063231.la2wfnnm5jh65hrd@beryllium.lan>

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

  reply	other threads:[~2021-09-13  9:05 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02 15:11 [PATCH 0/5] Add heurestic and customizable value for VPN auth errors Jussi Laakkonen
2021-09-02 15:11 ` [PATCH 1/5] vpn: Report EALREADY back to caller if VPN is already disconnecting Jussi Laakkonen
2021-09-02 15:11 ` [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown Jussi Laakkonen
2021-09-13  6:32   ` Daniel Wagner
2021-09-13  9:05     ` Jussi Laakkonen [this message]
2021-09-02 15:11 ` [PATCH 3/5] vpn-provider: Add auth error check heuristic to avoid losing creds Jussi Laakkonen
2021-09-02 15:11 ` [PATCH 4/5] openvpn: Default to 10 AuthErrorLimit unless set by user Jussi Laakkonen
2021-09-02 15:11 ` [PATCH 5/5] doc: Document AuthErrorLimit in VPN connection API Jussi Laakkonen
2021-09-13  6:43 ` [PATCH 0/5] Add heurestic and customizable value for VPN auth errors Daniel Wagner
2021-09-13  9:36   ` Jussi Laakkonen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6747e0f9-5c38-ac97-b759-9f69d0aa6154@jolla.com \
    --to=jussi.laakkonen@jolla.com \
    --cc=connman@lists.linux.dev \
    --cc=wagi@monom.org \
    --subject='Re: [PATCH 2/5] vpn-provider: Ignore error adding when state is idle/unknown' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).