Thank you Seb for reviewing the changes.
Sebastien Roy 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/
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.
Accepted.
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).
I'll find out more if such media need to be considered. If so, then
will fix line 178,179.
* 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().
Accepted. I've change the funtction signature of set_packet_filter() from
void set_packet_filter(int, filter_func_t *, void *,const char *);
boolean_t set_packet_filter(dlpi_handle_t, filter_func_t *, void
*,const char *);
and moved all the details of filtering in that one function.
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?
Accepted all the comments about in.rarpd.c. I had only considered
DLPI_ETIMEDOUT in which case continue with dlpi_recv() again. and
DL_SYSERR thinking other error that dlpi_recv() may return wouldn't
exist, like DLPI_EINHANDLE. Reason being we could trust that dlpi_recv
had received a valid handle. I've reorganized the handling to be
if(retval==DLPI_ETIMEDOUT)
continue;
else if(retval!= DLPI_SUCCESS)
error handle
/*continue with DLPI_SUCCES */
* 653: This is a stray call to syslog() it seems. All other errors are
logged via the error() function.
Good catch. Accepted.
thanks,
Sagun
_______________________________________________
networking-discuss mailing list
[email protected]
