On Thu, Jul 16, 2009 at 01:22:06PM -0400, Rishi Srivatsavai wrote: > Code review request for the RBridges project. RBridges project > is delivering layer-2 Bridging and TRILL support in Solaris.
Rishi, My comments are below. Cheers, Anders ---------------------------------------------------------------------- uts/common/net/trill.h * 105-106: Indentation is a level off * 107: Do you not need CONSTCOND to keep lint happy? uts/common/io/trill_impl.h * Should it be moved to uts/common/net? uts/common/io/trill.c * General: Sockfs does not serialize socket operations, so it is possible for your socket module to receive multiple bind requests at the same time, or receive an unbind while a write()/ioctl() is going on. It looks like you need to add some logic to ensure that ts_link is not set to NULL while doing, for example, write(). Similarly, ts_link is set to NULL when the link is going away, but there does not look like there is any guard to make sure a write is not active. * 102: Why does the message length impact the ethernet type? Should it not always be TRILL (in the non-VLAN case)? * 175: Consider adding ASSERT(RW_LOCK_HELD(...)) * 871: Please add a trill_ prefix * 900-901: Just use kstat_named_init() * 917: Whether you can use KM_SLEEP in the 'create' downcall depends on the flags that are passed to the downcall. Sleeping is only OK if SOCKET_SLEEP is set. * 1128-1319: Please use ddi_copyin/copyout instead of so_copyin/copyout. * 1135, 1174, 1189: remove extra white space * 1371: Please split this into two statements * 1377: It is the responsibility of the socket module to enforce receive side flow control. When su_recv returns -1, that indicates that the buffer is full and that you should stop pushing up data. Sorry about the lack of documentation, but please have a look at udp_ulp_recv(). * 1437-1460: connect() binds to the destination address, which is rather odd. Does 'connect' even make sense for AF_TRILL sockets? And if you get rid of 'connect', then you can just remove 'shutdown' as well. * 1533-1583: Depending on when you integrate, you might want to remove these and use the "noops" that Darren will make available in his PF_PACKET push. _______________________________________________ networking-discuss mailing list [email protected]
