Hello Seb, Sebastien Roy wrote: > 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.
Thanks for the review Seb. My comments in-line below. > 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. REJECT. I checked this out but the return args in the door_return() call change depending on the case statement and it seemed better to leave it as is. > cmd/cmd-inet/usr.sbin/snoop > > * General: Does Wireshark have support for ISIS, TRILL, and BPDU's? If not, > it > should be added there. DEFER. Yes for ISIS and BPDUs but not for new TRILL TLVs in ISIS. It should be easy to add though. I will create a RFE to track this work. > lib/librstp/common/README > > * General: This README is almost totally nonsensical. It provides no > actual information while referring to things that don't exist. DEFER. The README is unchanged from upstream source. I will create a RFE to introduce a new README for OpenSolaris. > 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. ;-) After offline chat with Seb we agreed that this code to handle the upgrade of default_tag property was still needed. I will work with Seb in adding similar support in OpenSolaris packaging. > 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. ACCEPT. Removed the rwlock > * 324: Add intro-block-comment. Send what up? Send up where (where > does this STREAMS queue go)? ACCEPT. Added a comment > * 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. ACCEPT. Changed to use dld control ioctl. > * 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()). ACCEPT. Changed to use dld control ioctl. > * 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. ACCEPT. Using kstat_create_zone with zone scope set to GLOBAL_ZONEID. > * 619: If this is true, then why do you support setting MAC_PROP_MTU > through bridge_m_setprop()? ACCEPT. I have updated the comment. Setting a new bridge MTU is allowed. > * 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). DEFER. Number of bridge instances on a system is expected to be low. The timer flushes expired entries from the table so if I understand your suggestion right we will schedule a timer when entries exist and remove the timer when there are no longer any entries to expire in the table. I will file an RFE to track this change but I am not sure if it is worth optimizing this code at this time. We should revisit it later. > * 797-802: This fail code doesn't work since bipnew gets set to NULL > at 767. ACCEPT > * 1473: There no real point to Typo? > * 1532: Why isn't /dev/dld the control device like it is for all other > GLDv3 objects? We are using bridgectl for both ioctls and control messages from the bridge daemon. For libdlbridge we will use the dld control device for unprivileged ioctls as noted above. > * 2179: Is there any special handling for link up/down by anything > related to bridging? Yes in bridged/dlpi.c there is handling of link up/down. > * 2643: XXX to be addressed here. ACCEPT. Removed as it is complete > * 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). These are control ioctls from the bridge daemon. As the bridge daemon requires bridgectl I think using it for both control messages and ioctls would be good. > * 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. ACCEPT > * 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. It is used for passing messages between the bridge daemon and the module, SDU messages are passed up via send_up_messages. > 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. ACCEPT > * 2135: What is this? Please add a comment explaining what this is. ACCEPT > * 2147: not that it makes a huge difference, ds_lowlink is boolean_t, > not int. ACCEPT > uts/common/io/mac/mac_provider.c > > * 648: Comment needs fixing; this appears to be used for both receive > and transmit. ACCEPT I will post an updated webrev once I have finished updates and testing the changes from code reviews. Thanks, Rishi _______________________________________________ networking-discuss mailing list [email protected]
