tcpdump mailing list archives
Re: [clearview-discuss] libdlpi with libpcap
From: sagun shakya <Sagun.Shakya () sun com>
Date: Sun, 27 Jan 2008 20:53:11 -0500
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:
Created a new file pcap-streams.c and moved common functions into this file.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.
* 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.
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.* 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.
* 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.
Current thread:
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 27)
- Re: [clearview-discuss] libdlpi with libpcap Peter Memishian (Jan 28)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 28)
- Re: [clearview-discuss] libdlpi with libpcap Peter Memishian (Jan 29)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 28)
- Re: [clearview-discuss] libdlpi with libpcap Guy Harris (Jan 29)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 29)
- Re: [clearview-discuss] libdlpi with libpcap Peter Memishian (Jan 30)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 28)
- Re: [clearview-discuss] libdlpi with libpcap Guy Harris (Jan 30)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 30)
- Re: [clearview-discuss] libdlpi with libpcap sagun shakya (Jan 30)
- Re: [clearview-discuss] libdlpi with libpcap Peter Memishian (Jan 28)