Opened 9 years ago

Closed 9 years ago

#259 closed defect (wontfix)

b10-auth and bind10 cleanup comments

Reported by: each Owned by: shane
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket:
Total Hours: Internal?: no

Description

While reviewing trac #167, Stephen made some comments about src/bin/auth/main.cc and src/bin/bind10 that were unrelated to the ticket at hand; I am moving those comments into this ticket to be addressed later.

Review of branches/trac167 revision 2268

Files modified/added since the branch was created at revision 1879
(output of svn diff -r 1879:HEAD --summarize).

M src/bin/auth/main.cc
M src/bin/bind10/tests/bind10_test.py
M src/bin/bind10/bind10.py.in

src/bin/auth/main.cc
General
The documentation in this module is sparse.

The opening brace of some functions is not on the end of the signature when the signature is on one line (BIND-10 C++ coding standards).

check_axfr_query
A C-style cast is used - the C++ style "xxx_cast<>()" is preferred.

A "magic" number (0xFC80) is hard-coded in the function.

The single-line "if" blocks should be surrounded by braces (BIND-10 C++ coding standards).

dispatch_axfr_query
A C-style cast is used - the C++ style "xxx_cast<>()" is preferred

class TCPClient
No indication given should an error occur; instead the object silently disappears.

class UDPServer
Does not appear to be any logging should an error occur in the receive function, nor if a zero-length message is received.

If B10_FROM_SOURCE is defined, the path "/src/bin/auth/auth.spec" is appended to it to file the specification file. Just a bit unhappy that this path is buried deep in the code - it would be better either declared as a constant at the head of the file or placed into a separate header file.

src/bin/bind10/tests/bind10_test.py
TestProcessInfo
test_init
Should check that pi.dev_null_stderr is also set to False on default construction.

"env" is an argument to the ProcessInfo constructor. Why have the tests for the initialization of this member been commented out?

TestBob
Only checks that the arguments have been stored correctly. What tests are done that the other methods work?

src/bin/bind10/bind10.py.in
(Just a check of the changes since revision 1879)

Class !ProcessInfo
Remark: As this class creates a process, to my untutored eyes ProcessInfo seems a misleading name.

Subtickets

Change History (7)

comment:1 Changed 9 years ago by shane

Some suggested additional tests, based on the last review of #167:

  • The run_server() function should test to insure that an IPv6 address works
  • We should add a test to check what is thrown if we pass an invalid port number

comment:2 Changed 9 years ago by each

  • Owner set to each
  • Status changed from new to accepted

comment:3 in reply to: ↑ description Changed 9 years ago by each

check_axfr_query
dispatch_axfr_query

This is all being rewritten in trac #221, so I'll leave it as it is for now.

class TCPClient
No indication given should an error occur; instead the object silently disappears.

class UDPServer
Does not appear to be any logging should an error occur in the receive function, nor if a zero-length message is received.

"env" is an argument to the ProcessInfo constructor. Why have the tests for the initialization of this member been commented out?

The top of the file says:
# XXX: environment tests are currently disabled, due to the preprocessor
# setup that we have now complicating the environment

TestBob
Only checks that the arguments have been stored correctly. What tests are done that the other methods work?

The other methods all relate to other modules -- config handler and command handler interact with msgq; startup and shutdown with everything. I'm not sure how we can unit test these things. However, I'll request that Shane give it some consideration.

Remark: As this class creates a process, to my untutored eyes ProcessInfo seems a misleading name.

I agree, and changed it to Process in the trac259 branch. Checkpointing, and then I'll handle Shane's remarks.

comment:4 Changed 9 years ago by each

A thing I noticed while working in asio_link.cc: In IOServiceImpl, the pointers to tcp4_server_, udp4_server_, tcp6_server and udp6_server_ are never set to anything but NULL. I assume they were supposed to be retained so they could be deleted when the IOService object was, and have fixed this.

Regarding Shane's comments:
I have added asio_link unit tests to check on ports and addresses. They're not really complete yet, but it's a start.

The run_server() function should test to insure that an IPv6 address works

I'm not quite sure what this means. It currently tests to ensure that any address (v4 or v6) parses correctly, and ASIO will report an error if it can't bind to an address; is there a specific failure condition you want to check for that I've overlooked?

comment:5 Changed 9 years ago by each

  • Owner changed from each to shane
  • Status changed from accepted to assigned

Handing this to Shane now to clarify his comment, think about additional BoB testing, and maybe review.

comment:6 Changed 9 years ago by jinmei

Replying to each:

A thing I noticed while working in asio_link.cc: In IOServiceImpl, the pointers to tcp4_server_, udp4_server_, tcp6_server and udp6_server_ are never set to anything but NULL. I assume they were supposed to be retained so they could be deleted when the IOService object was, and have fixed this.

Right, and it's been fixed in Trac #221.

comment:7 Changed 9 years ago by shane

  • billable set to 0
  • Internal? unset
  • Resolution set to wontfix
  • Status changed from assigned to closed

I'm going to close this ticket, since it is basically asking for more testing of the boss process. As changes are made to that process tests are added. We don't really have a plan to go back and add unit tests to all places where tests are missing.

Note: See TracTickets for help on using tickets.