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 `
