>> http://jurassic.eng/net/aquila.prc/export/home/cathy/onnv-clone/webrev_review/ >> > > > The changes look good, only a couple of minor comments: > Thank you!
> 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>/ > I don't have strong opinion either way. Like you said, we could choose to make this change when we decide to ship the header files. > > 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. > Accepted. I've made the change. > 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 > 6534263 is filed. > 31: why is the inclusion of libdladm.h removed if that's where > dladm_status_t comes from? > Okay. I've changed it back. - Cathy
