Re: [clearview-discuss] libdlpi with libpcap

[sagun shakya <Sagun.Shakya () Sun COM>]

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.