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      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:6 stephen]:
 > '''src/bin/dhcp/.gitignore'''[[BR]]
 > Git ignore files are not usually put in the repository.
 Sent out a mail about .gitignore to ML. Will act depending on the reply.

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

 >
 > '''src/bin/dhcp6/iface_mgr.h'''[[BR]]
 > The methods in struct Iface should be documented using Doxygen tags.
 All methods and fields in Iface class are (and were) documented. I noted
 that IfaceMgr::send(Pkt4) and several others in IfaceMgr was not
 documented, so I added doxygen comments.

 >
 > 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.
 Ok. Iface is now a class.

 > openSocket(): the return type is uint16_t, so it can never return -1 to
 indicate an error.
 Fixed comment. Added a description that this method will throw exception
 if somewthing goes wrong. (the same holds true for openSocket4 and
 openSocket6).


 > '''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.)
 I'll be happy to do that once boost::asio library starts supporting all
 the weird things we are doing with sockets, like binding them to specific
 interface, joining multicast groups etc.

 > 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()).
 It was pointed out in #1282 that uint16_t should be used. Signed int is
 used to indicate errors, but we handle this with exceptions. My
 understanding is that the we can use uint16_t or uint32_t here. Jelte
 pointed out that uint16_t should be used, so I'm doing that.

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

 >
 > 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.
 Currently defined exceptions are: OutOfRange, InvalidParameter, BadValue,
 Unexpected and NotImplemented. Other exceptions are clearly not
 appropriate here, so I chose Unexpected. We could define new exception
 type if you think it is appropriate. Added comment about exceptions.

 >
 > getIface() (both versions): even if an "if" body is a single line, it
 should have braces round it.
 Done.
 >
 > '''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.)
 Ok. I suppose that for the foreseeable future, I'll be the sole user of
 this code. Turning this into usable tool will require some effort, so this
 won't happen anytime soon.

 Second part of the review will follow.

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