Hi Cathy, Cathy Zhou wrote: > This is a code review request for the libdladm resturcture changes, > which is a part of the support work for the Clearview Nemo unification > and vanity naming component. ... > and the webrev in: > > http://jurassic.eng/net/aquila.prc/export/home/cathy/onnv-clone/webrev_review/ >
The changes look good, only a couple of minor comments: General: We now have a single library, and still many header files for various sections of the API provided by that library. These are all named "libdl<something>.h". Since there is no "libdl<something>" library and all of these libdladm-related header files are in the top level of /usr/include, I'm not enamored with the naming scheme. It would make more sense to me to have a subdirectory named /usr/include/libdladm/, in which we have header files such as <libdladm/aggr.h>, <libdladm/wlan.h>, <libdladm/link.h>, etc... This isn't a very big deal since these are consolidation private header files that we don't ship, but maybe I'd consider this a more serious comment if/when the interfaces are promoted and we start shipping these files in /usr/include. Note that there is some precedent for shipping a library's bundle of header files under /usr/include/<library-name>/ usr/src/cmd/dladm/dladm.c: 645: dladm_aggr_down() no longer returns int, but returns a dladm_status_t, yet we ignore that value and assume that errno was set if the return value wasn't DLADM_STATUS_OK. We should use the returned dladm_status_t to print a meaningful error, as we shouldn't assume that the function sets errno. 2573: it looks like this should be dlpi_open() to isolate this part of dladm() from future changes to /dev/net, etc. If you feel this is out of scope, please file an RFE so that we can fix this as part of Clearview/UV. usr/src/lib/libdladm/common/libdladm_impl.h 31: why is the inclusion of libdladm.h removed if that's where dladm_status_t comes from? -Seb
