Opened 10 years ago

Closed 10 years ago

#96 closed task (fixed)

review: some trivial RDATA classes

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: 03. 1st Incremental Release
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

There are non DNSSEC related RDATA implementations that haven't been reviewed. These should be reviewed.

I believe the implementation is relatively simple and can be easily reviewed (relatively).

Those classes are: rdata::generic::TXT, rdata::in::A, and rdata::in::AAAA.

Files to be reviewed are:
trunk/src/lib/dns/rdata/generic/txt_16.{h,cc}
trunk/src/lib/dns/rdata/in_1/a_1.{h,cc}
trunk/src/lib/dns/rdata/in_1/aaaa_1.{h,cc}
trunk/src/lib/dns/tests/rdata_txt_unittest.cc
trunk/src/lib/dns/tests/rdata_in_a_unittest.cc
trunk/src/lib/dns/tests/rdata_in_aaaa_unittest.cc

I'm assigning this ticket to Shane as the review dispatcher.

Subtickets

Change History (6)

comment:1 Changed 10 years ago by shane

  • Owner changed from shane to jelte
  • Status changed from new to assigned

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

  • Owner changed from jelte to jinmei

I would like to see some documentation on how the gen script actually
works :)

txt_16.cc:

wire-constructor, seems to me it's quite trivial to support multiple
character-strings here (with a 'while data left' loop)

string constructor; should we fail on the string "foo\"? (i think so,
if we are going to change this part to also accept multiple string i
think this will have to change anyway)

toText; change XXX into TODO :)

a_1.cc:

if we try to construct from string, and fail, should we add a copy of
the source string in the exception?
(failed to parse IPv4 Address: foobar)
Depending on how this is called, and if the caller keeps track here,
this might not be necessary.

We could also do with more tests :) (esp. weird TXT strings)

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

  • Owner changed from jinmei to jelte

Thanks for the review. I've addressed the comments in r1517 and r1519. It would be great if you could take a look at it.

(I didn't touch XXX. It's a larger issue and should be addressed project-wide)

As for the point of more test cases, you're right. I've added new tests for malformed TXT input. We may need more, but I hope the current set provides reasonable coverage.

And finally.

I would like to see some documentation on how the gen script actually
works :)

agreed:-) Let's leave it to year2, however.

comment:4 Changed 10 years ago by shane

Bump.

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

  • Owner changed from jelte to jinmei

Those revisions look good, approval granted.

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

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

Replying to jelte:

Those revisions look good, approval granted.

Thanks. Closing it.

Note: See TracTickets for help on using tickets.