BIND 10 #3082: Implement DHCPv4 Client FQDN option (81)

[BIND 10 Development]

#3082: Implement DHCPv4 Client FQDN option (81)
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130821
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 option4_client_fqdn.h -

 1. Check line length option4_client_fqdn.h - several lines in the class
 brief are longer than 80 and wrap.

 2. I think it would help if the comment clarified this is size on bytes,
 ..."The size(bytes)"  or "The size in bytes"
 {{{
  /// @brief The size of the fixed fields within DHCPv4 Client Fqdn
 %Option.
 }}}

 3.  Insert "an" between "create" and "instance" in the following line:

 {{{
 /// This constructor is used to create instance of the option which will
 be
 }}}

 4.  Shouldn't your contructor briefs include an @throw?  You do mention,
 "Otherwise, the constructuro wil throw an exception", but I was under the
 impression that if a method throws it needed an @throw and it should list
 the exception Class(es) possible.

 5. setDomainName method can throw, and there's no metion of that in its
 brief.

 6. Why do we need a class for Rcode?



 option4_client_fqdn.cc -

 1. Check line length here too.

 2. Even though Option4ClientFqdnImpl is defined in the .cc file and not
 the .h,
  and its the imple part of PIMPL, the class defintion has no commentary at
 all.  It defines 4 methods that Option4ClientFqdnImpl does not, some of
 which
 throw but you can't tell that unless you look at the code for each.  I
 would
 think at least the methods unique to the Impl could use a header.

 3. Option4ClientFqdn::getFlag()

 You are testing that the parameter flag only be valid enum value.  It
 can't be
 anything but a valid value since the parameter type is Flag.  It shouldn't
 be
 possible to pass it a value of 0x03.  Isn't that the whole point of making
 them
 an enum set to begin with?


 option4_client_fqdn_unittest.cc -

 1.Rather that use the exact same names as the enums for the flags, maybe
 you
 should alter them a bit, like UBYTE_FLAG_S, or FLAG_S_BTYE? or BFLAG_S..

 Maybe you should simple delcare them as uint8_t consts to begin with as
 your
 main line code doesn't really make much use of them being enums.

 2. constructFromWire()

 You can initialize the array with chars if you want, so instead of this:

         6, 109, 121, 104, 111, 115, 116,     // myhost.
         7, 101, 120, 97, 109, 112, 108, 101, // example.
         3, 99, 111, 109, 0                   // com.

 you have this:

         6, 'm', 'y', 'h', 'o', 's', 't',      // myhost
         7, 'e', 'x', 'a', 'm', 'p', 'l', 'e', // example.
         3, 'c', 'o', 'm', 0                   // com.

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