BIND 10 #3184: Kea4: does not echo back relay agent info option

[BIND 10 Development]

#3184: Kea4: does not echo back relay agent info option
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tmark
                Type:  defect        |                       Status:
            Priority:  very high     |  reviewing
           Component:  dhcp4         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  Low
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:5 tmark]:
 > dhcp4_test_utils.h -
 >
 > Closing endif is tagged incorrectly.
 > {{{
 >  // DHCP6_TEST_UTILS_H
 > }}}
 Fixed.

 > dhcp4_test_utils.cc -
 >
 > You left all of the method briefs in the .cc file.  Convention is that
 they are
 > in the .h file, not both.
 I had troubles addressing this one, because there was no such file in my
 repo. But I thought it would be rude to throw such a fine comment, so I
 added dhcp4_test_utils.cc.

 > wireshark.cc -
 >
 > Extraneous spaces at the end of lines. HA! I finally caught someone with
 this!  Marcin and Stephen always manage to get me on this one.
 Ha! I forgot that one time to call whitespace-cleanup and you caught me. I
 should be more sneaky next time.

 > wireshark.cc -
 >
 > You might consider a bit of restructure here.  There is an underlying
 mechanism for building packets from captured packet hex strings that
 should be extracted from the captureRelayedDiscover() into a generic
 method, something like:
 >
 > {{{
 >     Pkt4Ptr Dhcpv4SrvTest::packetFromCapture(const std::string&
 hex_string) {
 >         std::vector<uint8_t> bin;
 >
 >         // Decode the hex string and store it in bin (which happens
 >         // to be OptionBuffer format)
 >         isc::util::encode::decodeHex(hex_string, bin);
 >
 >         Pkt4Ptr pkt(new Pkt4(&bin[0], bin.size()));
 >         captureSetDefaultFields(pkt);
 >
 >         return (pkt);
 >     }
 > }}}
 >
 > You could then start a static array of captured hex strings with numeric
 const indexes to look them up, or maybe a map of captures with string
 names for keys, or number of other things; rather than a new method for
 each new captured packet.  Down the road you could load packet inventory
 from file etc.
 >
 > For now, simply the basic method as above and alter captureRelayDiscover
 to delcare the hex_string and just do this:
 >
 > {{{
 >     return (packetFromCapture(hex_string));
 > }}}
 >
 > would suffice.
 Done as suggested.

 > dhcp4_srv_unittest.cc -
 >
 > In TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho), you should probably put an
 > ASSERT_NO_THROW around the call to captureRelayedDiscover method.  That
 method uses a Pkt4 constructor that can throw.
 Done.

 >  cppcheck, clang build, and valgrind unit tests all pass
 Excellent.

 I think I addressed all issues. There's new file dhcp4_test_utils.cc that
 contains code moved from dhcp4_srv_unittest.cc. Several classes were moved
 to dhcp4_test_utils.h as well.

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