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

Handle disconnected interfaces (and periodically attempt to re-connect) #191

Merged
merged 4 commits into from
Sep 29, 2020
Merged

Handle disconnected interfaces (and periodically attempt to re-connect) #191

merged 4 commits into from
Sep 29, 2020

Conversation

thepacketgeek
Copy link
Contributor

@thepacketgeek thepacketgeek commented Sep 25, 2020

Addresses #150, which I was able to repro on a Mac by disconnecting an Ethernet dongle during sniffing. The timeout suggestion fix works and I'd like some guidance around the following:

Timeout selection
Currently, there's no timeout so Sniffer.next() is called in rapid succession. I used a timeout of 10 ms here as it's a good middle ground for keeping sniffing real-time and not spinning too much on a quiet interface.

Interface disconnects handled silently
There's no more CPU spike @ 100% after an interface is disconnected. The thread will silently continue to re-establish the channel for that interface. Should this be handled silently and should bandwhich periodically attempt to reconnect (currently at the same 1 second interval of display updates)

ezgif-7-738d31e02fe0

@thepacketgeek thepacketgeek changed the title Panic on non-Timeout errors in packet Sniffer Handle disconnected interfaces (and periodically attempt to re-connect) Sep 25, 2020
Sniffer::next() now returns an io::Result so the thread in main can determine between no packets & a sniffing error
- Matching on EtherType to remove duplicate code determining IP Version

Added Sniffer::reset_channel to allow main to poll a previously connected interface
@imsnif
Copy link
Owner

imsnif commented Sep 26, 2020

Hey @thepacketgeek, this looks like a good direction.

Currently, there's no timeout so Sniffer.next() is called in rapid succession. I used a timeout of 10 ms here as it's a good middle ground for keeping sniffing real-time and not spinning too much on a quiet interface.

I think this would work well, seeing as it will only sleep if the network interface becomes unavailable. I think this is a relatively rare occurrence all-in-all.

There's no more CPU spike @ 100% after an interface is disconnected. The thread will silently continue to re-establish the channel for that interface. Should this be handled silently and should bandwhich periodically attempt to reconnect (currently at the same 1 second interval of display updates)

I think handling it silently is fine. Maybe in the future we can develop a warning box, interface status line or some such, but right now I feel this is out-of-scope information for the tool.

As for the PR itself:
I glanced over it briefly, and it seems to me we're doing some extra work because we're handling the sleep in main. I feel this change can be much smaller if we handle it in sniffer.rs. Then we can still return an option and sleep within the iterator (with a while loop or some such maybe?). The only thing we'd then have to find a solution for is the case of quitting the app and closing the thread while the interface is still unavailable. Unless I'm missing something of course. :) What do you think?

@thepacketgeek
Copy link
Contributor Author

Thanks for the feedback @imsnif! I changed Sniffer::next() back to returning an Option<Segment> with one tradeoff from the prior commit:

The thread is parked prior to returning None. This works as the thread will call Sniffer::next() immediately after this function returns None.

Err(err) => match err.kind() {
    std::io::ErrorKind::TimedOut => {
        park_timeout(PACKET_WAIT_TIMEOUT);
        return None;
    }
    _ => {
        park_timeout(CHANNEL_RESET_DELAY);
        self.reset_channel().ok();
        return None;
    }
},

@imsnif
Copy link
Owner

imsnif commented Sep 28, 2020

@thepacketgeek - I saw your fixes, my apologies for not getting to them yet. I'm going to take a look tomorrow.

Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Hey, so thanks with bearing with my slightly hectic week. :)

This change looks great. I left one small nitpick (I'd have removed it myself but wanted to make sure with you it wasn't something I'm missing). Once we address that, I'll merge this.

src/tests/cases/raw_mode.rs Outdated Show resolved Hide resolved
@thepacketgeek
Copy link
Contributor Author

Thanks for the review @imsnif!

@imsnif
Copy link
Owner

imsnif commented Sep 29, 2020

Awesome, thanks for this @thepacketgeek!

@imsnif imsnif merged commit 0737704 into imsnif:main Sep 29, 2020
@thepacketgeek thepacketgeek deleted the iface-disconnect branch September 29, 2020 16:12
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