Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

replace 'daddr' checks with 'is_valid_address()' #681

Closed
wants to merge 3 commits into from
Closed

replace 'daddr' checks with 'is_valid_address()' #681

wants to merge 3 commits into from

Conversation

martiuslim
Copy link
Contributor

@martiuslim martiuslim commented Jul 18, 2018

defined a new function is_valid_address as suggested in #656 to use Rust's ip().is_unspecified() and ip().is_multicast() functions instead of comparing against 0.0.0.0:0

@garious garious added the CI Pull Request is ready to enter CI label Jul 18, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 18, 2018
@garious garious added the CI Pull Request is ready to enter CI label Jul 18, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 18, 2018
@garious
Copy link
Contributor

garious commented Jul 18, 2018

Thanks! You'll need to run the latest cargo fmt and cargo +nightly clippy. Looks like fmt will convert some tabs to spaces and clippy should ask you to move those redundant parentheses.

@aeyakovenko
Copy link
Member

aeyakovenko commented Jul 18, 2018

this looks great! Can you add test as well, something that does a

let bad_address = "0.0.0.0:1".parse().unwrap();
assert!(!Crdt::is_valid_address(bad_address));

for port 0, multicast, unspecified...

@martiuslim
Copy link
Contributor Author

@garious running cargo fmt causes 59 files to be changed for some reason (although in all of them, my diff tool doesn't show any changes 😮). Should I just run rustfmt on the relevant files changed in this pr? Also, running cargo +nightly clippy didn't flag anything on my machine and finished successfully. Am I doing something wrong?

@garious
Copy link
Contributor

garious commented Jul 19, 2018

It's possible I'm pickier than Clippy. :) Push the patch with the formatted code and let's see what CI says about it.

@garious garious added the CI Pull Request is ready to enter CI label Jul 19, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 19, 2018
@garious
Copy link
Contributor

garious commented Jul 19, 2018

Looks good, but needs to be rebased.

@aeyakovenko aeyakovenko added the CI Pull Request is ready to enter CI label Jul 19, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 19, 2018
@aeyakovenko aeyakovenko added the automerge Merge this Pull Request automatically once CI passes label Jul 20, 2018
@solana-grimes solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Jul 20, 2018
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to merge conflict

@garious garious added CI Pull Request is ready to enter CI automerge Merge this Pull Request automatically once CI passes labels Jul 20, 2018
@solana-grimes solana-grimes removed CI Pull Request is ready to enter CI automerge Merge this Pull Request automatically once CI passes labels Jul 20, 2018
@solana-grimes
Copy link
Contributor

💔 Unable to automerge due to CI failure

@mvines
Copy link
Contributor

mvines commented Jul 20, 2018

automerge is a little busted right now, haven't had a chance to debug why. I'll keep eye on this PR and merge once CI is green in the meantime

@mvines
Copy link
Contributor

mvines commented Jul 20, 2018

Close, cargo fmt is just unhappy about one trailing space in this patch:

cargo fmt -- --write-mode=check
Diff in /solana/src/crdt.rs at line 1919:
         assert!(!me.alive.contains_key(&node_with_same_addr.id));
         assert!(me.alive[&node_with_diff_addr.id] > 0);
     }
-
+
     #[test]
     fn test_is_valid_address() {
         let bad_address_port = "127.0.0.1:0".parse().unwrap();

@garious
Copy link
Contributor

garious commented Jul 20, 2018

Will merge in #721 after CI passes.

@garious garious closed this Jul 20, 2018
vkomenda pushed a commit to vkomenda/solana that referenced this pull request Aug 29, 2021
Bumps [rollup](https://github.com/rollup/rollup) from 2.32.0 to 2.32.1.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v2.32.0...v2.32.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants