Opened 9 years ago

Closed 9 years ago

#806 closed enhancement (complete)

Implement RP record type

Reported by: stephen Owned by: jinmei
Priority: high Milestone: Sprint-20110419
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Change History (11)

comment:1 Changed 9 years ago by shane

  • Priority changed from major to critical

comment:2 Changed 9 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:3 Changed 9 years ago by jinmei

Branch trac806 is ready for review.

Proposed changelog entry:

218.	[func]		jinmei
	src/lib/dns: added support for RP RDATA.
	(Trac #806, git TBD)

comment:4 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:6 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I have two things:

  • The comment in toWireRenderer seems misleading: // similar to toWireBuffer, but names in RDATA should be compressed. Actually, the names must not be compressed (as noted in the code) and the testdata have them uncompressed as well, the comment suggests that they should be compressed.
  • The build fails for me with ../../../../src/lib/dns/.libs/libdns++.so: undefined reference to `isc::dns::rdata::generic::RP::RP(isc::dns::rdata::generic::RP const&)'. The header of the copy constructor is generated with the common members.

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

Replying to vorner:

I have two things:

  • The comment in toWireRenderer seems misleading: // similar to toWireBuffer, but names in RDATA should be compressed. Actually, the names must not be compressed (as noted in the code) and the testdata have them uncompressed as well, the comment suggests that they should be compressed.

Ah, good catch. It was an error due to naive copy paste. Fixed. (Note: I first thought it was derived from the generic rdata.h and made some editorial fix that is not actually related to this point: commit 31b9619. I've not reverted this cleanup)

  • The build fails for me with ../../../../src/lib/dns/.libs/libdns++.so: undefined reference to `isc::dns::rdata::generic::RP::RP(isc::dns::rdata::generic::RP const&)'. The header of the copy constructor is generated with the common members.

Hmm, it's strange that it didn't cause an error with clang++ (since we cannot use the default copy constructor for the base Rdata class we should define it explicitly). Anyway, the simple and correct fix to this is to define the trivial copy constructor. Done in commit ad04d07.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • The build fails for me with ../../../../src/lib/dns/.libs/libdns++.so: undefined reference to `isc::dns::rdata::generic::RP::RP(isc::dns::rdata::generic::RP const&)'. The header of the copy constructor is generated with the common members.

Hmm, it's strange that it didn't cause an error with clang++ (since we cannot use the default copy constructor for the base Rdata class we should define it explicitly). Anyway, the simple and correct fix to this is to define the trivial copy constructor. Done in commit ad04d07.

Maybe there was no need to copy it anywhere? But then, why g++ did fail? Well, no matter.

So, I think this can be merged.

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

Replying to vorner:

Hmm, it's strange that it didn't cause an error with clang++ (since we cannot use the default copy constructor for the base Rdata class we should define it explicitly). Anyway, the simple and correct fix to this is to define the trivial copy constructor. Done in commit ad04d07.

Maybe there was no need to copy it anywhere? But then, why g++ did fail? Well, no matter.

Actually, when I experimentally removed the declaration of the copy constructor the compiler complained. But it's probably not worth pursuing at least this time. I've simply merged the latest code to master. Will close ticket.

Thanks for the review.

comment:11 Changed 9 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.