Re: [PATCH] Phonet: 'connect' socket implementation for Pipe controller

["Rémi Denis-Courmont" <remi@remlab.net>]

	Hello,

Just a few comments...

Le mardi 12 octobre 2010 18:26:51 Kumar A Sanghvi, vous avez écrit :
> diff --git a/include/net/phonet/pep.h b/include/net/phonet/pep.h
> index def6cfa..b60b28c 100644
> --- a/include/net/phonet/pep.h
> +++ b/include/net/phonet/pep.h
> @@ -802,6 +682,42 @@ static void pipe_destruct(struct sock *sk)
>  	skb_queue_purge(&pn->ctrlreq_queue);
>  }
> 
> +#ifdef CONFIG_PHONET_PIPECTRLR
> +static int pep_connresp_rcv(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct pep_sock *pn = pep_sk(sk);
> +	static u8 host_pref_rx_fc[3] = {3, 2, 1}, host_req_tx_fc[3] = {3, 2, 1};

Why is this 'static' ? Doesn't that break concurrent uses?

> +	u8 remote_pref_rx_fc[3], remote_req_tx_fc[3];
> +	u8 negotiated_rx_fc, negotiated_tx_fc;
> +	int ret;
> +
> +	pipe_get_flow_info(sk, skb, remote_pref_rx_fc,
> +			remote_req_tx_fc);
> +	negotiated_tx_fc = pipe_negotiate_fc(remote_req_tx_fc,
> +			host_pref_rx_fc,
> +			sizeof(host_pref_rx_fc));
> +	negotiated_rx_fc = pipe_negotiate_fc(host_req_tx_fc,
> +			remote_pref_rx_fc,
> +			sizeof(host_pref_rx_fc));
> +
> +	pn->pipe_state = PIPE_DISABLED;
> +	sk->sk_state = TCP_SYN_RECV;
> +	sk->sk_backlog_rcv = pipe_do_rcv;
> +	sk->sk_destruct = pipe_destruct;
> +	pn->rx_credits = 0;
> +	pn->rx_fc = negotiated_rx_fc;
> +	pn->tx_fc = negotiated_tx_fc;
> +	sk->sk_state_change(sk);
> +
> +	ret = pipe_handler_send_created_ind(sk,
> +			PNS_PIPE_CREATED_IND_UTID,
> +			PNS_PIPE_CREATED_IND
> +			);
> +
> +	return ret;
> +}
> +#endif
> +
>  static int pep_connreq_rcv(struct sock *sk, struct sk_buff *skb)
>  {
>  	struct sock *newsk;

> diff --git a/net/phonet/socket.c b/net/phonet/socket.c
> index aca8fba..123a374 100644
> --- a/net/phonet/socket.c
> +++ b/net/phonet/socket.c
> @@ -225,6 +225,102 @@ static int pn_socket_autobind(struct socket *sock)
>  	return 0; /* socket was already bound */
>  }
> 
> +static int pn_socket_connect(struct socket *sock, struct sockaddr *addr,
> +		int len, int flags)
> +{
> +	struct sock *sk = sock->sk;
> +	struct sockaddr_pn *spn = (struct sockaddr_pn *)addr;
> +	long timeo;
> +	int err;
> +
> +	lock_sock(sk);
> +
> +	if (len < sizeof(struct sockaddr_pn))
> +		return -EINVAL;
> +	if (spn->spn_family != AF_PHONET)
> +		return -EAFNOSUPPORT;

You should move lock_sock(sk); here, I think.

> +
> +	switch (sock->state) {
> +	case SS_UNCONNECTED:
> +		sk->sk_state = TCP_CLOSE;
> +		break;
> +	case SS_CONNECTING:
> +		switch (sk->sk_state) {
> +		case TCP_SYN_RECV:
> +			sock->state = SS_CONNECTED;
> +			err = -EISCONN;
> +			goto out;
> +		case TCP_CLOSE:
> +			err = -EALREADY;
> +			if (flags & O_NONBLOCK)
> +				goto out;
> +			goto wait_connect;
> +			break;

I think the kernel policy is against redumdant break statements.

> +		}
> +		break;
> +	case SS_CONNECTED:
> +		switch (sk->sk_state) {
> +		case TCP_SYN_RECV:
> +			err = -EISCONN;
> +			goto out;
> +		case TCP_CLOSE:
> +			sock->state = SS_UNCONNECTED;
> +			break;
> +		}
> +		break;
> +	case SS_DISCONNECTING:
> +	case SS_FREE:
> +		break;
> +	}
> +	sk->sk_state = TCP_CLOSE;
> +	sock->state = SS_UNCONNECTED;

This is dead code...

> +	sk_stream_kill_queues(sk);
> +
> +
> +	sock->state = SS_CONNECTING;

...because of this ^ .

> +	err = sk->sk_prot->connect(sk, addr, len);
> +	if (err < 0) {
> +		sock->state = SS_UNCONNECTED;
> +		sk->sk_state = TCP_CLOSE;
> +		goto out;
> +	}
> +
> +	err = -EINPROGRESS;
> +wait_connect:
> +	if (sk->sk_state != TCP_SYN_RECV && (flags & O_NONBLOCK))
> +		goto out;
> +
> +	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> +	release_sock(sk);
> +
> +	err = -ERESTARTSYS;
> +	timeo = wait_event_interruptible_timeout(*sk_sleep(sk),
> +			sk->sk_state != TCP_CLOSE,
> +			timeo);
> +
> +	lock_sock(sk);
> +	if (timeo < 0)
> +		goto out; /* -ERESTARTSYS */
> +
> +	err = -ETIMEDOUT;
> +	if (timeo == 0 && sk->sk_state != TCP_SYN_RECV)
> +		goto out;
> +
> +	if (sk->sk_state != TCP_SYN_RECV) {
> +		sock->state = SS_UNCONNECTED;
> +		err = sock_error(sk);
> +		if (!err)
> +			err = -ECONNREFUSED;
> +		goto out;
> +	}
> +	sock->state = SS_CONNECTED;
> +	err = 0;
> +
> +out:
> +	release_sock(sk);
> +	return err;
> +}
> +
>  static int pn_socket_accept(struct socket *sock, struct socket *newsock,
>  				int flags)
>  {


-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis