Re: [PATCH 3/6] Phonet: Pipe End Point for Phonet Pipes protocol

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Thu, Oct 02, 2008 at 01:50:52PM +0300, Rémi Denis-Courmont escreveu:
> On Wednesday 01 October 2008 16:18:56 ext Arnaldo Carvalho de Melo, you wrote:
> > > +static struct sock *pep_find_pipe(const struct hlist_head *hlist,
> > > +					const struct sockaddr_pn *dst,
> > > +					u8 pipe_handle)
> > > +{
> > > +	struct hlist_node *node;
> > > +	struct sock *sknode;
> > > +	u16 dobj = pn_sockaddr_get_object(dst);
> >
> > What is the lock that protects this list traversal?
> 
> Either (accepted or unaccepted) sock lists should only be used with the sock 
> lock of the listening sock. Is that insufficient?
> 
> > > +static int pep_wait_connreq(struct sock *sk, int noblock)
> >
> > This function looks familiar... inet_csk_accept,
> > inet_csk_wait_for_connect...
> 
> I don't recall why I gave up on using request_sock and listen_sock there.
> 
> > perhaps we need a connection_sock father 
> > for inet_connection_sock? :-)
> 
> But you cannot have double inheritance, right? inet_sock and 
> connection_sock... I guess that's why listen_sock is _not_ a sock.

I'm not saying that you could use in its current form, only that it
looks as something to think about after you get phonet merged and
polished.
 
> > > +{
> > > +	struct task_struct *tsk = current;
> > > +	struct pep_sock *pn = pep_sk(sk);
> > > +	long timeo = sock_rcvtimeo(sk, noblock);
> > > +
> > > +	for (;;) {
> > > +		DEFINE_WAIT(wait);
> > > +
> > > +		if (sk->sk_state != TCP_LISTEN)
> > > +			return -EINVAL;
> > > +		if (!hlist_empty(&pn->ackq))
> > > +			break;
> > > +		if (!timeo)
> > > +			return -EWOULDBLOCK;
> > > +		if (signal_pending(tsk))
> > > +			return sock_intr_errno(timeo);
> > > +
> > > +		prepare_to_wait_exclusive(&sk->sk_socket->wait, &wait,
> > > +						TASK_INTERRUPTIBLE);
> > > +		release_sock(sk);
> > > +		timeo = schedule_timeout(timeo);
> > > +		lock_sock(sk);
> > > +		finish_wait(&sk->sk_socket->wait, &wait);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct sock *pep_sock_accept(struct sock *sk, int flags, int
> > > *errp) +{
> > > +	struct pep_sock *pn = pep_sk(sk);
> > > +	struct sock *newsk = NULL;
> > > +	struct sk_buff *oskb;
> > > +	int err;
> > > +
> > > +	lock_sock(sk);
> > > +	err = pep_wait_connreq(sk, flags & O_NONBLOCK);
> > > +	if (err)
> > > +		goto out;
> > > +
> > > +	newsk = __sk_head(&pn->ackq);
> > > +
> > > +	oskb = skb_dequeue(&newsk->sk_receive_queue);
> > > +	err = pep_accept_conn(newsk, oskb);
> > > +	if (err) {
> > > +		skb_queue_head(&newsk->sk_receive_queue, oskb);
> > > +		newsk = NULL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	sock_hold(sk);
> > > +	pep_sk(newsk)->listener = sk;
> > > +
> > > +	sock_hold(newsk);
> > > +	sk_del_node_init(newsk);
> > > +	sk_acceptq_removed(sk);
> > > +	sk_add_node(newsk, &pn->hlist);
> > > +	__sock_put(newsk);
> > > +
> > > +out:
> > > +	release_sock(sk);
> > > +	*errp = err;
> > > +	return newsk;
> > > +}
> > > +
> > > +static int pep_ioctl(struct sock *sk, int cmd, unsigned long arg)
> > > +{
> > > +	int answ;
> > > +
> > > +	switch (cmd) {
> > > +	case SIOCINQ:
> > > +		if (sk->sk_state == TCP_LISTEN)
> > > +			return -EINVAL;
> > > +
> > > +		lock_sock(sk);
> > > +		if (!skb_queue_empty(&sk->sk_receive_queue))
> > > +			answ = skb_peek(&sk->sk_receive_queue)->len;
> > > +		else
> > > +			answ = 0;
> > > +		release_sock(sk);
> > > +		return put_user(answ, (int __user *)arg);
> >
> > this is so common I wonder we if a helper wouldn't help 8) Look at
> > dccp_ioctl before Ilpo does 8)
> 
> It is not so common with the next patch which checks for urgent inline... As 
> far as I know, there is no common queue for out-of-band data.

I saw that, but out-of-band data is also something up for study wheter
introductin common infrastructure is feasible.

- Arnaldo