Opened 7 years ago

Closed 7 years ago

#2360 closed defect (fixed)

RRset::toText() should be consistent about included RRSIG

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121106
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 (last modified by jinmei)

If I understand the code correctly, AbstractRRset::toText() and
TreeNodeRRset::toText() show different behaviors when the RRset
includes an RRSIG in it: The former doesn't print the RRSIG while the
latter does.

Obviously this must be consistent.

The fix is easy, but the main question is which behavior. I
personally suggest the TreeNodeRRset approach. It's also consistent
with the toWire() behavior, and IMO it's more intuitive for the
concept of "signed RRset". Optionally we might provide an option to
control the behavior (or provide a different version of toText()), on
which I don't have a specific opinion yet.

Don't forget updating the documentation to clarify this point (and
also for toWire()), too.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 7 years ago by jelte

IMO, we should simply include the signatures for this ticket, and implement toText() options in a different one, should the need arise (there are more options we can think of than only whether to include signatures in the output string)

comment:4 Changed 7 years ago by muks

I think this would also involve updating the DNS test code in testutils as it assumes the RRset behavior.

comment:5 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121106

comment:6 Changed 7 years ago by muks

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

Picking

comment:7 Changed 7 years ago by muks

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

Up for review.

comment:8 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:9 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to muks

looks good. One trivial issue is that in the added line of doxygen for toText() it says 'rendered', while the rest of the doxygen block talks about conversion, and perhaps it should say 'are also added to the resulting string' or something similar.

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

  • Owner changed from muks to jelte

Replying to jelte:

looks good. One trivial issue is that in the added line of doxygen for toText() it says 'rendered', while the rest of the doxygen block talks about conversion, and perhaps it should say 'are also added to the resulting string' or something similar.

Done. :)

comment:11 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

ok, looks good, please merge

comment:12 Changed 7 years ago by muks

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

Merged to master branch in commit 5a9bc9bc1467bfd5d0be7de9db40824a967be9b0:

* 1114159 [2360] Update wording in API doc comment for RRset::toText()
* ad46427 [2360] Also render any associated RRSIG in RRset::toText()
* 42c8b84 [2360] Test that RRset::toText() returns any associated RRSIG too
* 2c50591 [master] Delete trailing whitespace

Resolving as fixed. Thank you for the reviews Jelte.

Note: See TracTickets for help on using tickets.