Opened 3 years ago

Closed 3 years ago

#20634 closed defect (fixed)

Unit test address/get_if_addrs6_list_no_internal should succeed if there are only internal addresses

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: easy intro test CoreTorTeam201611
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

Split off #19960:

It is ok for the log message generated by address/get_if_addrs6_list_no_internal to be either:

  • "connect() failed", or
  • "unable to create socket", or
  • "Address that we determined via UDP socket magic is unsuitable for public comms."

But we don't currently allow the third option.

address/get_if_addrs6_list_no_internal: [forking] 
  FAIL src/test/test_address.c:850: expected log to contain "connect() failed" o
r "unable to create socket"  Captured logs:
    1. err: "Address that we determined via UDP socket magic is unsuitable for p
ublic comms.\n"

  [get_if_addrs6_list_no_internal FAILED]

Child Tickets

Attachments (5)

0001-Add-error-checking-for-error-Address-that-we-determi.patch (2.6 KB) - added by neel 3 years ago.
Patch to add error for "Address that we determined via UDP socket magic is unsuitable for public comms."
tor_test_output.log (32.4 KB) - added by neel 3 years ago.
Log file for my patch for "Address that we determined via UDP socket magic is unsuitable for public comms."
tor_patch_get_if_addrs6.patch (4.2 KB) - added by neel 3 years ago.
Updated patch
tor_patch_get_if_addrs6_2.patch (5.3 KB) - added by neel 3 years ago.
Patch with getsockname() error reason to test_address_get_if_addrs6_list_internal()
tor_patch_get_if_addrs6_3.patch (4.2 KB) - added by neel 3 years ago.
Cleaned up patch with all four error messages and (hopefully) no typos.

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by neel

Patch to add error for "Address that we determined via UDP socket magic is unsuitable for public comms."

Changed 3 years ago by neel

Attachment: tor_test_output.log added

Log file for my patch for "Address that we determined via UDP socket magic is unsuitable for public comms."

comment:1 Changed 3 years ago by neel

I have created a patch for this. Tell me what you think about this.

Thanks,
Neel Chauhan

comment:2 in reply to:  1 Changed 3 years ago by teor

Status: newneeds_revision

Replying to neel:

I have created a patch for this. Tell me what you think about this.
...

Thanks for this patch, neel!

Here's how I would make the patch better:

  • I would keep expect_log_msg_containing_either as a 2-argument version, and add another macro expect_log_msg_containing_either3 for the 3-argument version. That way, we can use whichever one we need in future tests.
  • I just read get_interface_address6_via_udp_socket_hack, and there are actually 4 things that can go wrong. So let's cover them all - the extra one is:
    • "getsockname() to determine interface failed"

(I guess that means we need a 4-argument macro expect_log_msg_containing_either4 as well.)

Can you make these changes, and submit another patch?

Changed 3 years ago by neel

Updated patch

comment:3 Changed 3 years ago by neel

I uploaded an updated patch as tor_patch_get_if_addrs6.patch​.

-Neel

comment:4 Changed 3 years ago by teor

Hi Neel, thanks, almost there!

Both tests can fail for all 4 reasons.
Can you put all 4 reasons in both tests?

Thanks!

Changed 3 years ago by neel

Patch with getsockname() error reason to test_address_get_if_addrs6_list_internal()

comment:5 Changed 3 years ago by neel

I added an updated patch (again).

-Neel

comment:6 Changed 3 years ago by teor

Ok, two typos, and then I think we're good to go:

  • expect_log_msg_containing_either4 checks str3 twice, and doesn't check str4.
  • test_address_get_if_addrs6_list_internal uses expect_log_msg_containing_either3 when it should use expect_log_msg_containing_either4

Changed 3 years ago by neel

Cleaned up patch with all four error messages and (hopefully) no typos.

comment:7 Changed 3 years ago by teor

Status: needs_revisionneeds_review

Ok, I added a fixup commit for typos, whitespace, and line length; and a changes file, then I put it on a branch on github:
https://github.com/teor2345/tor/commits/bug20634_029

Our coding standards about whitespace, line length, and changes files are in:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md

For whitespace and line length, I usually just run our automated check:
make check-spaces

If you're happy with the fixup and the changes file, let me know, and I will put the branch on the merge queue.

Thanks for helping out!

comment:8 Changed 3 years ago by teor

Keywords: CoreTorTeam201611 added

comment:9 Changed 3 years ago by neel

I'm fine with it.

-Neel

comment:10 Changed 3 years ago by teor

Status: needs_reviewmerge_ready

Please merge bug20634_029 in my github repository to 0.2.9, if possible.

It's a unit test fix, so I think it can go in after the feature freeze.
(The bug has been around since 0.2.8.1-alpha, so it's not a regression.)

comment:11 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merging to 0.2.9 and forward. Thanks, Neel! Thanks Teor!

Note: See TracTickets for help on using tickets.