Hi Jim, I have your comments incorporated. For the ones that were not Accepted or needed explanation I've inlined below.
James Carlson wrote: >> If you have access to the SWAN, webrevs are located at: >> http://zhadum.east.sun.com/export/ws/ss150715/clearview-libdlpi-port-onnv/webrev/ >> > > Not yet reviewed (I'll have to get to them tomorrow): > > lib/libuuid/common/etheraddr.c > lib/libuuid/common/etheraddr.h > > cmd/cmd-inet/sbin/dhcpagent/dlpi_io.c > > 94: shouldn't there be a free(msgbuf); return (-1); near here? > > The reason for continuing and not returning(-1) is because we are only informing that there was more data then what was asked for. 'msglen' was the data size requested but but there was more date on received on the stream. > cmd/cmd-inet/sbin/dhcpagent/interface.c > > 122,130,139,357,363: this should be MSG_ERROR, not MSG_ERR. > (MSG_ERR assumes that errno was set, and it appends ": %m" to the > output.) (The one at 370 is correct.) > ACCEPTED ah..yes, errno can be set by any other process so it would append the ":%%m" to the output. > cmd/cmd-inet/sbin/dhcpagent/packet.c > > 1331: why is 'arg' a void * rather than just a dhcp_pif_t *? It > looks like all of the callers know that it's a PIF. > Accepted. none of the callers of recv_pkt() use 'arg' except dhcp_collect_dlpi(). > usr/src/cmd/cmd-inet/usr.sbin/in.rarpd.c > > 132,135: why were these renamed? The functions in question are the > ones that handle RARP and ARP "request" messages, so the original > name seemed reasonable to me. > I was thinking along the line - these function provide RARP and ARP replies. But on second thought that seems more confusing. Preserving original name. > 407,408: initializers seem to be unused. > dlpi_get_physaddr() needs to be passed a 'saddrlenp' >= DLPI_PHYSADDR_MAX. 'str' is initialized so that it gets malloc'd in _link_ntoa(). > 511: why have an extra 'saddr' buffer? Why not just pass in 'shost' > here and avoid the extra memcpy at line 540? > This can be done and I've changed it. > 655: why was syslog() changed to error()? The former just logs an > error; the latter causes the daemon to abend. (Perhaps not a big > problem, as the rest of the daemon is littered with exit points.) > I was thinking since dlpi_send() wasn't able to send the data, it should bail out. But since a RARP_REQUEST is requested again, using syslog and not exiting out of the daemon would be better. Change would need the message printed in case 'dflag' is set to change as well. > 854: cstyle: compare pointer against NULL. > > 1000: as long as you're cleaning things up, please use ANSI C. > > 1006,1019: could reasonably be const. > Accepted. -Thanks, Sagun -- Sagun Shakya 781.442.7344/ X27344 sagun.shakya at sun.com
