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
