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          |  stephen
            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 marcin):

 * owner:  marcin => stephen


Comment:

 Replying to [comment:4 stephen]:
 > 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.

 This is nice idea but my concern is that the complete option definition
 consists of the parameters passed to the !OptionDefinition's constructor
 and the definition of record fields (obviously, for options containing
 record of fields). In such case you would have to look into the header
 file to get the ''general'' definition and then to initStdDefs6 to find
 out the record fields types. That's confusing.

 >
 > 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.)

 I unrrolled the code for D6O_IA_NA and put the case entries in the
 alphabetical order.

 >
 > '''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.

 I have been thinking about it for a while and in fact I couldn't find any
 name that would give me 100% satisfaction. The !OptionRecord may be
 misleading because you may think that this class supports only those
 options that are of the OPT_RECORD_TYPE type while this class supports the
 whole variety of option types. I disagree with !OptionRecord. I tend to
 think about renaming it to OptionGeneric or OptionIndexed.

 >
 > 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()).

 I would keep this change for the merge time as this is trivial.

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

 Comments added.

 >
 > '''src/lib/dhcp/option_custom.h'''[[BR]]
 > readInteger(): typo in comment - "thet tha index".
 >
 Fixed.
 >
 > '''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?

 You're right. I removed the extra argument.

 >
 > '''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.

 Agreed. I rmoved the part of the comment about efficiency.

 >
 > 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.

 Fixed.

 >
 > '''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")

 I added some comments as requested. Also I pulled both cases to a separate
 "if".

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

 Fixed.

 >
 >
 > '''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)"?

 No it does not compile. If we wanted to enable implicit conversions
 between char* and iterators we would probably have to make the constructor
 a template (like it is done for std classes):
 {{{
     template<typename InputIterator>
     OptionCustom(const OptionDefinition& def, Universe u,
                  InputIterator first, InputIterator last)
         : Option(u, def.getCode(), first, last),
           definition_(def) {
         createBuffers(data_);
     }
 }}}
 However this will not compile either because !Option ctor would not accept
 those arguments. Eventually, all other constructors would have to be
 changed either. If you think that it is important, maybe we could create a
 ticket for this?

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

 Added.

 >
 > 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.

 The try...catch had been used for debugging purposes. Now I removed it.

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

 Ok. I corrected it.

 >
 > setIPv4AddressData test: header comment typo - "opton".

 Fixed.

 >
 > '''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.

 I think it is worth to do this little change in a merge time. As long as
 the review is in progress let's avoid merging master to trac2491 branch so
 as the list of commits on the branch is not polluted by changes on master.

 >
 > 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.

 Good catch! I added the check.

 >
 > 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".

 Removed.

 ----

 I haven't implemented all option definitions so I am wondering if there is
 any sense to add !ChangeLog at this time. Maybe we could defer this until
 all option definitions are implemented? Comments?

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