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
_______________________________________________
networking-discuss mailing list
[email protected]
