Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move non-windows stuff to pcap_unix; Convert windows to implicit linking #568

Merged
merged 10 commits into from
Jan 4, 2019

Conversation

notti
Copy link
Collaborator

@notti notti commented Nov 27, 2018

This add support for npcap on windows.
Windows now supports both npcap and winpcap at the same time. If both
winpcap and npcap are installed at the same time, npcap is preferred.
Building on windows now doesn't need neither cgo nor npcap/winpcap sdks.

The way this works is that all the non-windows stuff is now in pcap_unix.go, while all the windows stuff is in pcap_windows.go. Those two files provide a go-only internal api to pcap.go which to the outside looks the same as before (except for the small change mentioned below).
The new windows code now doesn't use cgo (this is a pain to use on windows) and uses implicit linking (I couldn't get delay loading to work with cgo). => neither cgo nor npcap/winpcap sdk is needed. syscall.Syscall is used for all the calls and the wpcap.dll is loaded in init(). Since both npcap and winpcap provide a wpcap.dll with libpcap api - no distinctions between the two are necessary (except for the not supported api functions in older libpcap versions as before). The only place where npcap is used, is an additional ddlload path that is added before loading the actual wpcap.dll.

Gains: Now the pcap tests work on windows (only with npcap - with winpcap some of the tests fail due to outdatet libpcap (e.g. no pcapng support, ...))

API of pcap.go is the same as before except for one small change:

func DatalinkNameToVal(name string) C.int {

The function signature of this one seems to be wrong and slipped through the cracks (all related functions (e.g., DatalinkValToName) accept int as value and not C.int
Additionally, I think C.*-types should never be exposed and this seems like a mistake.
If this is bad since it might break something, I can change it back.

Ad performance: the new version has an additional function call in getNextBufPtrLocked since pcapNextPacketEx can't be inlined. But the new ReadPacketData is faster since it doesn't use C.GoBytes any longer (which is a call into C code! - I think this function is supposed to be used from within C and not go...)
Testing with the gopacket_benchmark was inconclusive.

Ad windows: Right now there is no support to select winpcap if both npcap and winpcap are installed... I don't know if there would be a good way to do that.

This add support for npcap on windows.
Windows now supports both npcap and winpcap at the same time. If both
winpcap and npcap are installed at the same time, npcap is preferred.
Building on windows now doesn't need neither cgo nor npcap/winpcap sdks.
This fixes google#500
On windows network device names for libpcap and net don't match :(
@negbie
Copy link
Contributor

negbie commented Nov 29, 2018

Finally I can remove this bookmark ;)
https://stackoverflow.com/questions/38047858/compile-gopacket-on-windows-64bit

Thank you very much @notti !!

@notti
Copy link
Collaborator Author

notti commented Nov 30, 2018

Happy to help.
If you encounter issues, or it's working for you, please add a comment.
I could only test on windows 7 64 bit (with both winpcap and npcap, tests that are included in gopacket, example programs, and some some smoke tests) - as well as linux.

Copy link
Collaborator

@gconnell gconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notti, this is truly phenomenal work. Thanks a ton for working on this!

@gconnell
Copy link
Collaborator

gconnell commented Jan 4, 2019

Looks great! Merging in now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants