Hello Anders, Thanks for reviewing the code changes, my comments in-line below.
Anders Persson wrote: > 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 ACCEPT > * 107: Do you not need CONSTCOND to keep lint happy? I don't see any lint complaints in nightly output. > uts/common/io/trill_impl.h > > * Should it be moved to uts/common/net? It is private for the TRILL module so I believe the location is okay. > 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. ACCEPT. Thanks for pointing this out, I will add code to handle this. > * 102: Why does the message length impact the ethernet type? Should it not > always be TRILL (in the non-VLAN case)? It doesn't, we are using the msglen param to distinguish the two types: TRILL data packets and control IS-IS packets. > * 175: Consider adding ASSERT(RW_LOCK_HELD(...)) ACCEPT > * 871: Please add a trill_ prefix Not sure which one here, please specify. > * 900-901: Just use kstat_named_init() ACCEPT > * 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. ACCEPT > * 1128-1319: Please use ddi_copyin/copyout instead of so_copyin/copyout. ACCEPT > * 1135, 1174, 1189: remove extra white space ACCEPT > * 1371: Please split this into two statements ACCEPT > * 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(). ACCEPT > * 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. ACCEPT, I will remove connect and shutdown. > * 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. ACCEPT, yes I will use them. I will post an updated webrev once I am done testing my changes. Thanks, Rishi _______________________________________________ networking-discuss mailing list [email protected]
