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

ksniff graceful shutdown on Ctrl+C + Wireshark doesn't close on exit #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aviramha
Copy link

@aviramha aviramha commented Mar 21, 2020

Thanks for the awesome plugin Eldad!
One annoying UX I had is if I close ksniff because I had enough traffic it also closes Wireshark.
I changed the flow so that Wireshark stays open and needed to add handling of Ctrl+C so it'll be a valid exit scenario.
This seems to fix a bug (#53 ) on capturing to a file, whereas I couldn't find a way to exit in a nice manner.

It's my first Go coding so feel free to fire away.

@aviramha
Copy link
Author

@eldadru Bump

@eldadru
Copy link
Owner

eldadru commented Mar 31, 2020

Hi @aviramha - sorry for the delay, had a rough time..

I'll review your code today, thank you so much for this contribution!

pkg/cmd/sniff.go Outdated Show resolved Hide resolved
pkg/cmd/sniff.go Outdated Show resolved Hide resolved
Copy link
Owner

@eldadru eldadru left a comment

Choose a reason for hiding this comment

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

See my comments

@aviramha
Copy link
Author

Done! Check it out :)

@aviramha aviramha requested a review from eldadru March 31, 2020 20:10
@eldadru
Copy link
Owner

eldadru commented Mar 31, 2020

Thanks, did you push the new code? I don't see new commits

@aviramha aviramha force-pushed the better_exit_and_detached_wireshark branch from a7e93a1 to 8d8d5bc Compare March 31, 2020 20:43
@eldadru
Copy link
Owner

eldadru commented Mar 31, 2020

It seems the windows build is broken

pkg/cmd/sniff.go:356:4: unknown field 'Setpgid' in struct literal of type syscall.SysProcAttr pkg/cmd/sniff.go:357:4: unknown field 'Pgid' in struct literal of type syscall.SysProcAttr Makefile:26: recipe for target 'windows' failed make: *** [windows] Error 2 The command "make all" exited with 2.

@aviramha
Copy link
Author

aviramha commented Apr 1, 2020

Should I create a _linux and _windows file for the specific procedure? (not sure what's the go approach here)

@eldadru
Copy link
Owner

eldadru commented May 9, 2020

Hi @aviramha, sorry again for the late response...

In this case, I believe the cleaner approach is to extract the process execution code to its own struct and hide the "specific os code" there, the struct will expose a single call to execute the Wireshark command.

WDYT?

@aviramha
Copy link
Author

Sounds good, from what I understand specific OS code can be seperated only by files, so I assume that we'll have windows file and unix file?

@Slach
Copy link

Slach commented Sep 30, 2020

@aviramha any news about this PR?

@aviramha
Copy link
Author

@aviramha any news about this PR?

I don't think I'll get to it soon. Feel free to continue where I left it :)

@bostrt
Copy link
Collaborator

bostrt commented Jan 21, 2021

Hey @aviramha, your hard work so far is very much appreciated! I just wanted to check-in and see if you think you'd be able to continue work on this PR?

@aviramha
Copy link
Author

I don't think so

@pnovotnak
Copy link

I can take this over, I spend a few mins each day killing off tcpdump binaries (let alone the ones I forget) so I may as well just spend an hour up front.

In this case, I believe the cleaner approach is to extract the process execution code to its own struct and hide the "specific os code" there, the struct will expose a single call to execute the Wireshark command.

@bostrt you're saying the contents of the (o *Ksniff) Run() function ought to be hidden in a struct, yea?

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.

5 participants