Re: [clearview-discuss] libdlpi with libpcap

[sagun shakya <Sagun.Shakya () sun com>]

Due to other work priority I wasn't able to work on fixing and addressing these comments.  I have finally addressed the comments from Meem and made a few other changes. My comments are inline and please find an update webrev at: 

http://cr.opensolaris.org/~sagun/libpcap/

If you'd like to see the changes made from the last round of review you can find a webrev at:

http://cr.opensolaris.org/~sagun/codereview_fix/


-Thanks,

Sagun


Peter Memishian wrote:
> I took a quick look at this.  Some offhand observations:
>
>         * There's too much duplicated code betwen pcap-libdlpi.c and
>           pcap-dlpi.c.  For instance, both strioctl() and the pcap_stats()
>           functions are identical, as is the bufmod-related logic
>           (starting with "Loop through packets") in pcap_read(), and
>           the bufmod configuration in pcap_open_live().  I'd move the
>           relevant bits of logic to a new pcap-streams.c file and call
>           those routines from pcap-*dlpi.c.
>   
Created a new file pcap-streams.c and moved common functions into this 
file.
>         * The linklist structure should be renamed; it's too close to
> 	  "linked list". 
>   
Changed to linkname_list.
>         * The link chaining logic in list_interfaces() doesn't seem like
>           it would work for more than two links.  In particular, seems
>           like you need:
>
>                 if (lwp->lw_list == NULL) {
>                         lwp->lw_list = entry;
>                 } else {
>                         entry->ll_next = lwp->lw_list;
>                         lwp->lw_list = entry;
>                 }
>
>           (Also, there's no need to initialize `entry' or cast `lwp'.)
>   
Fixed.
>         * In pcap_read_libdlpi(), I'm not sure what the special error
>           handling after dlpi_recv() is about.  It looks to be some
>           holdover from the old pcap-dlpi.c code, but I can't see how it
>           applies to dlpi_recv() since dlpi_recv() only calls getmsg()
>           when poll() has indicated there's data to read, and thus
>           it should never get EINTR or EAGAIN -- and if it did, that is
>           an issue that should be handled inside dlpi_recv() itself.
>   
The special error handling was left so that when the read operation was 
terminated a warning/error message wouldn't be printed. But a case fo 
EAGAIN isn't needed so I've changed it to handle only EINTR.
>         * Seems error handling would be cleaned up quite a bit by
>           introducing a utility function like:
>
>           static void
>           pcap_libdlpi_err(dlpi_handle_t *dh, const char *func, int err,
>               char *errbuf);
>
>   
Added as well as a similar function in pcap-streams.c
>         * Please check for spelling errors -- e.g. "promsic" on lines 330
>           and 341.
>
>         * In pcap_open_live(), `flags' should be removed and DLPI_RAW |
>           DLPI_PASSIVE directly passed into dlpi_open().  Also, the
>           comment above dlpi_open() should be updated to say something
>           more appropriate like "Enable Solaris raw and passive DLPI
>           extensions; see the Solaris dlpi(7P) manpage for details".
>
>         * Surely line 317 is supposed to set `retv' so that line 320 will
>           print something meaningful?
>
>         * Would be good to be consistent with either `ret' or `retv' as
>           a variable to house the return value.
>   

All of the above have been fixed.

-
This is the tcpdump-workers list.
Visit https://cod.sandelman.ca/ to unsubscribe.