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?
* 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.
* 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
