pcap: make stoppable when no packets to sniff

[jcrussell]

In newer versions of libpcap (1.8.1), the read timeout will not cause
the read to return if there are no packets to sniff. To fix this, I
changed the pacp handle to non-blocking and added a new helper,
pcap_wait, that uses select to wait up to the read timeout for
packets. I have tested that it has the desired behavior but I have not
benchmarked it to determine how much of a slowdown, if any, there is.

Fixes #253.

[gconnell]

Instead of a stop channel here, let's use an atomic. From quick benchmarking, they appear to be around 4x faster:

package main

import (
        "sync/atomic"
        "testing"
)

func BenchmarkSelectBool(b *testing.B) {
        c := make(chan bool)
        for i := 0; i < b.N; i++ {
                select {
                case <-c:
                default:
                }
        }
}
func BenchmarkSelectStruct(b *testing.B) {
        c := make(chan struct{})
        for i := 0; i < b.N; i++ {
                select {
                case <-c:
                default:
                }
        }
}
func BenchmarkAtomic(b *testing.B) {
        x := uint64(0)
        for i := 0; i < b.N; i++ {
                if atomic.LoadUint64(&x) != 0 {
                        b.Fatal("oops")
                }
        }
}

gives this result:

gconnell@rockyduck:~$ go test select_test.go --test.bench=.
testing: warning: no tests to run
BenchmarkSelectBool-12          200000000                9.10 ns/op
BenchmarkSelectStruct-12        200000000                9.10 ns/op
BenchmarkAtomic-12              1000000000               2.51 ns/op
PASS
ok      command-line-arguments  8.305s

[jcrussell]

Nice, updated.

[gconnell]

Let's do the call to pcap_next_ex FIRST in the loop, then fall back to the timeout only if we don't have a packet.

for atomic.LoadUint32(&p.stop) == 0 {
  result = NextError(pcap_next_ex(...))
  if (we should error) {
    return err
  } else if (we have packet) {
    return packet
  }
  pcap_wait(...)
}

This way, we don't call an extraneous (expensive) select syscall in between every packet, only if there isn't one already on the queue.

[jcrussell]

Rewrote the core loop as you suggested.

[gconnell]

to be a little more self-documenting, let's say: if p.timeout == BlockForever

[gconnell]

According to the man pages, pcap_get_selectable_fd() is not available on Windows.

Folks do use this for winpcap, so we'll need to hande that eventuality.

[jcrussell]

Should I update the package documentation to note this? Or should we check runtime.GOOS and do something else?

[gconnell]

If only things were that simple... :(

Breaking Windows users is a non-starter, so we need to write code that compiles for both Windows and others. Here, this probably means splitting the implementation up into different files and using https://golang.org/pkg/go/build/ build constraints to specify Windows vs. non-Windows code.

In short, we'll probably end up with three files: pcap.go, for common stuff. pcap_win.go, with the windows implementation of Windows-specific functionality. pcap_other.go with non-windows stuff.

It would have been really great if libpcap hadn't decided to do backwards-incompatible changes.

[jcrussell]

I added separate files for windows/non-windows. From the standard library, there was one example of a +build !windows and the filename was _unix.go instead of _other.go.

In the Windows version, I left the pcap handle in blocking mode -- it should work at least as well as the existing code.

[gconnell]

According to http://www.tcpdump.org/manpages/pcap_get_selectable_fd.3pcap.html:

select() and poll() do not work correctly on BPF devices; pcap_get_selectable_fd() will return a file descriptor on most of those versions (the exceptions being FreeBSD 4.3 and 4.4), but a simple select() or poll() will not indicate that the descriptor is readable until a full buffer's worth of packets is received, even if the read timeout expires before then. To work around this, an application that uses select() or poll() to wait for packets to arrive must put the pcap_t in non-blocking mode, and must arrange that the select() or poll() have a timeout less than or equal to the read timeout, and must try to read packets after that timeout expires, regardless of whether select() or poll() indicated that the file descriptor for the pcap_t is ready to be read or not. (That workaround will not work in FreeBSD 4.3 and later; however, in FreeBSD 4.6 and later, select() and poll() work correctly on BPF devices, so the workaround isn't necessary, although it does no harm.)

Does this mean that in FreeBSD 4.3-4.6, this code will fail to work? Or that it will fail to work in FreeBSD 4.3 on?

[jcrussell]

I interpret that as meaning that it won't work on FreeBSD 4.3-4.6.

Another thing to note in the package documentation? Is it worth trying to test?

[gconnell]

Sorry to take so long on this, but if you're still up for this and want to merge up to head, I can take another look and see if we can push this through.

[avgerin0s]

Any news on this one?

[jcrussell]

@gconnell: merged to master -- sorry for the delay.

[gconnell]

Hey, jcrussel, I still get "This branch cannot be rebased due to conflicts"... are you sure you sync'd to the most current master?

[gconnell]

Oh, it looks like you just may not have pushed the merge up to your pull request branch...

[jcrussell]

Hm, I did merge (25a8a4c).

Did you want me to rebase it instead? I think that breaks the PR though, doesn't it?

[gconnell]

Never mind, apparently my merge worked fine. I just have my merge/rebase dropdown set to rebase by default, and it was showing me an error.

[avgerin0s]

@jcrussell @gconnell Thanks both. Any ideas when this is going to be released? It's been a long time since one (see #331)

[gconnell]

Apologies for being horrible at actual releases/tagging. Just created v1.1.13 at head.

[avgerin0s]

@gconnell Thanks for releasing. closed #331