sagun shakya wrote:
> I've incorporated the comments received so far and here are the updated
> webrevs :
>
> http://cr.grommit.com/~sshakya/libdlpi-port-commentfixed/ (off-SWAN)
> http://zhadum.east.sun.com/export/ws/ss150715/clearview-libdlpi-port-onnv-moving/webrev/
>
I based my review on these updated webrevs. This looks great, and a net
loss of 1300 lines of code to boot! :-) I only have a couple of comments:
usr/src/cmd/cmd-inet/sbin/dhcpagent/dlpi_io.h
* 48: This is only used in one placd in dlpi_io.c. I'd yank this out
and put it at the top of dlpi_io.c.
usr/src/cmd/cmd-inet/sbin/dhcpagent/interface.c
* 178,179: This will fail if the media has 0 length physical addresses
(such as on a poin-to-point interface). I guess one question is, does
this need to work on such media? The previous code built a
destination address to be used in DL_UNITDATA_REQ, which was never
NULL even if the physical address length was 0 (because that address
included the SAP as well).
* 369-377: My preference would be to put the details of all filtering
setup inside of the set_packet_filter() function, including pushing
pfmod. That way, all of the implementation details of configuring a
packet filter for DHCP is self-contained in one function rather than
two. I would then also pass a dlpi_handle_t to set_packet_filter().
usr/src/cmd/cmd-inet/usr.sbin/in.rarpd.c
* 514,533,536: The structure of this construct doesn't look quite right.
You break out of the loop for DL_SYSERR, but you continue for any
other kind of error. What's special about DL_SYSERR in that case? It
would also be more legible to reorganize this such that the error
conditions are handled first, like:
retval = dlpi_recv(...);
if (retval != DLPI_SUCCESS) {
error("...", ...);
continue;
}
/* go on with the DLPI_SUCCESS path, without indenting all of that
code */
* 527: Might as well fix this pre-existing cstyle nit:
if (cause != NULL) {
* 537: Why do we print an error for DL_SYSERR, but not for any other
kind of DLPI error?
* 653: This is a stray call to syslog() it seems. All other errors are
logged via the error() function.
usr/src/lib/libuuid/common/etheraddr.c
* The shrinkage of code in this library is mind boggling. :-)
-Seb
