On 12/27/19 10:22 AM, Tuong Lien Tong wrote: > Hi Ying, > > Thinking more about this... > I suppose we can even remove the function and utilize the generic macro > 'tipc_wait_for_cond()' but with the new condition, that is much simpler and > also saves some footprint.
Agreed. > The 'tipc_sk_connected()' condition is really what we should expect in this > case instead. Yes. > So, in the case of 'TIPC_DISCONNECTING', regardless of whether we have a > 'sk->sk_err', it will return that error or '-ETIMEOUT' but never '0'. But I think it's better to return the error attached on 'sk->sk_err' to user because it can really reflect why connect() is failed. > I will send the patch v2 for your review. > > BR/Tuong > > -----Original Message----- > From: Tuong Lien Tong <[email protected]> > Sent: Thursday, December 26, 2019 5:39 PM > To: 'Ying Xue' <[email protected]>; > [email protected]; [email protected]; > [email protected] > Subject: Re: [tipc-discussion] [net] tipc: fix wrong connect() return code > > What about this? > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c > index 6ebd809ef207..04f035bcc272 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -2446,7 +2446,7 @@ static int tipc_wait_for_connect(struct socket *sock, > long *timeo_p) > done = sk_wait_event(sk, timeo_p, > sk->sk_state != TIPC_CONNECTING, > &wait); > remove_wait_queue(sk_sleep(sk), &wait); > - } while (!done); > + } while (!tipc_sk_connected(sk)); > return 0; > } > > BR/Tuong > > -----Original Message----- > From: Ying Xue <[email protected]> > Sent: Thursday, December 26, 2019 2:55 PM > To: Tuong Lien Tong <[email protected]>; > [email protected]; [email protected]; > [email protected] > Subject: Re: [net] tipc: fix wrong connect() return code > > On 12/26/19 3:16 PM, Tuong Lien Tong wrote: >> Sorry, my mistake! I thought it was "while (!done || !sk->sk_err)"... >> But, this is really confusing and one might ask why we continue the loop > while the socket has encountered an error (sk-> sk_err! = 0)??? > > This is totally reasonable because it can make code simpler. > [Tuong]: but a performance hit since it almost looks like the sk_err has to > be read twice? > >> Moreover, it's not necessarily the "sk_err" will be the only case, >> For example: >> sk->sk_state != TIPC_CONNECTING but sk->sk_err = 0 (somehow) and a > pending/interrupt signal > > If sk->sk_state is changed from TIPC_CONNECTING to TIPC_ESTABLISHED, > sk->sk_err should be kept 0, but if sk->sk_state is changed to other > states, sk->sk_err must be set with a proper error code in socket > receive path, otherwise, it's a bug. > > However, there might be one below race condition: > > sk->sk_state is set to TIPC_DISCONNECTING, but before sk->sk_err is set > with an error code, the process who calls connect() is woken up by one > single. Even so, the process is still blocked when it tries to hold sock > lock with lock_sock() in sk_wait_event() because the socket lock is > taken in socket receive path. After socket lock is released, the process > will continue being run. But at that moment, sk->sk_err has been set > with an error code. So, this is no problem for us. > >> >> then we should return '-EINTR'? > > > > _______________________________________________ > tipc-discussion mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/tipc-discussion > > _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
