Hi Marcel,
On Tue, Oct 09, 2012 at 04:46:31PM +0200, Marcel Holtmann wrote:
...
> > -
> > +struct hci_chan *hci_chan_lookup_handle(struct hci_conn *hcon, __u16 handle);
> > +struct hci_chan *hci_chan_lookup_handle_all(struct hci_dev *hdev,
> > + __u16 handle);
>
> this naming is pretty bad. I have no idea what one function does
> different compared to the other. Especially since none of them take a
> hci_chan as argument, but start with that prefix.
>
> Would be the naming hci_conn_lookup_chan be a lot clearer? Or maybe
> hci_chan_lookup_from_dev or similar.
So are names like:
hci_conn_lookup_hchan_by_handle
hci_conn_lookup_hchan_from_hdev
better?
...
> > +struct hci_chan *hci_chan_lookup_handle(struct hci_conn *hcon, __u16 handle)
> > +{
> > + struct hci_chan *hchan;
> > +
> > + list_for_each_entry(hchan, &hcon->chan_list, list) {
> > + if (hchan->handle == handle)
> > + return hchan;
> > + }
> > +
> > + return NULL;
> > +}
>
> Since this function is unprotected, you better make this __hci_....
>
> And on a different note. It is not used at all. So why is this public
> anyway?
I will make it static.
...
> > + rcu_read_lock();
> > +
> > + list_for_each_entry_rcu(hcon, &h->list, list) {
> > + struct hci_chan *hchan;
> > +
> > + hchan = hci_chan_lookup_handle(hcon, handle);
> > + if (hchan) {
> > + rcu_read_unlock();
> > + return hchan;
>
> Please use break here. Have a global hchan variable assigned to NULL and
> just break here.
OK.
Best regards
Andrei Emeltchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
