Opened 8 years ago

Closed 6 years ago

#1397 closed enhancement (fixed)

Add RRset::addRdata(string) method

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

Description

I've found we often need to create RRsets from string especially in
tests. In such cases it would be more convenient if we could directly
pass a string for RDATA to the RRset object, rather than creating
an Rdata object and passing it to RRset.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 6 years ago by muks

  • Summary changed from suggested extension: RRset::addRdata(string) to Add RRset::addRdata(string) method

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

No ChangeLog entry is necessary.

comment:5 Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

This seems okay as far as it goes, but the new method is only called by a single new unittest. In order to prove that this is useful, we could convert existing tests to use this slightly optimized method. Or we could spend our time fixing real bugs.

comment:6 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

The branch doesn't modify existing testcases. The ticket is for adding the method to be usable in future tests (we keep adding support for new RR types).

We have quite an extensive backlog, and the point about prioritizing is well taken. But we need to get rid of the rest of the backlog too, and add these facilities that are useful in new code. In the past, we were chasing feature goals all the time and we put off work on such things forever.

Is the branch fine to merge?

comment:7 Changed 6 years ago by pselkirk

  • Owner changed from pselkirk to muks

Yes, please merge.

comment:8 Changed 6 years ago by muks

The branch does not compile. I missed observing this during development as make check had passed in src/lib/dns/ but I forgot to run a make check at toplevel.

Did you build and check the tree during the review? Please go through the checklist at:
http://bind10.isc.org/wiki/CodeReviewProcedure

comment:9 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

I've pushed a commit to fix it to trac1397. Please review it.

comment:10 Changed 6 years ago by pselkirk

  • Owner changed from pselkirk to muks

I generally run make check as part of my review, although that started failing in SegmentObjectHolderTest.grow until I disabled that test. So this may have been one branch where the check didn't complete. In any case, it passes distcheck now. The latest commit is good, and it's nice to see a unit test for the new addRdata. Okay to 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 cc0fc600dfdc1f4652aa7ec5a7472db06c1cbd4d:

* 30c527e [1397] Add missing TreeNodeRRset::addRdata() implementation for new interface
* 6db6928 [1397] Add std::string version of Rset::addRdata()
* 0022aaf [1397] Fix variable that's tested
* 7417af2 [1397] Change EXPECT_EQ to ASSERT_EQ

There was a merge conflict which was resolved and reviewed by Shane on Jabber.

Resolving as fixed. Thank you for the review Paul.

Note: See TracTickets for help on using tickets.