Version 25 (modified by jreed, 6 years ago) (diff)

updated as discussed on mailing list


All BIND 10 code must be reviewed by another coder on the BIND 10 team before it may be merged into the main trunk.

Typically a piece of code will be created in a branch, and worked on until it is thought to be ready by the coder working on it. When this happens it is reviewed, as described below.

Starting the Review Process

When a coder has a piece of code that is "done", then they must set the ticket "Action" for "review to" to UnAssigned in the drop-down list (or assign it to a developer who has agreed to review it).

During a sprint, the developers will review the tickets to see what review tickets are unassigned.

The coder can ask someone to become the reviewer, or ask the Programme Manager to pick someone to review the code. If the Programme Manager does this, he or she will assign the ticket and add an explaination of what is expected; this will cause an e-mail from Trac to be sent. It is not necessary to copy the Programme Manager on any follow-up mails between the coder and reviewer.


Reviewing the Code

When the reviewer is ready for the code, he or she takes ownership of the ticket on the Trac site.

The code should be reviewed to make sure it is clear, correct, and follows BIND 10 coding guidelines.

A short checklist:

  • General
    1. Building and testing the code should work ("make distcheck" will perform this check). (Also run lettuce or other system tests as applicable.)
    2. The code must have Documentation (for the coder or user as appropriate)
    3. The CodingGuidelines must be followed
    4. There must be automated unit tests
    5. Tests should avoid introducing delays, such as sleep(), as much as possible.
    6. Installation should work
    7. In test cases and examples in documentation, documentation IPv4/IPv6 prefixes ( and 2001:db8::/32) should be used.
    8. If the change would require a ChangeLog entry, the entry must be prepared before the review and it must get reviewed, too. See ChangeLogDetails for the ChangeLog entry format. (see also the relevant discussion on the dev list:
  • C++ Code
    1. Is the code exception safe? The reviewer should be careful especially about allocating resources by operator new or low-level system interfaces such as (socket(2)).
  • For API's
    1. Does the API make sense?
    2. Are there functions missing?
    3. Are there functions unnecessary?
    4. Does the documentation specify exceptions and memory allocations?
  • For code, read through the source code and try to find anything that might 'not work' or is not understandable. Some hints:
    1. Are there any internally undocumented weird steps performed?
    2. Are there hard-coded constants that should have a defined name?
    3. Are design choices written down?
    4. Are branches, switch statements and loop conditions correct?
  • Documentation
    1. When you add/delete/modify a configuration/command option to a module, update the corresponding XML manual such as b10-xfrin.xml with an example of how to specifically configure it with bindctl.

Anything about the code is fair game, as long as it fits in the BIND 10 style and is intended to make the software better.

Any questions or concerns should be added to the ticket, and then ownership passed back to the coder.


The ticket may pass back and forth between the coder and the reviewer several times as it is discussed. This is quite normal, especially for large and/or complicated tickets.

Comments about the review should be added to the ticket.

Ticket ownership matters! A reviewer may make several comments, especially in a long review process. The signal to the coder that a given review pass is complete is getting ownership of the ticket back. Likewise when the coder thinks that all of the issues have been addressed, the signal to the reviewer is getting ownership of the ticket back. When the review is complete, the developer may choose to copy-and-paste the above checklist in the reply and note "YES" for each item checked or explain why not checked or about any issues.

If problems arise in the process - for example due to unclear guidelines about how things should work, or disagreements about what the best way forward in a given case is - then the issue can be brought to the mailing list or to the Programme Manager.

Completing the Review Process

When all issues have been resolved, the reviewer adds an entry to the ticket noting that, and passes ownership back to the coder.

The code is then ready for merge. Branching and merging procedures can be found in the GitGuidelines page. The ChangeLog file is updated. After this merge is complete, the ticket may be closed. Depending on the scope of the change, either a congratulatory cup of caffeinated liquid or a bubbly alcoholic beverage may be imbibed.


The reviewer should provide feedback within 5 working days of receiving the review. For a complex review, the review may take longer than 5 days, but the coder needs to know that someone is working on it.

About this Document

Created by Shane 2009-12-18
Reviewed on 2013-01-02
Review scheduled 2013-04-01