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

[Tor Trac #23588]: Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs() (Revision 2) #256

Closed
wants to merge 17 commits into from

Conversation

@neelchauhan
Copy link
Contributor

commented Jul 31, 2018

For the bug here.

@coveralls

This comment has been minimized.

Copy link

commented Jul 31, 2018

Pull Request Test Coverage Report for Build 3142

  • 46 of 46 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 62.328%

Totals Coverage Status
Change from base Build 3123: 0.04%
Covered Lines: 44563
Relevant Lines: 71498

💛 - Coveralls
Copy link
Member

left a comment

Hi, thanks for this revised patch.

The code looks good, but some of the comments need updating. Please add extra commits to this branch with the comment updates.

src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Show resolved Hide resolved
src/core/or/policies.c Outdated Show resolved Hide resolved
…) and hs_get_extend_info_from_lspecs()
src/core/or/policies.c Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Please initialise addresses and ports before using them.

src/core/or/policies.c Show resolved Hide resolved
src/feature/hs/hs_common.c Show resolved Hide resolved
@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch 3 times, most recently from 2d13c08 to b21f7df Aug 13, 2018
@teor2345

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

Please revert the entire commit 01ffb21. You can revert this commit on the command line using:

git revert 01ffb21
src/core/or/policies.c Outdated Show resolved Hide resolved
src/core/or/policies.h Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch from b21f7df to ccfddb4 Aug 20, 2018
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
Copy link
Member

left a comment

Hi,

Looks good, but link specifiers are tricky.
Let's copy the code from get_lspecs_from_node().

src/test/test_policy.c Outdated Show resolved Hide resolved
src/test/test_policy.c Show resolved Hide resolved
@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch from f442ccf to 2d4190f Nov 5, 2018
Copy link
Member

left a comment

Thanks for these tests. It looks like we're missing a few tests for each function.

src/test/test_policy.c Outdated Show resolved Hide resolved
src/test/test_policy.c Show resolved Hide resolved
@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch from 08547fa to 2d4190f Nov 28, 2018
@teor2345

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Hi @neelchauhan, I'm not sure what happened with the commit you just pushed.
I'm seeing the same set of commits on GitHub.

@neelchauhan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

The reason why you see the same commits with a new push is because I have two computers and I accidentally pushed an older branch on one so I had to re-push the latest copy on another to avoid losing work.

Sorry for this.

@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch from d838a4b to 08547fa Dec 1, 2018
Copy link
Member

left a comment

Just need a few tweaks to the log messages.

src/core/or/policies.c Show resolved Hide resolved
src/core/or/policies.c Outdated Show resolved Hide resolved
@neelchauhan neelchauhan force-pushed the neelchauhan:b23588a branch from 0283a9e to ce18842 Dec 3, 2018
src/core/or/policies.c Outdated Show resolved Hide resolved
src/feature/hs/hs_common.c Outdated Show resolved Hide resolved
@teor2345

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

OK, I think we're good here, I'll do the rebase and squash.

@teor2345

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Squashed in #583.

@teor2345 teor2345 closed this Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.