Hi Seb and Guy,
I've generated a new webrev that incorporates your comments. The webrev
can be found at:
http://cr.opensolaris.org/~sagun/libpcap-review2/
In the changes, I have not added the support for consumers of libpcap to
have access to raw WIFI frames. I will be sending those changes separately.
I did not fix the logic for the following comment since I think it is
okay to return a failure in the case of pcap-libdlpi.c as the logic is
different to the logic in pcap-dlpi.c.
* 130: This looks like a bug. In the pcap-dlpi.c code, failure to
enable multicast promiscuous mode does not result in a failure, but
only a warning.
Please let me know if you have any further comments.
Thanks,
Sagun
Sebastien Roy wrote:
sagun shakya wrote:
I've addressed all the comments from below and updated the webrev.
http://cr.opensolaris.org/~sagun/libpcap/
While testing the changes I found out that my changes didn't work on
a Solaris system that had libdlpi.h version
that existed before the public libdlpi.h was integrated. Some changes
had to be made to configure.in and I added a couple of new macros to
aclocal.m4. Could you please review those changes as well.
Good stuff, Sagun. Here are my comments:
aclocal.m4 and configure.in
* The macros and code in these files seem to be going out of their way
to support old versions of the public libdlpi library that didn't
contain a dlpi_walk() function. Why? It seems to me that the logic
should be; if there's a public libdlpi library (defined as a library
that has dlpi_handle_t and dlpi_walk()), then use it. Otherwise,
don't use it. There's no value (IMO) to using some hybrid libdlpi
library that one or two Sun employees have lying around on their stale
builds of Nevada.
dlpisubs.c
* General question: Did you modify any of the code in these functions?
* 118-123: I would think that in 2008, most compilers can do a better
job at assigning register use than this (making every single variable
a "register" variable.) You don't have to fix this; this is more a
question for those more knowledgeable than me at compiler optimization.
pcap-dlpi.c
* 139: This belongs in a header file. There should be a private
header file with declarations for the functions in dlpisubs.c (the
existing pcap-int.h perhaps?).
pcap-libdlpi.c
* 46,51: The indentation looks off here.
* 61: Is the idea that the caller frees the memory allocated here? If
so, it would be good to state that in a comment so that libpcap
novices like me don't assume that there's a memory leak in this code. :-)
* 83: Why is this "register"?
* 101: Perhaps this is a good time to ask about DLPI_NATIVE. For
those not familiar with the concept, the idea is that some DLPI
providers masquerade as a well-known media type by default, but can
morph into their "native" types on-demand. Recent Solaris WiFi DLPI
devices behave this way (DL_ETHER by default, but DL_WIFI under the
covers). Passing the DLPI_NATIVE flag to dlpi_open() causes such
providers to provide access to their native media type. Do we have a
plan to eventually add support for DLPI_NATIVE here so that libpcap
consumers can have access to raw WiFi frames, for example?
* 211: Related to my previous comments on dlpi_walk() support. It
looks like this function flies apart and doesn't work in the absence
of dlpi_walk(), so I don't see the point of attempting to support
libdlpi without dlpi_walk() when pcap-dlpi.c should work just fine for
such platforms (better than pcap-libdlpi.c in fact).
* 257: The indentation could be cleaned up by checking for len != 0
first. For example:
len = p->cc;
if (len != 0) {
bufp = p->bp;
goto process_pkts;
}
do {
/* dlpi_recv() stuff */
} while (len == 0);
process_pkts:
return (pcap_process_pkts(p, callback, user, count, bufp, len));
* 277: What is expected? Please elaborate on this comment, as it has
little value as-is.
* 278: Is this logic correct? I'm looking at the old pcap-dlpi.c, and
the handling for EINTR is completely different.
* 321: dlpi_close() handles being called with a NULL dlpi_hd. (that
should probably go in the dlpi_close() man page, but I don't see it
there)
* 323: free() handles being called with a NULL argument.
-Seb
`
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.
