BIND 10 #2786: DHCPv4: domain-name option must be encoded as text and NOT as FQDN

[BIND 10 Development]

#2786: DHCPv4: domain-name option must be encoded as text and NOT as FQDN
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  defect        |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20130523
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit commit b3c562f2bd85fa9f825d59f8601ba99e04e661f3

 '''src/lib/dhcp/option_string.{cc,h}'''[[BR]]
 General: the implementation stores the string in a class member variable,
 which duplicates the storage of the base class.  As the "string" nature of
 the class only appears in the {get,set}Value() methods and the string
 constructor, this seems wasteful.  Can't the data be stored in the
 underlying class and converted to/from a string as needed? (As an aside,
 it also means that most process can be done by the parent class methods.)
 Note that this is a suggestion: the amount of code saved is minimal.

 setValue/constructor taking a string argument: should check for an empty
 string and throw an exception if this is the case. (This puts the
 exception closer to the point of the error.)

 Exceptions: suggest that the exception message includes the type code of
 the option - it will make diagnosis of any problem easier.


 '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 unpackOptions4 test: trivial point: "v4Opts+n" should have spaces around
 the "+". (This appears in several places in this test)

 stdOptionDefs4 test: interesting correction.  I note that before the
 change, "end" was equal to "begin", yet apparently the test passed.  This
 suggests that testStdOptionDefs4() will always register a "pass"
 regardless of the value of the "end" argument, so either:
 * "end" is not really needed as an argument.
 * testStdOptionDefs4() registers a "pass" regardless of its arguments.
 It's worth investigating this.


 '''src/lib/dhcp/tests/option_string_unittest.cc'''[[BR]]
 Should include a test for setValue() with an empty string.

 Should include a test for trying to construct a string option using a
 zero-length string.


 '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 unpackOptions test: comment under the "x = pkt->getOption(14)" statement
 is incorrect - it is option 14 that should exist, not option 13. (A
 reference to option 13 also occurs in another comment later in that block
 of code).

 '''!ChangeLog entry'''[[BR]]
 This is OK.

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