Opened 7 years ago

Closed 7 years ago

#2164 closed task (fixed)

Introduce RRset::getRRsigDataCount()

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20120821
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: scalable inmemory
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is one building block of #2163. It's similar to getRdataCount(),
but works on the stored RRSIG (if any), i.e., returns the number of
RRSIG RRs associated with the RRset. If no RRSIG is attached,
it will return 0.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:2 Changed 7 years ago by jelte

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

comment:3 Changed 7 years ago by muks

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

Picking

comment:4 Changed 7 years ago by muks

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

Up for review.

Tests were first added for RRSIG related parts of RRset class. There were no tests for these methods.

comment:5 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to muks

Looks good, just two minor comments;

  • since rrsig_ is a shared_ptr, you can just do 'if (rrsig_) {'
  • perhaps we can also add a test to make sure getSigRdataCount() returns 0 after removeRRSig() is called

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jelte

Hi Jelte

Replying to jelte:

  • since rrsig_ is a shared_ptr, you can just do 'if (rrsig_) {'

Done. :)

  • perhaps we can also add a test to make sure getSigRdataCount() returns 0 after removeRRSig() is called

Done. :) This one made me check Message::removeRRset() and there's a bug in it that is untested (in the existing implementation), but #2165 updates that code, so I have left it as-is in this branch.

Last edited 7 years ago by muks (previous) (diff)

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

Replying to muks:

Hi Jelte

Replying to jelte:

  • since rrsig_ is a shared_ptr, you can just do 'if (rrsig_) {'

Done. :)

  • perhaps we can also add a test to make sure getSigRdataCount() returns 0 after removeRRSig() is called

Done. :) This one made me check Message::removeRRset() and there's a bug in it that is untested (in the existing implementation), but #2165 updates that code, so I have left it as-is in this branch.

comment:9 Changed 7 years ago by muks

Sorry I replied instead of editing my last-but-one comment. :)

comment:10 Changed 7 years ago by muks

I have added one more patch to the branch which tests Message::toWire() with RRSIG records. Though it's strictly not a part of this bug, it has to be done before #2165 (the next bug in this meta bug series) so that its branch can be tested with it. Such a test was missing. Very necessary because make check passes cleanly without it, even if no RRSIG were rendered.

Last edited 7 years ago by muks (previous) (diff)

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

  • Owner changed from jelte to muks

The change removes this line from the testdata Makefile.am:
EXTRA_DIST += message_toWire4.spec message_toWire5.spec

I assume that is an accident :)

The dynamic cast in the added test in message_unittest makes me think that this new method should probably go into the AbstractRRset interface as well.

For the rest it looks fine :)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by muks

Hi Jelte

Replying to jelte:

The change removes this line from the testdata Makefile.am:
EXTRA_DIST += message_toWire4.spec message_toWire5.spec

I assume that is an accident :)

Eek! Fixed. :)

The dynamic cast in the added test in message_unittest makes me think that this new method should probably go into the AbstractRRset interface as well.

I wondered where to put it as well, as the RRSIGs are really handled only in the RRset class. But you tipped the balance in AbstractRRset's favour. :) It also avoids the dynamic_cast, so I agree this is better.

The method was also renamed to be more consistent with the rest of the RRSIG related methods of AbstractRRset.

Also a minor change adding virtual to a method was made.

comment:13 Changed 7 years ago by muks

  • Summary changed from introduce RRset::getSIGRdataCount() to Introduce RRset::getRRsigDataCount()

comment:14 Changed 7 years ago by muks

  • Owner changed from muks to jelte

comment:15 in reply to: ↑ 12 Changed 7 years ago by jinmei

Replying to muks:

The change removes this line from the testdata Makefile.am:
EXTRA_DIST += message_toWire4.spec message_toWire5.spec

I assume that is an accident :)

Eek! Fixed. :)

The dynamic cast in the added test in message_unittest makes me think that this new method should probably go into the AbstractRRset interface as well.

I wondered where to put it as well, as the RRSIGs are really handled only in the RRset class. But you tipped the balance in AbstractRRset's favour. :) It also avoids the dynamic_cast, so I agree this is better.

Ah, actually, it was the intent. It's intended to be defined in the
abstract base class from the beginning. Maybe the subject line was
not really clear.

comment:16 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

Ok, looking good :) please go ahead and merge

comment:17 Changed 7 years ago by muks

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

Merged to master in commit 199af97d473d4b67b8a9a631296f543b92ef8fcc:

* d8e429d [2164] Add getRRsigDataCount() implementation to RBNodeRRset too
* eaef6b2 [2164] Rename method to getRRsigDataCount() and add it to AbstractRRset's interface
* 2bf182f [2164] Add virtual to the RRset::getRRsig() method
* 4232ef3 [2164] Add back EXTRA_DIST effects that were removed by mistake
* 8e4054a [2164] Add toWire() test for Message class with RRSIG records
* 98ce97d [2164] Add another testcase when RRSIGs are removed
* 3ed457d [2164] Update syntax of rrsig_ test as it is a shared_ptr
* ceec0dc [2164] Add RRset::getSIGRdataCount()
* 0c63626 [master] Add tests for RRSIG related parts of RRset class
* 932c5ac [master] Untabify code

Resolving as fixed. Thank you for the reviews Jelte.

Note: See TracTickets for help on using tickets.