BIND 10 #2000: update libdns++ OPT RDATA so it can handle EDNS options

BIND 10 Development do-not-reply at isc.org
Wed Feb 19 09:49:30 UTC 2014


#2000: update libdns++ OPT RDATA so it can handle EDNS options
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  enhancement   |  stephen
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  bind10-1.2-release-freeze
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:  NSID
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => stephen


Comment:

 Hi Stephen

 Replying to [comment:6 stephen]:
 > '''src/lib/dns/rdata/generic/opt_41.h'''
 > '''src/lib/dns/rdata/generic/opt_41.cc'''
 > The methods (and attributes) of both OPT and the internal PseudoRR
 > class need documentation.

 This has been added. For `OPT`, it was added only for the new methods.

 > The storage of the pseudo-RRs as a vector of shared pointers to
 > PseudoRR objects seems a bit complicated, since only the getPseudoRRs
 > () method accesses them.

 The pseudo-RRs are actually stored as a `boost::shared_ptr` to a
 `std::vector` of `PseudoRR` objects.

 > Why not store the PseudoRRs as a vector of bytes?  This would would
 > simplify the construction (which only needs to check that the lengths
 > in the input buffer indicate that the buffer could represent a set of
 > PseudoRRs), the toWire() methods (which just write out the buffer as a
 > set of bytes) and append() method (which just appends the code, length
 > and data to the current buffer). Only the getPseudoRRs() method need
 > be altered to split the buffer into a vector of PseudoRR objects, the
 > code of which is already in the constructor.  (Of course, this
 > suggestion depends on the anticipated usage - if getPseudoRRs() is
 > called more frequently than anything else, the current approach would
 > reduce overhead.)

 The `boost::shared_ptr` is in use because `Rdata` objects are
 copy-constructed in our code and doing a full copy of the options will
 waste time and space.

 The `PseudoRR` class makes storage of the options more straightforward
 to understand. There is no significant overhead due to wiredata storage
 (as the wiredata has to be copied by the `Rdata` implementation anyway).
 Some overhead is in `PseudoRR` object creation. In both cases, we have
 to parse through the various options to validate the passed wire data
 during construction anyway. As you say, storing the option data raw and
 parsing every time during `getPseudoRRs()` would not be optimal. If we
 need to add API later to delete or rewrite options (such as modify an
 incoming OPT RR and send it out), the current implementation would be
 more suitable.

 When making this API I wondered how it would be used by the caller for
 various use-cases of options. After trying different types of storage
 (including raw pointers instead of `shared_ptr`), the current approach
 was picked so that it would first address the copy-construction issue,
 and also be simple to understand and change if API changes are required.

 > '''src/lib/dns/tests/rdata_opt_unittest.cc'''
 > In the test data (as indicated in rdata_opt_wiredata), I would suggest
 > making the numeric values of the option code and the length different.
 > If there is some error in the source code that gets the option's code
 > and length confused, it won't show up in a test using that data.

 The option code has been changed.

 > '''src/lib/dns/tests/testdata/rdata_opt_fromWire{1,2,3,4}'''
 > Please start comments with a capital letter.

 Done.

 > '''src/lib/dns/tests/testdata/rdata_opt_fromWire{3,4}'''
 > s/psuedo/pseudo/

 Fixed.

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


More information about the bind10-tickets mailing list