BIND 10 #1226: V4 packet library - packet parsing

[BIND 10 Development]

#1226: V4 packet library - packet parsing
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20111109
                  Component:  dhcp   |            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:4 stephen]:
 > Reviewed commit 0fed56c3692e358184958cc1263cff67db0f62cb
 >
 > '''src/lib/asiolink/io_address.cc'''[[BR]]
 > Any reason why a static IOAddress method is used to create an IOAddress
 rather than have a constructor take a uint32_t? (I see that from_bytes()
 also does that and I'm puzzled by that as well.)
 Fixed.

 > In the Jabber room although I suggested the uint32_t conversion
 operator, I then had second thoughts: if the IOAddress were only holding a
 V4 address, a conversion operator would be fine.  But because it can hold
 a V6 address and because conversions are implicit it would be possible to
 have an apparently correct statement generate an exception for no
 discernible reason.  For this reason, perhaps a toUint32() method to
 return the address as a uint32_t would be better?
 Does it matter if the exception is thrown during conversion attempt or
 during function call? In both cases exception clearly states what went
 wrong.

 There is also warning in the text of the operator that it will throw
 BadValue exception during attempt of IPv6 conversion.

 > '''src/lib/asiolink/pkt4.h'''[[BR]]
 > Header of getBuffer() needs to make it clear that the returned output
 buffer is valid only while the Pkt4 object is valid.
 Done.

 > The buffer returned by getBuffer() should be "const" (else it is
 possible to modify the internals of the Pkt4 object without going through
 one of the defined interfaces.  Similarly, the method getBuffer() should
 also be const.
 Returned type is now const. getBuffer() method is now consts. All other
 getters are const, too.

 > Not related to this change, but the various Pkt4::getXxxx() methods
 should be "const" as they don't change the contents of the Pkt4 object.
 Ahh. I came to the same conclusion, before reading this. Done already. :)

 With added consts, some of the lines are over 80 characters, but I believe
 that there was agreement on Jabber that we set the limit to 100.

 > '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 > Declaration of "const static" variables: the "static" is not needed as
 by default, "const"s (and "typedef"s) have internal linkage.
 Ok. Fixed.

 >
 > fixedFieldsPack test: Still declaring variables as "type * name" instead
 of "type* name" :-)
 And there were no spaces before and after +, / and = signs. Fixed all of
 them. Number of this kind of changes decreases, which is a good sign.

 Fixed code ready for review.

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