> -----Original Message-----
> From: Ying Xue <[email protected]>
> Sent: September 28, 2018 7:26 AM
> To: Jon Maloy <[email protected]>; Jon Maloy
> <[email protected]>
> Cc: Mohan Krishna Ghanta Krishnamurthy
> <[email protected]>; Tung Quang Nguyen
> <[email protected]>; Hoang Huu Le
> <[email protected]>; Canh Duc Luu
> <[email protected]>; Gordan Mihaljevic
> <[email protected]>;
> [email protected]; tipc-
> [email protected]
> Subject: Re: [net-next 3/5] tipc: refactor function tipc_sk_filter_connect()
>
> On 09/25/2018 07:34 AM, Jon Maloy wrote:
> > We refactor the function tipc_sk_filter_connect(), both to make it
> > more readable and as a preparation for the next commit.
> >
> > Signed-off-by: Jon Maloy <[email protected]>
> > ---
> > net/tipc/socket.c | 101
> > +++++++++++++++++++++++-------------------------------
> > 1 file changed, 43 insertions(+), 58 deletions(-)
> >
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c index
> > 4c50cff..14e6471 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -1959,91 +1959,76 @@ static void tipc_sk_proto_rcv(struct sock *sk,
> > }
> >
> > /**
> > - * tipc_filter_connect - Handle incoming message for a
> > connection-based socket
> > + * tipc_sk_filter_connect - check incoming message for a
> > + connection-based socket
> > * @tsk: TIPC socket
> > - * @skb: pointer to message buffer. Set to NULL if buffer is consumed
> > - *
> > - * Returns true if everything ok, false otherwise
> > + * @skb: pointer to message buffer.
> > + * Returns true if message should be added to receive queue, false
> > + otherwise
> > */
> > static bool tipc_sk_filter_connect(struct tipc_sock *tsk, struct
> > sk_buff *skb) {
> > struct sock *sk = &tsk->sk;
> > struct net *net = sock_net(sk);
> > struct tipc_msg *hdr = buf_msg(skb);
> > - u32 pport = msg_origport(hdr);
> > - u32 pnode = msg_orignode(hdr);
> > + bool con_msg = msg_connected(hdr);
> > + u32 pport = tsk_peer_port(tsk);
> > + u32 pnode = tsk_peer_node(tsk);
> > + u32 oport = msg_origport(hdr);
> > + u32 onode = msg_orignode(hdr);
> > + int err = msg_errcode(hdr);
> >
> > if (unlikely(msg_mcast(hdr)))
> > return false;
> >
> > switch (sk->sk_state) {
> > case TIPC_CONNECTING:
> > - /* Accept only ACK or NACK message */
> > - if (unlikely(!msg_connected(hdr))) {
> > - if (pport != tsk_peer_port(tsk) ||
> > - pnode != tsk_peer_node(tsk))
> > - return false;
> > -
> > - tipc_set_sk_state(sk, TIPC_DISCONNECTING);
> > - sk->sk_err = ECONNREFUSED;
> > - sk->sk_state_change(sk);
> > - return true;
> > - }
> > -
> > - if (unlikely(msg_errcode(hdr))) {
> > - tipc_set_sk_state(sk, TIPC_DISCONNECTING);
> > - sk->sk_err = ECONNREFUSED;
> > - sk->sk_state_change(sk);
> > - return true;
> > - }
> > -
> > - if (unlikely(!msg_isdata(hdr))) {
> > - tipc_set_sk_state(sk, TIPC_DISCONNECTING);
> > - sk->sk_err = EINVAL;
> > - sk->sk_state_change(sk);
> > - return true;
> > + /* Setup ACK */
> > + if (likely(con_msg)) {
> > + if (err)
> > + break;
> > + tipc_sk_finish_conn(tsk, oport, onode);
> > + msg_set_importance(&tsk->phdr,
> msg_importance(hdr));
> > + /* ACK+ message with data is added to receive
> queue */
> > + if (msg_data_sz(hdr))
> > + return true;
> > + /* Empty ACK-, - wake up sleeping connect() and
> drop */
> > + sk->sk_data_ready(sk);
> > + msg_set_dest_droppable(hdr, 1);
> > + return false;
> > }
> > + /* Ignore connectionless message if not from listening socket
> */
> > + if (oport != pport || onode != pnode)
> > + return false;
> >
> > - tipc_sk_finish_conn(tsk, msg_origport(hdr),
> msg_orignode(hdr));
> > - msg_set_importance(&tsk->phdr, msg_importance(hdr));
> > -
> > - /* If 'ACK+' message, add to socket receive queue */
> > - if (msg_data_sz(hdr))
> > - return true;
> > -
> > - /* If empty 'ACK-' message, wake up sleeping connect() */
> > - sk->sk_data_ready(sk);
> > -
> > - /* 'ACK-' message is neither accepted nor rejected: */
> > - msg_set_dest_droppable(hdr, 1);
> > - return false;
> > -
> > + /* Rejected SYN - abort */
> > + break;
> > case TIPC_OPEN:
> > case TIPC_DISCONNECTING:
> > - break;
> > + return false;
> > case TIPC_LISTEN:
> > /* Accept only SYN message */
> > - if (!msg_connected(hdr) && !(msg_errcode(hdr)))
> > + if (!con_msg && !err)
> > return true;
> > - break;
> > + return false;
> > case TIPC_ESTABLISHED:
> > /* Accept only connection-based messages sent by peer */
> > - if (unlikely(!tsk_peer_msg(tsk, hdr)))
> > + if (likely(con_msg && !err && pport == oport && pnode ==
> onode))
> > + return true;
> > + if (!tsk_peer_msg(tsk, hdr))
> > return false;
> > -
> > - if (unlikely(msg_errcode(hdr))) {
> > - tipc_set_sk_state(sk, TIPC_DISCONNECTING);
> > - /* Let timer expire on it's own */
> > - tipc_node_remove_conn(net, tsk_peer_node(tsk),
> > - tsk->portid);
> > - sk->sk_state_change(sk);
> > - }
> > + if (!err)
> > + return true;
> > + tipc_set_sk_state(sk, TIPC_DISCONNECTING);
>
> It's better to set a error code to sk->sk_err.
I tried it, and it broke the test program, which is expecting the current
bahavior. There is no doubt this would be the right thing to do if we had done
it from the beginning, but now it is too late I am afraid.
//jon
>
> > + tipc_node_remove_conn(net, pnode, tsk->portid);
> > + sk->sk_state_change(sk);
> > return true;
> > default:
> > pr_err("Unknown sk_state %u\n", sk->sk_state);
> > }
> > -
> > - return false;
> > + /* Abort connection setup attempt */
> > + tipc_set_sk_state(sk, TIPC_DISCONNECTING);
> > + sk->sk_err = ECONNREFUSED;
> > + sk->sk_state_change(sk);
> > + return true;
> > }
> >
> > /**
> >
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
