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

fix the program cannot be terminated through ctrl-c #2

Merged
merged 1 commit into from
Mar 12, 2023

Conversation

mozillazg
Copy link
Contributor

No description provided.

@jschwinger233
Copy link
Owner

Thanks for your PR.

Basically I have no question in this change, just want to add a point:

I'm not sure how this change can fix the non-interrupting issue. If ctrl-c doesn't stop the process, there must be somewhere blocking for a long term so that process has no change to reach the "cancel point" to check <-ctx.Done(). As far as I can see, this can only happen on the bpf syscall LookupAndDelete or fs IO skbw.Write and pcapw.WritePacket, but none of these could be fixed by this PR, could it?

@mozillazg
Copy link
Contributor Author

If ctrl-c doesn't stop the process, there must be somewhere blocking for a long term so that process has no change to reach the "cancel point" to check <-ctx.Done()

skbdump/main.go

Lines 109 to 119 in 105f01e

for {
if err := bpfObjs.DataQueue.LookupAndDelete(nil, &data); err == nil {
break
}
select {
case <-ctx.Done():
return
case <-time.After(time.Microsecond):
continue
}
}

In most cases, line 111 will always be executed and there is no way to break the first for loop. for example:

skbdump -i eth0

I can not terminate this process through ctrl-c.

@jschwinger233
Copy link
Owner

Understood, you pointed out that the process can't reach ctx check point when two bpf syscalls keep succeeding fetching events.

You're correct.

@jschwinger233 jschwinger233 self-requested a review March 12, 2023 04:10
@jschwinger233 jschwinger233 merged commit 79533b0 into jschwinger233:main Mar 12, 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