BIND 10 #167: review: configure addresses and ports to listen on for DNS servers

[BIND 10 Development]

#167: review: configure addresses and ports to listen on for DNS servers
----------------------+-----------------------------------------------------
 Reporter:  larissas  |        Owner:  UnAssigned                                    
     Type:  task      |       Status:  reviewing                                     
 Priority:  major     |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  b10-auth  |   Resolution:                                                
 Keywords:            |    Sensitive:  0                                             
----------------------+-----------------------------------------------------
Changes (by stephen):

  * owner:  stephen => UnAssigned


Comment:

 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.

 ''run_server()''
 There is no check that the "port" argument is valid.  This function
 converts the passed string to "short" via a call to "atoi()", but atoi()
 makes no check as to the validity of the argument.  Suggest that this
 could be checked via a call to boost::lexical_cast.

 Remark: If the address argument does not match the flag -4/-6, an error
 message is output and the program terminated via a call to exit().
 However, main() explicitly deletes the auth_server object before
 terminating.  If this latter behaviour is desired in all cases, run_server
 should return on error instead of exiting.

 Slight inconsistency: the first "if" checks "if (address != NULL)", the
 second "if" checks "if (address)".  (In fact, the first clause of the
 second "if" could be combined into the first "if" construct.)

 It appears that if "address" is given, the servers.xxx4_server variables
 are set regardless of whether the address is an IPV4 or IPV6 address.

 ''main()''
 The "case" branch parsing of "-6" results includes what looks like
 temporary code 'cout << "here" << endl'.

 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.

-- 
Ticket URL: <http://bind10.isc.org/ticket/167#comment:18>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development