Re: [tipc-discussion] [net] tipc: fix wrong connect() return code

[Tuong L. T. <tuo...@de...>]

Hi Ying,

Unfortunately, the 'tipc_wait_for_cond()' macro is not applicable here... It's for the sendmsg() cases only which expects the socket in the 'ESTABLISHED' state first, otherwise it will return '- ENOTCONN' (see the 'tipc_sk_sock_err()'). Of course, we could adjust it for the case but looks not so good..., so I have decided to keep the function and just change the wait condition to 'tipc_sk_connected()'.
Please see the patch v2 I just sent out.

BR/Tuong

-----Original Message-----
From: Ying Xue <yin...@wi...> 
Sent: Friday, December 27, 2019 2:04 PM
To: Tuong Lien Tong <tuo...@de...>; tip...@li...; jon...@er...; ma...@do...
Subject: Re: [tipc-discussion] [net] tipc: fix wrong connect() return code

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 <tuo...@de...> 
> Sent: Thursday, December 26, 2019 5:39 PM
> To: 'Ying Xue' <yin...@wi...>;
> tip...@li...; jon...@er...;
> ma...@do...
> 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 <yin...@wi...> 
> Sent: Thursday, December 26, 2019 2:55 PM
> To: Tuong Lien Tong <tuo...@de...>;
> tip...@li...; jon...@er...;
> ma...@do...
> 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
> tip...@li...
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> 
>