Opened 7 years ago

Closed 6 years ago

#2000 closed enhancement (fixed)

update libdns++ OPT RDATA so it can handle EDNS options

Reported by: jinmei Owned by: muks
Priority: medium Milestone: bind10-1.2-release-freeze
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: NSID
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is necessary for NSID support.

This task itself is independent from NSID (in the OPT RDATA class
we'll handle the options as opaque data).

We'll need to

  • decide internal class representation for option data
  • update all methods except the "from text" constructor

See also BIND 9's implementation at bind9/lib/dns/rdata/generic/opt_41.c

Subtickets

Change History (11)

comment:1 Changed 7 years ago by jinmei

  • Component changed from Unclassified to libdns++
  • Owner set to UnAssigned

comment:2 Changed 6 years ago by muks

  • Owner changed from UnAssigned to muks
  • Status changed from new to assigned

comment:3 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Up for review.

comment:4 Changed 6 years ago by muks

ChangeLog entry:

XYZ.    [func]          muks
        Update the OPT RR type (RFC 6891) to actually support options (pseudo RRs).
        (Trac #2000, git ...)

comment:5 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:6 follow-up: Changed 6 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit 916893ac37eeeb169c90c7cf374630a7cfba127b

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.

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

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.

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

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

Other
Suggested ChangeLog entry is OK

comment:7 in reply to: ↑ 6 Changed 6 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

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

comment:8 Changed 6 years ago by stephen

  • Owner changed from stephen to muks

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

Ack. Should really be looking at the code while I type the comments and not rely on memory.

that it would first address the copy-construction issue, and also be simple to understand and change if API changes are required.

Fair point, although as the number of options is generally small, I doubt that the copy construction overhead would be high. However, what is there is a valid solution to the problem, and there is no need to change it.

All changes OK, please merge.

comment:9 Changed 6 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Please can you review commit c1c4ea8d1b7629dac379f192d30cc79535214236 on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

comment:10 Changed 6 years ago by stephen

  • Owner changed from stephen to muks

Reviewed commit c1c4ea8d1b7629dac379f192d30cc79535214236.

All OK, please merge.

comment:11 Changed 6 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit ff7fa9a2025f682ee668f64197523ab7230b8a85:

* c1c4ea8 [2000] Stop using API that was removed
* f9daba2 [2000] Add API documentation for specialized methods and PseudoRR class
* 6e87315 [2000] Update option code to be different from option length
* bb87247 [2000] Start comments with a capital letter
* 8f85863 [2000] Fix spelling mistake
* 916893a [2000] Add another toWire() test
* 0408d1e [2000] Add OPT::toWire() implementations
* a001951 [2000] Add appendPseudoRR() unittests
* e2c4caa [2000] Add more fromWire() tests
* b6875c7 [2000] Update OPT RDATA parser to handle EDNS options

Resolving as fixed. Thank you for the reviews Stephen.

Note: See TracTickets for help on using tickets.