Opened 5 years ago

Closed 4 years ago

#12376 closed enhancement (implemented)

Refactor and unit-test all the code that looks at network interfaces to get current IP

Reported by: rl1987 Owned by: rl1987
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: refactoring 026-triaged-1 026-defferrable
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now Tor has large hairy functions such as get_interface_address6() and get_interface_addresses_raw() that attempt to use system-specific API to try and get an IP address of network interface Tor instance is currently using. They should be refactored to improve code readability and maintainability. Also, we need to make sure there are unit tests for this kind of code.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:2 Changed 5 years ago by rl1987

Owner: set to rl1987
Status: newassigned

comment:3 Changed 5 years ago by rl1987

I have covered most of resolve_my_address() function with tests:

https://github.com/rl1987/tor/tree/resolvemyaddr

Also, I have done some refactoring and unit testing regarding the rather messy get_interface_addresses_raw() function:

https://github.com/rl1987/tor/tree/if_addr_refactoring

What should I do next? What should be in the checklist of things to get done to get the above changes merged into Tor master?

comment:4 Changed 5 years ago by nickm

Status: assignedneeds_revision

Sorry about the delays!

reviewing tesolvemyaddr branch:

  • tor_gethostname,(char *name, size_t namelen)) needs documentation.
  • mock functions in test_config.c should documentation (eg, "This is a mock function to replace X; this replacement does Y.")
  • Otherwise, this looks fine, assuming test coverage is okay

reviewing if_addr_refactoring branch:

  • The tests look good, assuming coverage is okay
  • Instead of duplicating the "+#ifdef HAVE_GETIFADDRS ...#elif define(_WIN32)) ...#elif defined(SIOCGIFCONF) && defined(HAVE_IOCTL) " checks in a few places, it's probably better to have address.h do "#define HAVE_IFADDRS_TO_SMARTLIST" in the first case, and #define HAVE_IP_ADAPTER_ADDRESSES_TO_SMARTLIST in the second case, and so on. Then the other code can check those more readable macros.
  • All the new functions in address.c need documentation.
  • Otherwise, looks good!

(For more info on our documentation style, and on getting test coverage, see doc/HACKING. )

comment:5 Changed 5 years ago by nickm

Keywords: 026-triaged-1 026-defferrable added

comment:6 Changed 4 years ago by rl1987

I believe I now have a 100 percent unit test coverage of resolve_my_address(). See resolvemyaddr branch.

comment:7 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

comment:8 Changed 4 years ago by nickm

I made a bunch of fixes in my if_addr_refactoring branch in my public repository. Still looks okay to you?

Looking at resolvemyaddr now.

comment:9 Changed 4 years ago by nickm

resolvemyaddr looks fairly solid. I made a few tweaks and merged. Thanks!

(Leaving this ticket open for the other branch.)

comment:10 Changed 4 years ago by nickm

(waiting on windows unit tests)

comment:11 Changed 4 years ago by rl1987

I have covered the Windows specific code with unit tests. New unit-tests are passing both on Wine and on Windows 7. See my if_addr_refactoring branch.

comment:12 Changed 4 years ago by rl1987

The if_addr_refactoring branch is ready for review.

comment:13 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good; let's try it out!

(squashed and merging to master.)

Note: See TracTickets for help on using tickets.