Re: [RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Gustavo Padovan <gustavo@padovan.org>]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:35:59 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:37PM -0300, Gustavo Padovan wrote:
> >  void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >  {
> >  	struct l2cap_conn *conn = chan->conn;
> > @@ -583,12 +549,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >  
> >  	switch (chan->state) {
> >  	case BT_LISTEN:
> > -		lock_sock(sk);
> > -		l2cap_chan_cleanup_listen(sk);
> > -
> > -		__l2cap_state_change(chan, BT_CLOSED);
> > -		sock_set_flag(sk, SOCK_ZAPPED);
> > -		release_sock(sk);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> 
> Looks like you are handling all cases so this can be put outside of
> switch.

No, I'm not, look to the BT_CONNECTED/BT_CONFIG option.

> 
> >  		break;
> >  
> >  	case BT_CONNECTED:
> > @@ -630,9 +592,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >  		break;
> >  
> >  	default:
> > -		lock_sock(sk);
> > -		sock_set_flag(sk, SOCK_ZAPPED);
> > -		release_sock(sk);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> >  		break;
> >  	}
> >  }
> > @@ -3416,7 +3377,9 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
> >  
> >  	/* Check if we already have channel with that dcid */
> >  	if (__l2cap_get_chan_by_dcid(conn, scid)) {
> > -		sock_set_flag(sk, SOCK_ZAPPED);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> > +
> >  		chan->ops->close(chan->data);
> >  		goto response;
> >  	}
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 4d36605..0302cb4 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -872,6 +872,25 @@ static int l2cap_sock_release(struct socket *sock)
> >  	return err;
> >  }
> >  
> > +static void l2cap_sock_cleanup_listen(struct sock *parent)
> > +{
> > +	struct sock *sk;
> > +
> > +	BT_DBG("parent %p", parent);
> > +
> > +	/* Close not yet accepted channels */
> > +	while ((sk = bt_accept_dequeue(parent, NULL))) {
> > +		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> > +
> > +		l2cap_chan_lock(chan);
> > +		__clear_chan_timer(chan);
> > +		l2cap_chan_close(chan, ECONNRESET);
> > +		l2cap_chan_unlock(chan);
> > +
> > +		l2cap_sock_kill(sk);
> > +	}
> > +}
> > +
> >  static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
> >  {
> >  	struct sock *sk, *parent = data;
> > @@ -931,6 +950,49 @@ static void l2cap_sock_close_cb(void *data)
> >  	l2cap_sock_kill(sk);
> >  }
> >  
> > +static void l2cap_sock_finalize_cb(void *data, int err)
> > +{
> > +	struct sock *sk = data;
> 
> Why we cannot just use l2cap_sock_finalize_cb(struct sock *sk, int err)
> what is the reason for extra assignment?

Because we want to be generic here, we don't know who else will use finalize()
so we need to keep a void * here.

> 
> I also think finalize is not good name since it does not mean what is
> actually finalized.

And it is not, I failed to come with a better name here. The initial one was
delete, even worse.

	Gustavo