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