Opened 7 years ago

Closed 7 years ago

#2587 closed defect (fixed)

investigate and fix AuthSrvTest.DDNSForward regression on NetBSD

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20130122
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0.5 Internal?: no

Description

This keeps happening:
http://git.bind10.isc.org/~tester/builder//BIND10/20121227230307-NetBSD4-i386-GCC/logs/unittests.out

We should look into it, figure out the cause, and fix it.

My current impression is that we only need some minor tweak in the
test code (i.e., it's not a bug of the main code) but I'm not sure
right now.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130122

comment:2 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:3 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Found it, this NetBSD is a tad stricter on the contents of the sockaddr struct when calling getnameinfo().

ASIO does not set the sa_len field, and most BSD's don't care (linux doesn't even have it). But getnameinfo() on this system will fail if it isn't set.

Proposed fix makes a non-const copy of the sockaddr struct, and if sa_len is available, it is set.

I also piggybacked a small unit test memory leak in srv_test.cc

comment:4 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

need to check something, taking back ticket for a bit

comment:5 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned

comment:6 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 Changed 7 years ago by jinmei

It basically looks okay. (I've not checked it on the netbsd box,
but the explanation makes sense, and I'm assuming you confirmed it on
it.)

A couple of minor points:

  • in auth_srv_unittest.cc, you can use convertSockAddr to convert sockaddr_storage* to sockaddr*:
        struct sockaddr* sa = convertSockAddr(&ss);
    
    (it does the same thing as the original code; it's just convenient shortcut).
  • the fix to memory leak looks correct, but I wonder we might rather switch to scoped_ptr at this opportunity.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Both good points, done.

comment:10 Changed 7 years ago by jinmei

I made one minor editorial change.

It can be merged, with a couple of minor additional points:

  • I guess we can even remove the explicit definition of ~SrvTestBase. ::testing::Test should have a virtual destructor, and that should be enough.
  • I'd explicitly include scoped_ptr.hpp in srv_test.cc, too, instead of relying on that it's included in srv_test.h

But I'd leave these points to you, and either way I don't think I need
to re-review it. So, please merge with or without (any of) these.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte
  • Total Hours changed from 0 to 0.5

comment:12 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks, addressed those comments as well, and merged. Closing ticket.

Note: See TracTickets for help on using tickets.