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

CMake, BSD: Fix BPF detection, use clonable device #90

Merged
merged 5 commits into from
Apr 18, 2023

Conversation

klemensn
Copy link
Contributor

@klemensn klemensn commented Apr 18, 2023

  • CMake: Typofix BSD bpf guard
  • BSD: bpf(4) is clonable, use /dev/bpf
  • CMake: Fix net/bpf.h detection on OpenBSD
  • CMake: BSD: Fix bpf detection
  • CMake: Linux: Pick BPF last

On OpenBSD, NetBSD and FreeBSD this device is clonable, i.e. the same device
may be opened multiple times and there is no need to find an exclusive bpfN.

/dev/bpf0 exists on OpenBSD (and certainly other systems) as mere fallback,
but bpf[1-N] do not;  they're the same anyway.
```
crw-------  1 root  wheel   23,   0 Apr 11 16:01 /dev/bpf
crw-------  1 root  wheel   23,   0 Apr 11 16:01 /dev/bpf0
```

Simplify code and match bpf(4) handling in existing BSD userland tools.
@klemensn
Copy link
Contributor Author

There's more to fix in the CMake code and it makes sense to have it in one PR, so converting it to a draft until I have everything...

@klemensn klemensn marked this pull request as draft April 18, 2023 11:41
https://man.openbsd.org/bpf.4 explains how `<net/bpf.h>` usage requires
other headers as well, but CMake's `check_include_file()` only tests the
single header alone, thus it fails to detect bpf(4) on at least OpenBSD.

Pull BPF out of the generic loop and add `<sys/types.h>` to unbreak test
compilation.
https://cmake.org/cmake/help/latest/command/find_file.html as well as the
simpler `if (EXISTS ...)` expect files to be readable (i.e. `test -r`).

At least on OpenBSD, thus bpf detection fails unless cmake is privileged:
```
crw-------  1 root  wheel   23,   0 Apr 11 16:01 /dev/bpf
```

Rely on usual C header tests to discover bpf(4) instead.
This should unbreak bpf detection on other BSDs as well.
BPF exists on both Linux and BSD.
Make `src/eth-bsd.c` least preferred, so that `src/eth-linux.c` is used
whenever possible on Linux.
@klemensn klemensn changed the title BSD: bpf(4) is clonable, use /dev/bpf CMake, BSD: Fix BPF detection, use clonable device Apr 18, 2023
@klemensn klemensn marked this pull request as ready for review April 18, 2023 14:44
@klemensn
Copy link
Contributor Author

This gets src/eth-bsd.c to be used instead of src/eth-none.c on OpenBSD, as expected.
Linux should still pick src/eth-linux.c, but I haven't built there.

/dev/tun0 detection is equally broken and can use the same fix, but that should be another PR.

@ofalk
Copy link
Owner

ofalk commented Apr 18, 2023

Awesome @klemensn - happy someone is looking into the BSD part, because I have none to my avail at the moment !
May I consider this read to be merged?

@klemensn
Copy link
Contributor Author

Awesome @klemensn - happy someone is looking into the BSD part, because I have none to my avail at the moment ! May I consider this read to be merged?

Your call -- if this looks good enough, I'll fix other CMake stuff as well until OpenBSD is happy enough that I can consider switching upgrading our official net/libdnet port/package and switch it over to from GNU auto* tooling.

Works (better) for me, I don't see how this would make it worse for any other system...

@ofalk
Copy link
Owner

ofalk commented Apr 18, 2023

Awesome @klemensn - happy someone is looking into the BSD part, because I have none to my avail at the moment ! May I consider this read to be merged?

Your call -- if this looks good enough, I'll fix other CMake stuff as well until OpenBSD is happy enough that I can consider switching upgrading our official net/libdnet port/package and switch it over to from GNU auto* tooling.

Works (better) for me, I don't see how this would make it worse for any other system...

Builds fine on Linux and it looks fine. I'll merge it in a sec - this time as merge commit - not squash it :-)

@ofalk ofalk merged commit 8a0163b into ofalk:master Apr 18, 2023
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.

2 participants