Opened 4 years ago

Closed 4 years ago

#17608 closed enhancement (implemented)

Refactor accept/reject * redundancy checks out of policies_parse_exit_policy_internal

Reported by: teor Owned by:
Priority: Very Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.3-rc
Severity: Minor Keywords: easy refactor
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

policies_parse_exit_policy_internal would be a lot easier to read if the code that implements found_final_effective_entry was in its own function.

Child Tickets

Attachments (1)

17608_refactor_policies_parse_exit_policy_internal.patch (5.2 KB) - added by juce 4 years ago.
Hi, I'm new to tor development and want to start contributing as much as I can, so any feedback would be great!

Download all attachments as: .zip

Change History (5)

Changed 4 years ago by juce

Hi, I'm new to tor development and want to start contributing as much as I can, so any feedback would be great!

comment:1 Changed 4 years ago by juce

Status: newneeds_review

comment:2 Changed 4 years ago by teor

Milestone: Tor: 0.2.8.x-final

Thanks for this patch, looks great. I'm sorry I didn't get to this sooner, it's been a rather hectic 2 weeks.

I'd encourage you to submit more patches like this. I'll try hard to get to it within a week. If I miss it, you can get my attention on IRC or on the ticket.

Code review:

This is a code movement patch.
It refactors code for readability, it doesn't modify the behaviour of Tor at all.

I added a changes file. (Every change gets a changes file. They're combined into the ChangeLog with each release.)

I added a commit that modifies the patch slightly:

  • Make policies_log_first_redundant_entry take a const smartlist_t *, as it doesn't need to modify the smartlist, or the smartlist pointer. (Also assert that the list is not NULL.)
  • Make the patch conform to tor's code style. Code style can be checked using make check-spaces.
  • Keep DEFAULT_EXIT_POLICY just before the function it's used in.

Both commits are in the branch refactor-effective-entry on https://github.com/teor2345/tor.git

You can see what's in the branch (compared to master) at https://github.com/teor2345/tor/compare/refactor-effective-entry

Let's get this merged to master!
(There's no need to backport refactoring to 0.2.7.)

comment:3 Changed 4 years ago by dgoulet

It has a tiny tiny one extra space before the function call:

   policies_log_first_redundant_entry(*dest);

  if (add_default_policy) {

Apart from that, lgtm!

comment:4 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Adding a couple more cleanup commits:

  • Since the variable is no longer modified, it should be called 'policy' instead of 'dest'. ("Dest" is short for "destination".)
  • Fixed the space issue that dgoulet found above.
  • Fixed the comment a little.
  • Used SMARTLIST_FOREACH_BEGIN/END rather than for loop.

Also, merged. Thanks!

Note: See TracTickets for help on using tickets.