This looks great. It solves some problems with my original code that
had occured to me over the past couple of weeks, and cleans it up in
other areas. A few comments inline and below from my first
impressions...I haven't run the code yet, and haven't tried to modify
a real application to work with it, so I may have more comments soon.
Guy Harris <[EMAIL PROTECTED]> writes:
[...]
> /*
> ! * This is fun.
:-)
[...]
> ! /*
> ! * Is there already an entry in the list for this interface?
> ! */
> ! for (curdev = *alldevs; curdev != NULL; curdev = curdev->next) {
> ! if (strcmp(name, curdev->name) == 0)
> ! break; /* yes, we found it */
> ! }
This makes this algorithm O(n^2), which is unfortunate, but I couldn't
think of a better way to handle this that didn't introduce way too
much complexity when, in nearly all cases, n<10. :) Are there any
systems for which the performance of this, and of trying a
pcap_open_live() for each interface, is going to present a performance
problem?
If performance matters, caching the last entry and/or the most
recently used entry may speed things up slightly, since we'll tend to
append things to the end of the list, and are likely to get all
addresses for a single interface in a row.
[...]
> ! /*
> ! * Get a list of all interfaces that are up and that we can open.
> ! * Returns -1 on error, 0 otherwise.
> ! * The list, as returned through "alldevsp", may be null if no interfaces
> ! * were up and could be opened.
> ! */
> ! #ifdef HAVE_IFADDRS_H
> ! int
> ! pcap_findalldevs(alldevsp, errbuf)
> ! pcap_if_t **alldevsp;
> ! register char *errbuf;
I think it would be more intuitive to return pcap_if_t * here, and
return NULL for either no interfaces or for an error, since the
application is likely to react to both events in the same way. If we
set errno to 0 at the beginning, they can check that if they need to
differentiate. I don't know, though, now that I've typed that out it
sounds like it might be more of a pain than it's worth...
[...]
> ! /*
> ! * Return the name of a network interface attached to the system, or NULL
> ! * if none can be found. The interface must be configured up; the
> ! * lowest unit number is preferred; loopback is ignored.
> ! */
> ! char *
> ! pcap_lookupdev(errbuf)
> ! register char *errbuf;
> ! {
> ! pcap_if_t *alldevs, *curdev;
> !
> ! if (pcap_findalldevs(&alldevs, errbuf) == -1)
> ! return (NULL);
> !
> ! for (curdev = alldevs; curdev != NULL; curdev = curdev->next) {
> ! if (curdev->is_loopback)
> ! continue; /* ignore loopback devices */
> ! return (curdev->name);
> ! }
> ! return (NULL);
> }
2 things here.
First, since we have loopback devices sorted to the end, we don't have
to loop; we can just return the first one unless it is_loopback.
Second, we should probably call pcap_freealldevs here. This function
could be something like (untested):
char *
pcap_lookupdev(errbuf)
register char *errbuf;
{
pcap_if_t *alldevs;
char *ret;
if (pcap_findalldevs(&alldevs,errbur) == -1)
return(NULL);
if (!curdev->is_loopback && curdev->name)
ret = strdup(curdev->name);
else
ret = NULL;
pcap_freealldevs(alldevs);
return ret;
}
(perhaps with a static buffer instead of strdup())
[...]
What's the best way to handle the "all" device? If a GUI is using
this to display a list of interfaces to pick from, that should
definitely show up; arguably it should even be the default. Do we
want to stick it in the list, or just leave the application designer
to assume that if libpcap is new enough to have the interface finding
code, it will definitely have "all"?
Because some versions of libpcap will have the interface finding
routines and some older versions won't, it would be convenient to have
some define that was always set if the interface finding code was
included; that would mean an application could essentially take all of
inet.c, wrap it in a "#ifndef LIBPCAP_HAVE_FINDALLDEVS", and package
it along with their application. I don't see any precedent for this,
in libpcap, though...Any thoughts?
Would it be useful at all to hide the organization of the interface
list behind opendir(3)-like functions, such as pcap_getdevlist,
pcap_nextdev, pcap_prevdev, pcap_freedevlist, etc? That would give us
flexibility in changing how this data is stored, and make it possible
to only gather interface information as it was needed, instead of all
at once. In particular, in the case where the user wants packets from
the default device, we could in many cases avoid getting information
about the other devices.
-----ScottG.
-
This is the TCPDUMP workers list. It is archived at
http://www.tcpdump.org/lists/workers/index.html
To unsubscribe use mailto:[EMAIL PROTECTED]?body=unsubscribe
