Opened 10 years ago

Closed 10 years ago

#91 closed task (fixed)

review: DNS Message class

Reported by: jinmei Owned by: jelte
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

src/lib/dns/message.{cc,h} and src/lib/dns/tests/message_unittest.cc need review.

We'll certainly need to revisit the design, but for the year1 release we need a review to identify obvious bugs within the current design.

Subtickets

Change History (3)

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

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

Ok, looked at it, i agree with about all TODO, and TBD's :)

MessageFlag::getBit()

since it's a public function it could also use a description
(does it return bits set? default bits? location of bit?)

Opcodes: i'm still not entirely sure why we can't make constants
for these, so we don't need the () in things like
opcode == Opcode::Query() (same for all the others)

Rcode:

Extended Rcodes follow (EDNS required)

Make that TODO? :)

string Rcode::toText() const; should that be
const string Rcode::toText() const;?

Kind of skipped Section because of the notes above it.

DEFAULT_MAX_UDPSIZE; does this mean we actually default to udpsize 512
for EDNS0 messages? I see that auth sets it himself, would we want to
make a note of this in the api doc? (at the class description of
Message)

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

  • Owner changed from jinmei to jelte

Thanks for the review. I've addressed (at least some of) the comments in r1521.

Some responses to the comments:

Opcodes: i'm still not entirely sure why we can't make constants
for these, so we don't need the () in things like
opcode == Opcode::Query() (same for all the others)

I understand this is a general DISCUSS point that others are also concerned about. Let's revisit it post year1. For the year1 release I keep the current code.

Extended Rcodes follow (EDNS required)

Make that TODO? :)

Hmm, not sure about the point of this comment, but since BADVERS is already there I don't see any TODO here.

string Rcode::toText() const; should that be
const string Rcode::toText() const;?

No, in this case the returned string is an object, not a reference or pointer, so 'const' isn't necessary.

comment:3 Changed 10 years ago by jinmei

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

confirmed by the reviewer. closing.

Note: See TracTickets for help on using tickets.