Opened 10 years ago

Closed 9 years ago

#44 closed task (invalid)

review: src/lib/dns/cpp

Reported by: jreed Owned by: jinmei
Priority: very high Milestone: 02. Running, functional authoritative-only server
Component: Unclassified Version:
Keywords: review 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

Review what is in trunk as of revision 738 following code review procedures. If this is your code, please add notes here as appropriate. I initially assigned this to the code owner, but this ticket will be assigned to a reviewer. If there is already code review in process, please refer to the other ticket number.

I understand most of this already was reviewed -- at this time I did't attempt to list the revisions that were reviewed. This needs to be done.

Subtickets

Change History (6)

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

Review what is in trunk as of revision 738 following code review
procedures. If this is your code, please add notes here as appropriate. I
initially assigned this to the code owner, but this ticket will be
assigned to a reviewer. If there is already code review in process, please
refer to the other ticket number.

I understand most of this already was reviewed -- at this time I did't
attempt to list the revisions that were reviewed. This needs to be done.

Actually, most of this was NOT reviewed. We should get the following
reviewed:

  • base64
  • buffer/messagerenderer (some minor additions have been made since they were last reviewed)
  • question/rrset
  • message
  • rdata (under cpp)
  • rdata/*/*.{cc,h} specific derived classes for well-known RR types
  • rrparameterregistry (once reviewed, but has been modified substantially)

These are quite functional, but most of them are not in a reviewable
state: I'll need to perform overall cleanup, complete minor missing
features, add more tests, and write a decent (minimum?) level of
documentation.

I plan to do this for each of them, and ask for review one by one (so
that each review process will be sufficiently small to manage).

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

Replying to jinmei:

I plan to do this for each of them, and ask for review one by one (so
that each review process will be sufficiently small to manage).

Updated the current status.

I've created child tickets for reviews on new and updated parts of the DNS message library: #91, #94, #95.

There's still immature code, mainly those incorporated for DNSSEC support. See details below. I'm not sure what we can/should do for them for the year1 deliverable. Are we going to ship them with this level of maturity?

base32.{cc,h}

need detailed review. need more tests

base64.{cc,h}

need review
doc is missing, which will be a post year1 fodder (or we might simply migrate to crypto++, and drop the boost version)

dnssectime.h

need doc (post year1 fodder)

hex.{h,cc}

need more detailed review
need some more tests (such as odd-byte input)
need doc (post year1 fodder)
note: we might simply migrate to crypto++

sha1

we need to replace it
in any case, this code hasn't been reviewed yet

RDATA

general: docs are missing. it's a post year1 fodder

dnskey: still immature. gettag code is still naive about algorithm 1 (do we need to support it this time?). there are bugs. failing test has been added.

ds: still immature, there are bugs. failing test has been added.
nsec3: need review. very complicated, so it's likely to have bugs. failing tests for some trivial bugs have been added.
nsec: ditto
rrsig: still immature, there are bugs. failing test has been added.
soa: still has failed test

comment:3 Changed 10 years ago by jinmei

I've updated this ticket recently, but just in case here's the latest plan.

At this stage I'm afraid we need to admit we cannot review everything. My plan is to fix at least missing validation bugs in the "from wire" constructors and let the other things go with a note they are not (fully) reviewed.

comment:4 follow-up: Changed 10 years ago by jreed

jinmei: What do you want to do with this ticket? Is this just waiting on ticket #94 ?

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

Replying to jreed:

jinmei: What do you want to do with this ticket? Is this just waiting on ticket #94 ?

No, #94 is rather a minor part in this context. Much of the "immature code" (as noted in https://bind10.isc.org/ticket/44?replyto=4#comment:2) is still immature.

The problem of this ticket is it's too generic. The DNS library module is sufficiently big, and it doesn't make sense to have a single review for it.

comment:6 Changed 9 years ago by shane

  • Resolution set to invalid
  • Status changed from new to closed

Since this ticket doesn't make sense, I'm going to resolve this ticket. We can discuss state of code reviews, as we always do, for the 3rd incremental release.

Note: See TracTickets for help on using tickets.