Opened 9 years ago

Closed 9 years ago

#1128 closed enhancement (complete)

RR type implementation: SRV

Reported by: shane Owned by: zzchen_pku
Priority: medium Milestone: Sprint-20110802
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

Implement the SRV type, from RFC 2782.

See ticket #809 for more discussion.

Since this RR type is actually used in modern environments, I'm making this "major" priority.

Subtickets

Change History (15)

comment:1 Changed 9 years ago by stephen

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

comment:2 Changed 9 years ago by zzchen_pku

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

comment:3 Changed 9 years ago by zzchen_pku

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

trac1128 is ready for review now.

comment:4 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to zzchen_pku

I don't think the double casts in toText() are necessary, shouldn't using the dynamic casts directly work as well?

The inclusion of <string> in srv_33.h seems unnecessary, as do rrtype and rrttl if i'm reading this correctly.

And one last thing, I think the files are in the wrong directory, they should be in in_1/ instead of generic/ (rfc2782 says the type is defined for class IN only, not for all classes)

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

Not intending to steal the review process, but as I happen to notice a
couple of things...

  • The "from string" constructor incorrectly accepts this:
        generic::SRV("1 5 1a.example.com.");
    
  • Documentation is missing overall. (Don't follow the bad existing practice:-). See, e.g., the tsig_250 implementation for what's expected.

comment:7 in reply to: ↑ 5 Changed 9 years ago by zzchen_pku

Replying to jelte:

I don't think the double casts in toText() are necessary, shouldn't using the dynamic casts directly work as well?

The inclusion of <string> in srv_33.h seems unnecessary, as do rrtype and rrttl if i'm reading this correctly.

And one last thing, I think the files are in the wrong directory, they should be in in_1/ instead of generic/ (rfc2782 says the type is defined for class IN only, not for all classes)

Updated,please check.

comment:8 in reply to: ↑ 6 Changed 9 years ago by zzchen_pku

Replying to jinmei:

Not intending to steal the review process, but as I happen to notice a
couple of things...

  • The "from string" constructor incorrectly accepts this:
        generic::SRV("1 5 1a.example.com.");
    

Fixed.
I reused getToken() and tokenToNum() from TSIG, and I think the two methods can be put into utils, how do you think about it?

  • Documentation is missing overall. (Don't follow the bad existing practice:-). See, e.g., the tsig_250 implementation for what's expected.

Done.

comment:9 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jelte

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

  • Owner changed from jelte to zzchen_pku

I'll create a new ticket for that refactor of getToken() and tokenToNum() (i have a proposed patch)

I've pushed one small change to a Makefile (EXTRA_DIST values weren't updated after the moving of the source files), to make distcheck work.

IMO it's ready to merge, though we may want to ask Jinmei if he's happy with the documentation now.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

Replying to jelte:

I'll create a new ticket for that refactor of getToken() and tokenToNum() (i have a proposed patch)

I've pushed one small change to a Makefile (EXTRA_DIST values weren't updated after the moving of the source files), to make distcheck work.

IMO it's ready to merge, though we may want to ask Jinmei if he's happy with the documentation now.

Ok, I'll reassign it to jinmei.
Thanks for your review.

comment:12 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

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

Replying to zzchen_pku:

I'll create a new ticket for that refactor of getToken() and tokenToNum() (i have a proposed patch)

I've pushed one small change to a Makefile (EXTRA_DIST values weren't updated after the moving of the source files), to make distcheck work.

IMO it's ready to merge, though we may want to ask Jinmei if he's happy with the documentation now.

Ok, I'll reassign it to jinmei.
Thanks for your review.

Comment updates look mostly okay, but I noticed one error (the class
description wasn't close enough to the definition) and one possible
clarification (why we accept compressed targets), and made proposed
fixes. BTW I'd suggest running doxygen when you make non trivial
changes to the documentation (like this one, which is a new set of
doc); the kind of errors that I noticed can be easily detected by
quickly browsing the doxygen output.

I'm okay with the doc with my proposed changes.

comment:14 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:15 in reply to: ↑ 13 Changed 9 years ago by zzchen_pku

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

Replying to jinmei:

Comment updates look mostly okay, but I noticed one error (the class
description wasn't close enough to the definition) and one possible
clarification (why we accept compressed targets), and made proposed
fixes. BTW I'd suggest running doxygen when you make non trivial
changes to the documentation (like this one, which is a new set of
doc); the kind of errors that I noticed can be easily detected by
quickly browsing the doxygen output.

I'm okay with the doc with my proposed changes.

Okay, got it.
Merged, thanks.

Note: See TracTickets for help on using tickets.