Sowmini, Thanks very much for your comments! I will look into this and get back to you.
- Cathy > On (09/28/07 21:37), Cathy Zhou wrote: >> External webrev: >> >> http://cr.grommit.com/~yun/webrev_uv_09_28 >> >> Internal webrev: >> >> http://jurassic.sfbay/net/forwar.prc/builds/yz147064/clearview-uv-review/webrev_review/ >> >> >> Cscope: >> >> /net/forwar.prc/builds/yz147064/clearview-uv-review/ >> > > Cathy, > > here are my comments, after reviewing the dladm/libdladm portions. > > Thanks > Sowmini > > ---------------------------------------------------------------------- > > usr/src/cmd/dladm/Makefile > - ok > > usr/src/cmd/dladm/dladm.c > - (nit) add a comment over str2int explaining differences from atoi. > - (nit) line 365 s/possible/possibly > > usr/src/cmd/dladm/dladm.xcl > - ok > > usr/src/lib/libdladm/Makefile > - ok > > usr/src/lib/libdladm/Makefile.com > - ok > > usr/src/lib/libdladm/common/libdladm.c > - ok > > usr/src/lib/libdladm/common/libdladm.h > - add comments explaining DLADM_OPT_* constants. > > usr/src/lib/libdladm/common/libdlaggr.c > - lines 487,496,502: can free portstr as part of destroyconf > - line 514, 701: check unnecessary: free() will return if pointer is null. > - lines 508-509: unecessary- we're going to go to destroyconf regardless > of status. Is an else clause missing here for the success case? > Not sure how we would get to line 520 if DLADM_OPT_PERSIST is set. > See next comment: > - i_dladm_aggr_add_rmv() : > There seem to be some constraints about what flags are possible- these > should be clarified via comments. For example, > orig_portstr is only calloc-ed if DLADM_OPT_PERSIST is set; > Is it possible to have !PERSIST but ACTIVE? If yes, then we would > get to line 552 with null orig_portstr, and seg fault at the strlen > in dladm_set_conf_field(). Similarly, is it not possible to have > both PERSIST and ACTIVE set at the same time? If yes, then please > add some comments clarifying this. Otherwise, how would we get to line > 520 (see previous comment) > - i_dladm_aggr_create_sys() : mac_addr can be const u_char *. > - (pre-existing condition) VALID_PORT_MAC should check for, and fail > if mac_addr is null. > - dladm_aggr_policy2str: check (and bail) if str is null. Pass in > size of str, and use strncat instead of strcat (or alternatively, > allocate str as needed in dladm_aggr_policy2str() and expect the caller > to free it) > - dladm_aggr_create: why is key == 0 being accepted now? > - is there a reason why dladm_aggr_delete does not call > dladm_phys_down()? In other words, the only step skipped by > dladm_aggr_delete (but performed by dladm_phys_down) > is dladm_phys_info()- is this an optimization or an oversight? > - Question: why does dladm_phys_down delete all the active linkprop > before deleting a link? > > usr/src/lib/libdladm/common/libdlaggr.h > - ok. > > usr/src/lib/libdladm/common/libdllink.c > - lines 349-351: how is atomicity ensured across the pair > of {dladm_read_conf(); dladm_write_conf();} operations? > My understanding (comments in code would help here) is that > the read_conf() reads out the conf from the avl tree managed > by dlmgmtd, and the write_conf() will actually write this to > a file. So is there a possibility that 2 threads simultaneously > do read_conf, and modifications, but one thread's modifications > are lost? > > usr/src/lib/libdladm/common/libdllink.h > - ok > > usr/src/lib/libdladm/common/libdlmgmt.c > - ok > > usr/src/lib/libdladm/common/libdlmgmt.h > - ok > > usr/src/lib/libdladm/common/libdlvlan.c > - ok > > usr/src/lib/libdladm/common/libdlvlan.h > - ok > > usr/src/lib/libdladm/common/libdlvnic.c > - ok > > usr/src/lib/libdladm/common/libdlvnic.h > - ok > > usr/src/lib/libdladm/common/libdlwlan.c > - line 852, 865: why the double cast (uintptr_t itself typedef to > unsigned int in int_types.h)? > - line 1085 s/introudce/introduce > - line 1110 s/indicate/indicates > > usr/src/lib/libdladm/common/libdlwlan.h > - move wlan specific enums (e.g., dladm_wlan_radio_t, > dladm_wlan_powermode_t, > dladm_wlan_rates_t, radio_vals, powermode_vals) into libdlwlan.h > so that they are all in one place. > > usr/src/lib/libdladm/common/libdlwlan_impl.h > - val_desc_t is really a general purpose structure that is shared > with linkprop.c (and possibly other files that will be added > in the future), and not specific to wlan.; not clear why this is > not defined in libdladm.h instead? > > usr/src/lib/libdladm/common/linkprop.c > - move wlan specific enums (e.g., dladm_wlan_radio_t, > dladm_wlan_powermode_t, > dladm_wlan_rates_t, radio_vals, powermode_vals) into libdlwlan.h > so that they are all in one place. > - rename radio_vals to something like dladm_wlan_radio_vals so that > it follows the convention used by the other wlan vars. > - the implementation of dladm_init_linkprop is much easier to follow! > what is the intention of the "arg" (unused) argument to > i_dladm_init_one_prop? > - delete extra line at 249. > - This question was asked in one of the mailing lists earlier, but > since I can't remember the answer, maybe it needs a detailed comment > in the code: what do the fields pd_optval, pd_noptval mean? When > adding new properties, how does one determine if these are neeeded? > They seem to impact do_check_prop, so commenting that function as well > would help for future reference. > - move MAX_SUPPORT_RATES to one of the libdlwlan* files > > > usr/src/lib/libdladm/common/llib-ldladm > - ok. > > usr/src/lib/libdladm/common/mapfile-vers > - ok > usr/src/lib/libdladm/common/secobj.c > - ok >
