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
