Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#1396 closed enhancement (fixed)

Rdata::getLength() for use with xfrout

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:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

It would be useful if the libdns++/isc.dns Rdata class has a
"get length" method, that returns the length of the RDATA in wire
format (the possible maximum length, i.e., assuming names (if
included) are not compressed).

We can then use it in xfrout to calculate the expected length of
resonse messages (so they won't exceed 64KB).

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 6 years ago by muks

There are two ways to implement this:

  • The non-optimal way is to render it to a buffer and get its length after that. This is easy as it can be done in the RDATA class alone and use existing RDATA implementations' toWire() methods.
  • The lengthy way which would perform better is to add implementations of getWireDataLength() method to every RDATA type.

comment:3 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Status changed from new to reviewing

Up for review.

comment:4 Changed 6 years ago by muks

  • Summary changed from suggested extension: Rdata::getLentgh and use it for xfrout to Rdata::getLength() for use with xfrout

comment:5 Changed 6 years ago by muks

No ChangeLog is necessary.

comment:6 Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

I'm not sure what the point of this ticket is. xfrout already has get_rrset_len(). So all you've done is duplicate the same (non-optimal) behavior from the application that actually uses it, down into the base data class, without actually calling the new duplicated code.

If this will ever be used by anything other than xfrout, or if adding getWireLength() to every rdata type is measurably more efficient than the naive method in xfrout, then this might be worth pursuing. Otherwise, I'd be inclined to close it as wontfix.

comment:7 follow-up: Changed 6 years ago by muks

Hi Paul

Rendering to wire data to calculate lengths is very expensive. I am
guessing this is why Jinmei created this ticket, and this functionality
belongs in Rdata where derived types can return the required length in
constant time. We should not close it as wontfix.

This being a 3 point ticket, I didn't know how much was to be
implemented as part of it.

  • Adding type-specific getLength() implementations is a much larger task, and this should at least be put off until the RR types in review are first merged to master.
  • getLength() is virtual (with a default implementation) so that such type-specific implementations can be added later.
  • xfrout should be modified to use this newly introduced method. I don't know if this ticket includes doing that (the description says "we can use it in xfrout" but not if it should be done in this ticket).

To avoid confusion, I'll edit the branch further to update RRset to
have a getWireLength() method, add a Python binding for it and and
update xfrout to use this binding.

Adding implementations of Rdata should be done only after the current
RR types in review are merged to master. As there is a default
implementation, they will not cause a compile error if a derived
implementation is missing (though I suppose the default implementation
can be removed once all derived types have implementations). It is also
a much larger task.

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

Replying to muks:

Adding implementations of Rdata should be done only after the current...

Sorry, that should read:

Adding implementations of Rdata::getLength() should be done only after the current...

comment:9 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

Up for review. Even without the RR type specific implementations of Rdata::getLength(), this would already perform marginally better as it avoids rendering RR fields except RDATA.

comment:10 Changed 6 years ago by muks

I forgot to mention: the following changes were made to the branch:

  • Add RRset::getLength() + tests
  • Python binding + tests
  • Add TreeNodeRRset::getLength() + tests
  • Update xfrout code to use it

comment:11 Changed 6 years ago by pselkirk

  • Owner changed from pselkirk to muks

Looks good, please merge.

comment:12 Changed 6 years ago by muks

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

Merged to master branch in commit 2002e7a128f2b4d84d0f67a45e8e0fec2bf541c3:

* 1e762fa [1396] Use RRset::getLength() in xfrout
* 63eca99 [1396] Fix width of TTL field
* 6f177a3 [1396] Add TreenodeRRset::getLength() implementation
* 5410f9c [1396] Add Python binding for RRset::getLength()
* 000f7ef [1396] Add RRset::getLength()
* 7f9bc84 [1396] Rename method to getLength()
* 4b77db4 Add Rdata::getWireLength() method

Resolving as fixed. Thank you for the reviews Paul.

comment:13 Changed 6 years ago by muks

#3311 was created for adding type-specific Rdata::getLength() implementations.

Note: See TracTickets for help on using tickets.