_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel
> The biggest thing however is hci_sched_sco(). This function is ugly as
> hell and basically unreadable. Please use break or continue instead of
> cascaded if clauses.
ok I will work on it and submit another patch this week.
Brad
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Brad,
> per our conversation about this, I have reworked the sco flowcontrol
> patch against the linus git tree (2.6.21rc2) and successfully tested it.
> Most of the comments are out so you can more easily see what is
> changing. I left about 3 comments in that are parts of new blocks of code.
>
> I made a couple of changes like the signature of a function used as a fn
> pointer and avoiding a variable rename but this is essentially the same
> code people have been testing against older kernels.
>
> It's also in bluetooth-alsa cvs under plugz/patches/
>
> I used to have a testcase from Fabien that would consistently crash the
> unpatched kernel but I couldn't dig it up. That would be good for eg
> regression testing and I will put it in plugz cvs when we track it down.
I did an initial review and there are still some whitespace and coding
style issues that need to be fixed.
The biggest thing however is hci_sched_sco(). This function is ugly as
hell and basically unreadable. Please use break or continue instead of
cascaded if clauses. I am not gonna review this piece of the patch until
that is fixed.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Brad,
> > And why do we have to do "c->tx_timer.function = hci_sco_tx_timer" over
> > and over again. Isn't it enough if we set the timer function once after
> > init of the timer.
>
> should this happen directly inside hci_conn.c or through something like
> a notification?
>
> To use it in hci_conn I'd want to move the definition of the function
> there too but that may not be the right fit.
actually hci_sco_tx_timer() should be in hci_conn.c. There is no need to
put that into hci_core.c and make this so complex.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel
> And why do we have to do "c->tx_timer.function = hci_sco_tx_timer" over
> and over again. Isn't it enough if we set the timer function once after
> init of the timer.
should this happen directly inside hci_conn.c or through something like
a notification?
To use it in hci_conn I'd want to move the definition of the function
there too but that may not be the right fit.
Brad
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Brad,
> > The biggest thing however is hci_sched_sco(). This function is ugly as
> > hell and basically unreadable. Please use break or continue instead of
> > cascaded if clauses. I am not gonna review this piece of the patch until
> > that is fixed.
>
> I fixed a few whitespace problems and changed hci_sched_sco so it didn't
> get deep into nested if's. How does it look now?
please also fix the other coding style issues. Some comments and a lot
of if clauses are violating the whitespace requirements. It is "if ("
and not "if(". It is also "(char *) &val" and not "(char *)&val".
And the code in sco_sock_queue_rcv_skb() is bad. We don't initialize
variables if not really needed. In this case if skb_queue_len() check
fails simply call return -ENOMEM. Strip the label and call return 0 at
the end of the function. Don't make the code more complex than it
actually is.
And no forward declaration of sco_sock_queue_rcv_skb() if not really
needed.
Why are we using SCO_TXBUFS and not reuse SO_SNDBUF since the value is
directly mapped to sk_sndbuf. Same applies to sk_rcvbuf.
And why do we have to do "c->tx_timer.function = hci_sco_tx_timer" over
and over again. Isn't it enough if we set the timer function once after
init of the timer.
Regards
Marcel
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel