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
- 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)
----------------------------------------------------------------------------
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?
- 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?
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.
Similarly fro UID_STP_BR_CTRL_T, UID_STP_CFG_T, UID_STP_MODE_T etc.
- 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
- lines 7502-7506: should be using dladm_observe_to_bridge()
- line 7509 s/parseable/parsable
- 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
- 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).
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.
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)
- line 1235: use strnlen, bounded by MAXLINKNAMELEN. then line 1238 can just
check if llen == MAXLINKNAMELEN.
- line 900: should verify that the brprot is UNKNOWN/STP (and not some
random garbage) before disabling trill?
- dladm_bridge_get_portlist: what is the "256" constant?
- 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.
- 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.
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?)
- line 187: remove two spaces before "may" and make this a complete sentence.
- 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?
- 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)
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?
- 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?
- isisd.bool.h does not have standard include file guards.
- 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.
- area->area_tag is leaked: it is not freed in isis_area_destroy()
- nit: isis_vlans.h line 35, 40: missing spaces around >
- isis_vlans.h lines 71-73: is this really so perf critical that
we have to be so clever about it?
- isis_trillvlans.c line 139 missing space around +
- 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)?
- 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?
- 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
--Sowmini
_______________________________________________
networking-discuss mailing list
[email protected]
