Conversation
lmb
force-pushed
the
windows-ringbuf
branch
2 times, most recently
from
70059f5 to
1c86ca9
Compare
Signed-off-by: Lorenz Bauer <[email protected]>
|
@florianl please let me know if you want to review this! |
florianl
left a comment
florianl
left a comment
There was a problem hiding this comment.
Just minor comments. As I don't have access to Windows, I can't test that part.
| "github.com/cilium/ebpf/internal" | ||
| "github.com/cilium/ebpf/internal/efw" | ||
| ) | ||
|
|
There was a problem hiding this comment.
To guarantee compatibility between windows and others, maybe we should have an interface for poller and make sure the respective implementations are compatible.
| @@ -0,0 +1,75 @@ | |||
| package ringbuf | |||
There was a problem hiding this comment.
For consistency, maybe add a build constraint
| package ringbuf | |
| //go:build windows | |
| package ringbuf |
There was a problem hiding this comment.
That is redundant with the file name, no?
|
|
||
| prog, events := mustOutputSamplesProg(t, | ||
| sampleMessage{size: 5, flags: sys.BPF_RB_NO_WAKEUP}, // Read after timeout | ||
| sampleMessage{size: 6, flags: sys.BPF_RB_NO_WAKEUP}, // Discard |
There was a problem hiding this comment.
I'm wondering, where the discard case was dropped.
There was a problem hiding this comment.
Discard is currently unsupported on windows, so the helper issues a skip. It’s important to test the wake-up behaviour on windows, and the discard here doesn’t change the observed behaviour at all. So dropping it is the easiest solution.
| @@ -0,0 +1,105 @@ | |||
| package ringbuf | |||
There was a problem hiding this comment.
For consistency, maybe add a build constraint
| package ringbuf | |
| //go:build windows | |
| package ringbuf |
|
|
||
| "github.com/cilium/ebpf/internal/unix" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Similar to poller, maybe there should also be an interface for ringbufEventRing so we can make sure all implementations fit here.
|
Thanks! Made an issue for the interface changes, since I don’t have access to a laptop at the moment. |
2 participants
No description provided.