Skip to content

implement whitelist option #495#496

Closed
oooo-ps wants to merge 3 commits into
ikatson:mainfrom
YGGverse:whitelist
Closed

implement whitelist option #495#496
oooo-ps wants to merge 3 commits into
ikatson:mainfrom
YGGverse:whitelist

Conversation

@oooo-ps

@oooo-ps oooo-ps commented Oct 14, 2025

Copy link
Copy Markdown

#496

Please review, before merge as untested (only passing cargo test)

P.S. Why not to use the similar Option for the blacklist also, instead of empty struct. Is this updating dynamically somewhere? I think we may have ability to update list without app reload.

@oooo-ps

oooo-ps commented Oct 14, 2025

Copy link
Copy Markdown
Author

I think the list holder could be merged to some shared struct, to not copy similar features.
But this way requires API change.

@ikatson ikatson left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Left a couple comments, but the main one is critical - don't copy-paste code please, but refactor instead.

It's fine to "break" it, as it'll all be part of 9 release which is breaking anyway

@@ -0,0 +1,278 @@
use anyhow::{Context, Result};

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

please don't copy-paste, use the same file. Feel free to rename Blocklist and blocklist.rs to "IpRanges" and "ip_ranges.rs".

Of course "is_blocked" method should be renamed to just "contains", and similar methods if they exist should be renamed too, to make the struct reusable.

.blocked_outgoing
.fetch_add(1, Ordering::Relaxed);
debug!(?addr, "blocked outgoing connection");
debug!(?addr, "blocked outgoing connection (by the blacklist)");

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

blocklist

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