Hello Sowmini, Thanks for reviewing the code changes, my comments in-line below.
[email protected] wrote: > Rishi, > > I looked at the dladm/libdladm and quagga changes. > Here are my code-review comments.. > > ---------------------------------------------------------------------------- > files reviewed with no comments: > usr/src/lib/libdladm/common/usage.c > usr/src/lib/Makefile > usr/src/lib/libdladm/Makefile > usr/src/lib/libdladm/Makefile.com > usr/src/lib/libdladm/common/libdladm.c > usr/src/lib/libdladm/common/libdladm.h > usr/src/lib/libdladm/common/libdladm_impl.h > usr/src/lib/libdladm/common/llib-ldladm > usr/src/lib/libdladm/common/libdlbridge.h (please add comments) > usr/src/lib/libdladm/common/libdlvnic.c > usr/src/cmd/dladm/Makefile > ---------------------------------------------------------------------------- > General: > - please add comments to all of the exported functions to clarify semantics, > the input, and output args. A good place to do this would be in > libdlbridge.h ACCEPT > - Exported (via mapfile-vers) functions should first validate that the > pointerst that they are passed are non-null before following them > e.g., dladm_valid_bridgename should first check if bridge == NULL. Similarly > the following functions need to validate that the pointer they get > is non-null before using it: > dladm_observe_to_bridge, dladm_bridge_enable, dladm_bridge_state . > dladm_bridge_door_call, dladm_bridge_setlink, dladm_bridge_get_fwdtable, > dladm_bridge_get_trillnick, dladm_bridge_get_nick, dladm_bridge_set_nick, > dladm_bridge_get_port_cfg (there may be some that I missed, please check) REJECT, as long as the caller understands that the params must be non-NULL I think such checks will clutter the code. We already make assumptions on valid dld handle, linkid etc. > usr/src/cmd/dladm/dladm.c > - in bridge_opts, if P is used for "protect" and "p" is used for priority > then what's the flag for persistent? There is no support for temporary bridge instances. The bridging spec in section 1.1 describes the options: http://sac.sfbay.sun.com/Archives/CaseLog/arc/PSARC/2008/055/final.materials/bridging-spec.txt > - fmt_brind_id: is the addr field in the UID_BRIDGE_ID_T an ethernet address? > If yes, why not just use ethern_ntoa etc functions? REJECT. It includes priority prefix as well. > please rename "addr" in the UID_BRIDGE_ID_T to something more distinctly > cscope-able like ubid_addr. Also, the UID_BRIDGE_ID_T is a structure, > not an enum, so the convention si to use lower-case. REJECT. The file uid_stp.h is from upstream source so prefer to keep the changes to a minimum necessary. > Similarly fro UID_STP_BR_CTRL_T, UID_STP_CFG_T, UID_STP_MODE_T etc. Same as above. > - why is brsum_t constrained (in size) by pktsum_t? As I understand the code, > this structure is populated by print_bridge_stats() and has no relation > to pktsum_t, right? Similar question about brlsum_t We are re-using ls_prevstats which is a pksum_t. Same with brlsum_t at 7773. > - lines 7502-7506: should be using dladm_observe_to_bridge() ACCEPT > - line 7509 s/parseable/parsable ACCEPT > - why isn't blf_dest in bridge_listfwd_t declared as an ether_addr_t > and then ether_ntoa can be used at line 7904? Similarly for tln_nexthop > in trill_listnick_t and lines 7946-7948 of dladm.c ACCEPT > - Lines 7975, 7980, 7983- if this encompasses every line in the fields_arr > table, then there's no need to specify it to ofmt_open: if you specify > a NULL fields_str, then in the human-friendly mode, the ofmt functions > will default to printing as many fields as fit the standard 80 column > screen. Thus you can just initialize all_str to NULL, and only > re-assign it to some specfic string for bridgeMode (at line 8089) > or, if parsable is set, to the optarg (at line 8020). ACCEPT > usr/src/cmd/dladm/dladm.xcl and usr/src/lib/libdladm/libdladm.xcl > - no comment, I'm assuming you have run the internationalization tests > to verify this. ACCEPT. I will test. > usr/src/lib/libdladm/common/libdlbridge.c > - dladm_observe_to_bridge() is an exported function that could, in theory, > be passed a non-null-terminated string. So the strlen() call should be > bounded (i.e., use strnlen() with some reasonable upper-bound like > MAXLINKNAMELEN) ACCEPT > - line 1235: use strnlen, bounded by MAXLINKNAMELEN. then line 1238 can just > check if llen == MAXLINKNAMELEN. ACCEPT > - line 900: should verify that the brprot is UNKNOWN/STP (and not some > random garbage) before disabling trill? REJECT, caller does verify the user input in create_modify_add_bridge. > - dladm_bridge_get_portlist: what is the "256" constant? ACCEPT > - dladm_bridge_get_portlist is confusing: it sets *nports to the number > of ports, and returns a pointer to the ports- some comments > explaining the returned value would be helpful. ACCEPT > - dladm_bridge_get_portlist: what if there are no ports (i.e., nports == 0)? > Will the bridge_door_call return an error status? If not, there may be some > bugs in the way the callers access this function. The door call returns 0 in this case. I will check the callers. > usr/src/lib/libdladm/common/libdllink.h > - not sure why we need dladm_get_linkprop_values(). See comments for > linkprop.c > > usr/src/lib/libdladm/common/linkprop.c > - what is the need for dladm_get_linkprop_values()? Can't the caller (bridged) > just call dladm_get_linkprop directly, after allocating the prop_val array? > As it stands, dladm_get_linkprop_values() duplicates code in > dladm_get_linkprop() and besides, there's nothing in the name > ("dladm_get_linkprop_values") that indicates that this one only deals > with ints (int32? uint32? int64? uint64?) This function is needed to retrieve property values and runs the check function on the property values for conversion. If we used get_linkprop() we would still have to lookup the check fun from the property table and do all the other checks. I will clarify the comment as it is not specific for int values. > - line 187: remove two spaces before "may" and make this a complete sentence. ACCEPT > - line 2648: on sparc, sizeof int == 4, sizeof ulong_t is 8, sizeof uintptr_t > is 4. So how does this work on sparc? Wouldn't the truncation result in > errors? The function only supports max uint32 so doesn't result in truncated value. > - lines 3118-3120: suggest extracting this to a function > bridge_prop_to_code() that takes the bpp_name as input, and returns the > bpp_code as return value. Then this function can also be used to avoid > strcmp's in check_stp_prop (and also do additional validation of > the pd_name) REJECT, the code in check_stp_prop checks for a specific property so wouldn't be helpful to change it to a function. This can be done later if there are more callers. > usr/src/lib/libdladm/common/mapfile-vers > - not sure why dladm_get_linkprop_values is needed. > ---------------------------------------------------------------- > > SFW gate changes for Quagga are at: > http://cr.opensolaris.org/~rishi/sfw-trill/ > csope: > /net/zhadum.east/export/ws/rs200217/sfwtrilld/ > > - configure.ac: what does $opsys get set to on Nevada? and on opensolaris? >From the configure script it gets only set for: 21777 case "$host" in 21778 *-sunos5.[6-7]* | *-solaris2.[6-7]*) 21779 opsys=sol2-6 > - lib/command.c line 494: why is this change needed? Is it no longer true > that all the cmd_element structures are global, and therefore have > the strvec initialized to NULL? It is a memory leak fix, described in bug report (comment#1): http://defect.opensolaris.org/bz/show_bug.cgi?id=287 > - isisd.bool.h does not have standard include file guards. ACCEPT > - why isn't there an isis_destroy corresponding to isis_new()? I gather > this must be XCALLOC'ed so that we can have multiple instances of > isisd running in the future, but if that's the case, we should also > make sure we do the XFREE correctly. Alternatively, isis_new could > just use a global isis structure, and not make any half-hearted > steps toward the multiple instances support. The global exists throughout the life of the program. There are no multiple instances of isisd it is XCALLOC'ed to zero the structure. I will add a destroy call. > - area->area_tag is leaked: it is not freed in isis_area_destroy() ACCEPT > - nit: isis_vlans.h line 35, 40: missing spaces around > ACCEPT > - isis_vlans.h lines 71-73: is this really so perf critical that > we have to be so clever about it? REJECT. No but it is very well tested and is better than introducing a new one from scratch. > - isis_trillvlans.c line 139 missing space around + ACCEPT > - isis_trillio.c: line 186: sock_buff is defined with an upperbound > of 8192 in isis_dlpi.c, but here we use a recvfrom len of > circuit->interface->mtu. What if the latter is greater than > sizeof (sock_buff)? ACCEPT > - isis_trill.c: instead of opening/closing the dlhandl on each > trill_internal_reload, why not just open it once in main() and > close it when main()exits? REJECT. It is only called once on startup or on SIGHUP so doesn't seem beneficial to store it across calls. > - isis_trill.h - would be good to have some comments in here, providing > references to the IETF/IEEE docs from where these constants are derived ACCEPT, many are TBD I will review and check again. I will post an updated webrev when I have finished testing updates from code reviews. Thanks, Rishi _______________________________________________ networking-discuss mailing list [email protected]
