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.
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.
