BIND 10 #1238: IfaceMgr support for IPv4 (incl. Socket binding for v4)

[BIND 10 Development]

#1238: IfaceMgr support for IPv4 (incl. Socket binding for v4)
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111207
                  Component:  dhcp4  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed in two stages so as to avoid the files added/modified in the
 merge of 1228 into 1238.  I've removed from the first review comments that
 have been addressed by the changes made in the second set of commits (but
 some may have been missed).

 Changes between commits 9697c6b3cc3e49d96efc6777c1dba5ecb00eb785 and
 d5e189cf1573446503a4fafa3e909db60eb04623

 '''src/bin/dhcp/.gitignore'''[[BR]]
 Git ignore files are not usually put in the repository.

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 Dhcp6Srv constructor: Unless there is some reason to dismiss the
 exception, why not let it propagate?

 '''src/bin/dhcp6/iface_mgr.h'''[[BR]]
 The methods in struct Iface should be documented using Doxygen tags.

 Semantic point - although "struct" and "class" are interchangeable,
 "struct" tends to be used for collections of data (where the data members
 are public) and class where the methods are more important.  In this case,
 the data members are hidden, so "class" would probably be more
 appropriate.

 openSocket(): the return type is uint16_t, so it can never return -1 to
 indicate an error.

 '''src/bin/dhcp6/iface_mgr.cc'''[[BR]]
 General - at some time we might want to see if the boost::asio library (or
 classes in the BIND 10 asiolink library) offers a better way to interface
 to sockets. (But not now, let's continue with what we have.)

 General: some comments use "!//" and some are "/* ... */".  We should be
 consistent within a file.

 openSocket4/6(): This is declared to return uint16_t, but it returns the
 "sock" variable which is (correctly) declared "int" (the return type of
 socket()).

 openSockets(): should be spaces around binary operators such as "=", "<"
 and "!=".

 openSockets(): should it really throw an "Unexpected" exception if the
 socket open fails - it could fail for all sorts of (expected) reasons.
 And if it does, will this exception be caught? (The same goes for other
 exceptions.) Alternatively, if the reason is because the code is executed
 at startup and any failure indicates something wrong with the
 configuration, that should be mentioned in the class header.

 getIface() (both versions): even if an "if" body is a single line, it
 should have braces round it.

 '''src/bin/dhcp6/tests/iface_mgr_unittest.cc'''[[BR]]
 The packet writer is probably something that may be usefully put in the
 tools directory. (But that is a long-term goal - no need to do anything
 now.)

 Changes between 7d2f07481169780071bf564223a20a219b550385 and
 a26b979adb54baabdf939ed1a7852b2ee9b8b93c

 '''src/bin/dhcp6/iface_mgr.h'''[[BR]]
 At some time in the future we can probably replace !SocketInfo by a
 boost::ip::udp::socket (or an asiolink/udp_socket) object. (But not now,
 although please create a ticket to review this suggestion.)

 Iface::delAddress: This removes the first element in the list that has the
 specified value. If the intention is to remove all elements in the vector
 with that value, an alternative is:
 {{{
 addrs_.erase(remove(addrs_.begin(), addrs_.end(), *a), addrs_.end());
 }}}
 (See Scott Meyers's "Effective STL", Item 9.)  (delSocket probably can't
 be optimised the same way owing to the need to close the socket only if
 the socket is found and the need to create a comparison function for two
 !SocketInfo structures.)

 openSocket6(): at the statement before the return, could get rid of the
 temporary variable "info" with:
 {{{
 iface.addSocket(SocketInfo(sock, addr, port));
 }}}

 send() and receive4(): the correct "not implemented" exception is
 !NotImplemented.

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 Failes to compile due to error at line 124 (one or both of the arguments
 to the EXPECT_EQ don't have operator<<() defined.)

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