Rishi, On Thu, 2009-07-16 at 13:22 -0400, Rishi Srivatsavai wrote: > Code review request for the RBridges project. RBridges project > is delivering layer-2 Bridging and TRILL support in Solaris. > > Webrev for the ON changes (Bridging and TRILL): > http://cr.opensolaris.org/~rishi/rbridges/
Here are my comments. I reviewed the bridge module, GLDv3 kernel changes, and bfu. I also took a peek at other files, but did not do a full review. cmd/cmd-inet/usr.lib/bridged/door.c * 75-159: Every case in this switch ends with: unlock_engine() (void) door_return(); return; These could all be replaced by a "break;" since this is done after the switch. cmd/cmd-inet/usr.sbin/snoop * General: Does Wireshark have support for ISIS, TRILL, and BPDU's? If not, it should be added there. lib/librstp/common/README * General: This README is almost totally nonsensical. It provides no actual information while referring to things that don't exist. tools/scripts/bfu.sh * 8300-8362: I'm don't think I agree with the approach used here. One problem is that this won't work on OpenSolaris. Another is that you've duplicated the parsing of the file in bfu.sh while the parsing is already in dlmgmtd itself. This code is sure to be broken if the file format changes further. Instead, dlmgmtd could handle reading from the old format and writing to the new one. I don't see an actual need to have the file format changed before dlmgmtd runs. * 8317: There is no such thing as the "PPA hack" anymore. This is evidence that scripts in bfu.sh become obsolete and/or broken almost immediately. ;-) uts/common/io/bridge.c * 172,175: I fail to fully understand (based on the comments above this) why you need both a rwlock and a mutex. Why wouldn't using a single mutex be sufficient? From inspection, it doesn't look like the code benefits very much from supporting concurrent readers on the inst_list. * 324: Add intro-block-comment. Send what up? Send up where (where does this STREAMS queue go)? * 393: Having this ioctl entry point process ioctls through the bridge's STREAMS DLPI device in /dev/net adds unnecessary privilege requirements on your ioctls. For one, you will never be able to add unprivileged ioctls in the future, but also, it means that your current ioctl implicitly requires net_rawaccess (which seems wrong). Instead, you could register ioctls with dld_ioc_register() to be processed by the /dev/dld control node. It will mean that your ioctl data will need to contain the necessary bridge identifier for you to be able to do a lookup for the appropriate bridge object instead of having it passed in through the opaque arg. This entry point is also simpler as it does all of the necessary data size checking and privilege checks for you prior to calling your ioctl callback. * 404: There doesn't appear to be any sanity checking of the mblk. You need to make sure that the data is sane before diving into b_rptr's (assuming you reject my earlier suggestion to use dld_ioc_register()). * 574: kstats created through kstat_create() are visible from all zones. Since the bridge object itself isn't visible from all zones, that doesn't seem like the right scope for this kstat. kstat_create_zone() can be used to specify the zone scope of the kstat. * 619: If this is true, then why do you support setting MAC_PROP_MTU through bridge_m_setprop()? * 635: For the vast majority of the time, there won't be any entries to delete, so it seems strange to periodically lock the list to check if anything needs to be deleted during the entire lifetime of all bridges. An alternative would be to only schedule a timeout when something is deleted (instead of when something gets created). You'd need a separate bridge_mac_t garbage collection timer, and there could be one timer per "inactive" bridge (instead of one timer that affects all active bridges). * 797-802: This fail code doesn't work since bipnew gets set to NULL at 767. * 1473: There no real point to * 1532: Why isn't /dev/dld the control device like it is for all other GLDv3 objects? * 2179: Is there any special handling for link up/down by anything related to bridging? * 2643: XXX to be addressed here. * 3062: This should just be a callback registered through dld_ioc_register(). You then wouldn't need the privilege checks, and wouldn't need to do sanity checking on the size of your data (which is currently missing from this function). * 3097: EEXIST is one of the possible causes of bridge_create() failing, but is not the only one, and returning this could be quite misleading. bridge_create() and bmac_alloc() should both return proper error codes so that the ioctl can have meaningful error semantics. * 3302: Related to other comments, I don't see a place for STREAMS in this module. The only use seems to be to process control ioctls, and those should be funneled through /dev/dld. uts/common/io/dld/dld_str.c * 1713: References to "lowlink" are a bit alien. What is "lowlink"? Please add comments somewhere explaining what this is and why it's needed. * 2135: What is this? Please add a comment explaining what this is. * 2147: not that it makes a huge difference, ds_lowlink is boolean_t, not int. uts/common/io/mac/mac_provider.c * 648: Comment needs fixing; this appears to be used for both receive and transmit. -Seb _______________________________________________ networking-discuss mailing list [email protected]
