BIND 10 #2491: Option Definitions: Implement option definitions for the rest of DHCPv6 standard options.

[BIND 10 Development]

#2491: Option Definitions: Implement option definitions for the rest of DHCPv6
standard options.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20121213
         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 stephen):

 * owner:  stephen => marcin


Comment:

 Reviewed commit b1d0e42774ef51b404be44ee53cb930a630a63bb

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 createStatusCode(): do you need to explicitly indicate the template type
 in the call to writeInteger?  Won't the correct type be chosen by the fact
 that "code" is passed to it?

 '''src/lib/dhcp/libdhcp++.cc'''[[BR]]
 initStdOptionDefs():  I would be tempted to put the !OptionParams
 definition into a header file and and definition of params[] into a
 separate file so as make them easier to locate and update.  A function
 could return a "const !OptionDefinition*" pointing to the array.  A null
 entry in "name" could mark the end of the array.

 initStdOptionDefs(): Suggest putting the entries in the "case" statement
 in alphabetical order.  Also, the first two entries (D60_IA_NA and
 D60_ID_PD) are the only two that share common code, although there are
 several other entries (D60_VENDOR_CLAS, D60_VENDOR_OPTS, D60_REMOTE_ID)
 that have the same code. Either entries should share code, or each entry
 should have separate code.  (Also, given that some entries have up to
 three "addRecordField" lines, unrolling the loop for D60_IA_NA and
 D60_IA_PD would make the code for these cases look consistent with the
 rest of the "switch" statement.)

 '''src/lib/dhcp/option_custom.cc'''[[BR]]
 Just a thought: is !OptionCustom the right name for this class?  As it
 represents an option with a defined structure, something like
 !OptionRecord might be better.  I would tend to think that "custom
 options" are ones that a customer has created for a specific purpose.

 addArrayDataField(IOAddress) and writeAddress(): if #2396 is reviewed and
 completed before finishing this ticket, the "address.getFamily() ==
 AF_INET[6]" comparisons can be replaced with a call to address.isV4() (or
 isV6()).

 createBuffers(): I would like some comments as to what is going on here.

 '''src/lib/dhcp/option_custom.h'''[[BR]]
 readInteger(): typo in comment - "thet tha index".


 '''src/lib/dhcp/option_data_types.h'''[[BR]]
 readFQDN(): why is the length passed back as an argument?  Isn't it
 possible to retrieve the number of bytes read from the buffer from the
 size of the returned string?

 '''src/lib/dhcp/option_data_types.cc'''[[BR]]
 readFqdn(): The comment in the method states that the data is copied to an
 input buffer and is thus not efficient.  This is not the case: an
 !InputBuffer object just maintains a pointer to the external data, which
 has to remain valid while !InputBuffer is in use.  So although
 constructing a name from a "const vector" would be better, the addition of
 an !InputBuffer doesn't add much overhead.

 writeFQDN(): could be optimised slightly: within the "if block", if "buf"
 is resized after labels.getData() is called, the resize operation could
 use the data size returned in the getData() call instead of needing to
 call getDataLength() explicitly.

 '''src/lib/dhcp/option_definition.cc'''[[BR]]
 optionFactory(): would appreciate some comments here.  In particular:
 * why do the address types only return a specific value for array types
 but drop through to return a custom type if they are not arrays?
 * in the default option, what cases can cause dropping through the if and
 else clause? (Incidentally, both the "if" and the "else if" statements in
 this part of the code check that it is a V6 universe. That could be pulled
 out into a separate "if")

 '''src/lib/dhcp/tests/libdhcp++_unittest.cc'''[[BR]]
 testStdOptions(): typo "deinitions".  Also "class'es" should be "class's"


 '''src/lib/dhcp/tests/option_custom_unittest.cc'''[[BR]]
 fqdnData test: there should be no need for the intermediate "buf" vector.
 Instead of "buf.begin()" and "buf.end()", can't you use "data" and "data +
 sizeof(data)"?

 recordData test: comments "Initialize field 0/2" should say what they are
 initializing (as do the other comments).

 recordData test: when creating the new !OptionCustom object, do we expect
 the exception to be triggered or not?  There appears to be two identical
 initializations, one in a try...catch, the other in an ASSERT_NO_THROW.

 recordData test: Comment says we should have 5 data fields but the check
 is for the value 6.

 setIPv4AddressData test: header comment typo - "opton".

 '''src/lib/dhcp/tests/option_data_types_unittest.cc'''[[BR]]
 writeAddress(): if #2396 is reviewed and completed before finishing this
 ticket, it provides "IOAddress::toBytes()" that will eliminate the need
 for this method.

 writeBool test: after writing a second value, you may want to check that
 the first hasn't changed. A failure whereby the write operation filled the
 entire buffer with the value being written would cause this test to pass.

 writeInt test: you could remove the comments before each call to
 writeInt() (each really just mirrors the following operation) and in the
 preamble to this part of the test say that "Each write operation appends
 an integer value of a different type".

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