Thanks Seb. I Will send out an updated webrev. My comments are below:
Sebastien Roy wrote:
http://cr.opensolaris.org/~sagun/libpcap/
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.
I agree, besides just getting to use a simpler libdlpi library there is
no feature advantage in using the libldpi library without dlpi_walk(). I
will change the logic to be use:
if (dlpi_handle_t && dlpi_walk())
use pcap_libldpi.c
else
use pcap_dlpi.c
dlpisubs.c
* General question: Did you modify any of the code in these functions?
Not the code itself. I'll fix line 8.
* 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.
Since I'm not an expert either, I wasn't sure what to do here and in the
new file added. I was expecting there would be some comment regarding
how it would be best to use them.
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?).
I think pcap-int.h would be appropriate.
pcap-libdlpi.c
* 46,51: The indentation looks off here.
Fixed.
* 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. :-)
I'll add a comment here.
* 83: Why is this "register"?
Don't have a good reason except left what was similar to pcap-dlpi.c.
* 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?
There is support for this in libdlpi and this would be a useful feature.
Besides testing I don't think this would be a lot of work so if it makes
sense to include this now I'll make the changes.
* 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.
Will fix, but before that I have a question,I had changed the logic here
in comparision to pcap-dlpi.c; in pcap-dlpi.c the logic is :
if (promisc)
enable physical promiscuous mode
then enable multicast promiscuous mode
else
enable promiscuous mode at SAP level.
In pcap-libdlpi I have it to be, :
if (promisc)
enable physical promiscuous mode
else
enable multicast promiscuous mode
enable promiscuous mode at SAP level.
I guessthe logic should be as in pcap-dlpi.c?
* 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).
Agree and will fix.
* 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));
Will fix.
* 277: What is expected? Please elaborate on this comment, as it has
little value as-is.
I'll add a comment.
* 278: Is this logic correct? I'm looking at the old pcap-dlpi.c, and
the handling for EINTR is completely different.
Removing the condition (p->break_loop) in line 278 should make it the same.
* 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)
I agree with Meem's comment on this. Also, setting the handle to NULL is
not necessary at all. I'll fix it in pcap-libdlpi.c.
* 323: free() handles being called with a NULL argument.
Ok I"ll fix this.
Thanks,
-Sagun
-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.
