Re: [clearview-discuss] libdlpi with libpcap

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

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.