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

Add option to exclude IPs #341

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ilyes-ced
Copy link
Contributor

fixing old PR

solves issue #283
adds possibility to exclude an ip address x.x.x.x or an ip address with a port x.x.x.x:xxxx
can also exclude multiple ips at the same time with bandwitch -e x.x.x.x -e y.y.y.y --excluded-ipv4-port z.z.z.z:zzzz

Copy link
Collaborator

@cyqsimon cyqsimon left a comment

Choose a reason for hiding this comment

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

I'm probably going to be pretty critical in this review, so I would like to first clarify that your effort is greatly appreciated. Out of all our users, maybe 10% are willing to report bugs; maybe 5‰ are willing to submit PRs. So thank you. Added to that, Rust is not the easiest language to learn; in fact I dare say that most people (>80%) who talk about Rust have never written a single line. So I applaud you for your willingness to try something unfamiliar.

With that out of the way, let's get straight to it. Hopefully it's not too discouraging!

src/cli.rs Outdated
Comment on lines 44 to 53
#[arg(short, long)]
/// exclude ip addres with <-e x.x.x.x>
/// exclude multiple ip addresses with <-e x.x.x.x -e y.y.y.y>
pub excluded_ipv4: Option<Vec<Ipv4Addr>>,

#[arg(short = 'E', long)]
/// exclude ip addres with <-e x.x.x.x:zzzz>
/// exclude multiple ip addresses and port with <-e x.x.x.x:zzzz -e y.y.y.y:zzzz>
pub excluded_ipv4_port: Option<Vec<SocketAddrV4>>,

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is pretty rigid and non-extensible. For example, it's pretty reasonable for a user to want to exclude by hostname. Or by CIDR. Do we need an option for each? And then double that if we want to support IPv6 (which we definitely do; it's not 2003).

Much better would be to declare a struct/enum HostFilter (or something like that) and implement FromStr and Display for it so that it can be used as a clap option parameter. Then all these different kinds of possible filters can be united under a single option.

src/main.rs Outdated
Comment on lines 141 to 147
let mut utilization = { network_utilization.lock().unwrap().clone_and_reset() };
if let Some(ref ex) = opts.excluded_ipv4 {
utilization.remove_ip(ex)
}
if let Some(ref ex) = opts.excluded_ipv4_port {
utilization.remove_ip_port(ex)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to do this here. IMO there are two ways to achieve this logic and this is neither:

  1. Apply the filtering in the sniffer threads, so that the filtered traffic is never accounted.
  • This is actually not my preferred option. Sniffers are not "hostname-aware", and therefore cannot apply hostname filters if we want to support those.
  1. Apply the filtering while rendering. But then this logic belongs in UIState::update instead.

Comment on lines 49 to 51
// might be possible to refactor this part better
// i still don't understand the whole borrow/own system very well yet
let placeholder = self.connections.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see why you did this. Probably got yelled at by the borrow checker (cannot borrow as mutable because it is also borrowed as immutable)? If so, this is because you tried to remove from the HashMap (there's your mutable borrow) while holding a reference to one of its items (there's your immutable borrow).

The API that will allow you to do this without cloning is HashMap::entry.

Copy link
Collaborator

@cyqsimon cyqsimon Dec 6, 2023

Choose a reason for hiding this comment

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

All this being said, if you are doing this filtering in UIState::update (as previously mentioned), you likely won't need any of this. On a high level, this is because you are already copying data (from memory to the screen), so all you need is to tell it what to copy (i.e. a filter). Then there's no mutability issues involved at all.

Personally I think reducing mutability as much as possible is the best practise regardless of language.

@cyqsimon cyqsimon changed the title Add excluded ips (repost) Add option to exclude IPs Dec 6, 2023
@ilyes-ced
Copy link
Contributor Author

that was not discouraging, thank you for taking the time to write this review so i can know where i am wrong
i will try to fix the mistakes

@ilyes-ced
Copy link
Contributor Author

i made it the way you asked
ipV4 / ipV6 / socketV4 / socketV6 are filtred correctly at the start of ui_state::update (filtered out of network_utilization)
but i am yet to implement hostname filtering because i don't know where it is

@ilyes-ced
Copy link
Contributor Author

added hostname filtering as well but it only filters ipV4 and not V6 , if you have any clue about how i can include V6 as well please tell me
the way it does it is by reading ip_to_host in ui::update_state and adding the ip of the excluded hostname to the UIState.excluded_ips

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