Thanks Meem for your comments. I'll look at the comments and revise as needed.
sagun Peter Memishian wrote: > > As there are some clearview team members planning on reviewing the > > changes and currently there are other high priority work ongoing, I am > > extending the code review timer for another two weeks. I'm setting the > > timer to December 13th, 2007. > > > > The webrev can be found at: > > > > http://cr.opensolaris.org/~sagun/libpcap/ > > I took a quick look at this. Some offhand observations: > > * There's too much duplicated code betwen pcap-libdlpi.c and > pcap-dlpi.c. For instance, both strioctl() and the pcap_stats() > functions are identical, as is the bufmod-related logic > (starting with "Loop through packets") in pcap_read(), and > the bufmod configuration in pcap_open_live(). I'd move the > relevant bits of logic to a new pcap-streams.c file and call > those routines from pcap-*dlpi.c. > > * The linklist structure should be renamed; it's too close to > "linked list". > > * The link chaining logic in list_interfaces() doesn't seem like > it would work for more than two links. In particular, seems > like you need: > > if (lwp->lw_list == NULL) { > lwp->lw_list = entry; > } else { > entry->ll_next = lwp->lw_list; > lwp->lw_list = entry; > } > > (Also, there's no need to initialize `entry' or cast `lwp'.) > > * In pcap_read_libdlpi(), I'm not sure what the special error > handling after dlpi_recv() is about. It looks to be some > holdover from the old pcap-dlpi.c code, but I can't see how it > applies to dlpi_recv() since dlpi_recv() only calls getmsg() > when poll() has indicated there's data to read, and thus > it should never get EINTR or EAGAIN -- and if it did, that is > an issue that should be handled inside dlpi_recv() itself. > > * Seems error handling would be cleaned up quite a bit by > introducing a utility function like: > > static void > pcap_libdlpi_err(dlpi_handle_t *dh, const char *func, int err, > char *errbuf); > > * Please check for spelling errors -- e.g. "promsic" on lines 330 > and 341. > > * In pcap_open_live(), `flags' should be removed and DLPI_RAW | > DLPI_PASSIVE directly passed into dlpi_open(). Also, the > comment above dlpi_open() should be updated to say something > more appropriate like "Enable Solaris raw and passive DLPI > extensions; see the Solaris dlpi(7P) manpage for details". > > * Surely line 317 is supposed to set `retv' so that line 320 will > print something meaningful? > > * Would be good to be consistent with either `ret' or `retv' as > a variable to house the return value. >
