coding guidelines

This page documents some coding guidelines for the Kea project. See Stork coding guilelines for UI related guidelines. Also refer to the BIND 9 coding guidelines.

General rules
Python Style
C++ Style
Makefile.am Guidelines
User Interface (UI) Guidelines
IP address and port formatting
Imported Code
Deprecating features
DB schema versions
Guidelines Adopted by Other Projects

General rules

Those apply to all code in all languages.

Line length

Source code not exceeding 80 columns is preferred. This is derived from the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines], mainly for style consistency. However, C++ names (especially with namespaces) are significantly longer, thus strictly adhering to this causes overly wrapped code that is both ugly and difficult to read. Therefore it is permitted to use lines up to 100 columns long.

Rationale: It is 2021 and all ISC employees have equipment that is at most 3 years old. You should have FullHD (or better) display available to you. You should be able to display two files side by side comfortably. If this is a problem, get a larger display.

Tabs & Indentation

Do not use hard tabs.

Indentation at each level is 4 spaces for C++, other languages should use what is "usual and expected."

In C++ we use the BSD style (also from BIND 9), where continuing lines are aligned with the corresponding opening parenthesis, like this:

if (JS_DefineProperty(cx, o, "data",
                      STRING_TO_JSVAL(JS_NewStringCopyN(cx, data, res)),
                      0, 0, JSPROP_ENUMERATE) != 0) {

ChangeLog entries

Every change that is user visible must have a changelog entry. The changelog should be written in a way that could be understood by a knowledgeable user. If the changes comes as a patch, the original author should be asked how he/she would like to be credited. Typically, we use first/last name, but other options (name + company, just the company, just the nickname, anonymous) are fine, too.

Testing and Documentation addresses and prefixes

Use 192.0.2.0/24 and 2001:db8::/32 for purposes like addresses used in test cases or examples in documentation (source: RFC5737 and RFC3849). Likewise, use reserved example domain names such as example.com, .test, .example, etc for domain names used in these cases. They are reserved by specifications and should be the safest in terms of collision avoidance.

Dead code

Dead code is bad; it suffers from code rot, and it looks unclean. There are some circumstances where there is a reason to keep a bit of unused code around for a while, but these should be the exception rather than the rule, and it should be very clear why it is there, and on what conditions and when it will be re-enabled or removed completely.

Any dead code (both files that are unused and blocks of commented-out code) should in principle be removed. If there is a very good reason to keep it around for a while, it must be accompanied by a comment explaining why it is still there, and when it will be removed or enabled again. This comment should point to a ticket so that we do not forget about it.

Language choice

Kea is written in C++11. For scripting elements, such as tools and more complex scripts, Python is the recommended language. Shell should be used only for basic tasks. If any of the following is true, you should use Python, not shell:

the task is expected to get more complicated in the future, e.g. parsing list of API calls and generating documentation
the task requires more complex logic, e.g. building associative arrays that are traversed in various ways
the task is not small enough that it's done in one ticket and then forgotten
if you expect more devs be involved in the future
if the script is complex enough that should have unit-tests

Note: for historical reasons, keactrl as well as shell tests are written in shell. On a retrospect, this was a mistake. Both should have been python scripts.

Python has number of advantages of shell:

it provides access to a very rich set of libraries: JSON parsers, http implementation, process monitoring and more. While all of those could potentially be implemented in shell at great effort, the return of investment is not good.
good testing support. See pytest, unittest and other tools
ISC QA has good, multi-year experience with Forge tool that's written in Python. Stork has system tests written in Python, too.
python does not suffer from the portability problems as much (see bash vs dash vs sh vs tsch vs zsh differences)
python is inherently more efficient in some cases. There is long term plan to provide hooks in python. Shell based hooks are technically possible now, but are way less efficient, due to having to spawn multiple separate processes for each packet.
when python code fails, the default exception handling is very informative. In shell, you need to write extra code to get the basics, like exit code. Getting more descriptive text of what failed and why is often impossible. This give users a better chance to fix something on their own
most devs prefer python over shell
there are better debuggers for python

Python Style

We use python 3. Follow PEP8 if possible.

C++ Style

We use C++11 and in general all features available in the language can be used. However, when introducing more exotic constructs, make sure it does not break on older, but still popular platforms. CentOS and RHEL are particularly known for having stable, but old versions of everything, including compilers and libraries.

File Name Conventions

Use .cc for C++ source files. This is basically a mere preference and to ensure consistency throughout the package.

Use .h for C++ header files. This is because we may want to provide a C-callable wrapper for some APIs, and some C++ header files are to be included in a C source file. In that case C-compatible file names will look more natural.

Use all all-lowercase characters for file names. This is consistent with the current recommendation for python, and so it will make the file name convention consistent throughout the BIND10 source tree. Not mixing lower/upper cases will also help avoid name conflicts in a case insensitive file system. Note that this policy may not compatible with C++ class name convention (see below) if the file name is based on the class name (e.g., name "myclass.cc" for the definition of the "Myclass" class). We explicitly accept the conflict, but note that this means it will effectively prohibit mixing cases in class names ("Myclass" and "!MyClass" may not coexist).

Note header files should not include config.h.

Ordering Include Files

We include our own project headers first, then library, and finally system headers, whenever possible. Each header is expected to have any necessary #include statements it needs, and this helps insure that.

#include <dns/message.h>
#include <boost/shared_ptr.hpp>
#include <string>

There can be several exceptions. Some specific libraries require a particular header file must be included first. A notable example is Python.h. The top level config.h generated by autoconf should also be included before other generic header files.

Include Style

We use the pointy brackets version of #include. Also, we use a full path to the include (relative to some -I switch, not to the current directory).

Correct:

#include <util/threads/thread.h>
#include <dns/name.h>

Incorrect:

#include <thread.h>
#include "name.h"

Curly Braces

Always add braces even for a single-line block:

if (something_holds) {
    perform something;
} else if (nonorthogonal_condition) {
    perform otherthing;
} else { // optionally comment to clarify the fully orthogonal case
    perform finalthing;
}

Opening Curly Braces for Functions

The opening curly brace should occur on the same line as the argument list, unless the argument list is more than one line long.

void
f(int i) {
    // whatever
}

int
g(int i, /* other args here */
  int last_argument)
{
    return (i * i);
}

This was derived from the BIND 9 coding guideline. It's known this style may look awkward (and even may look inconsistent) for some, but for the reason stated at the beginning we follow this style.

Curly Braces for Catch

A catch statement should have braces on a single line, like this:

     .
     .
     .
   } catch (const SomeException& ex) {
     .
     .
     .

Note if the ex parameter is not used it should be omitted. There are only two forms of catch clauses which make sense: a const reference and the ... catch all.

Parentheses

Do put a space after 'return', and also parenthesize the return value.

    return 1;   // BAD
    return (1); // Good

This was derived from the BIND 9 coding guideline.

Operators

Use operator methods in a readable way. In particular, use the following style with operator==:

    if (x == 10) {  // Good
        // do something that has to be done when x is equal to 10
    }

instead of this:

    if (10 == x) {  // BAD
        // do something that has to be done when x is equal to 10
    }

because the former style is much more readable and intuitive for humans. While the latter style might help detect bugs like dropping one = in the expression, modern compilers with proper warning levels can do the same job more comprehensively. This is especially so for cleanly written C++ code (compared to plain old C).

See also developers' discussions at: https://lists.isc.org/pipermail/bind10-dev/2012-March/003266.html

Increment and Decrement operators (++/--)

Use the prefix form of increment/decrement operators by default:

    // for (int i = 0; i < 10; i++) { // No good
    for (int i = 0; i < 10; ++i) { // Good
        // do something for i = 0..10
    }

Preferring the prefix form of these operators is a well known practice for non trivial types due to performance reasons. And, for consistency, we use the same style for basic types like int even if it's mostly a preference matter for such types. By being consistent, it will be easier to notice when we use the less efficient style when it really matters.

Sometimes the context requires the use of postfix form, in which case it's okay to use that form. But if the intent is not obvious from the context, leave a comment about why the different form is used (since it's subjective whether it's "obvious", it's generally a good idea to leave the comment in this case).

Operator Overloading

Operator overloading is allowed when it's considered intuitive and helpful for improving code readability. But care should be taken, because often it could be only intuitive for that developer who introduced it. If it doesn't look intuitive for the reviewer, the developer has responsibility to convince the reviewer; if it fails the default action is to use non operator method/function for that purpose.

It's recommended to define operator<<(std::ostream& os, const TheClass& obj) if TheClass has operator==() and toText() methods. This allows the class can be used in EXPECT_EQ (and its variants) in googletests.

Explicit Constructors

By default C++ constructors with one argument are conversion functions. When they are not (which is the general case) they should be explicit as in:

class Foo {
public:
   // Constructor with one argument
   explicit Foo(std::string name);
};

Class Attributes

Accessors for class attributes should be called getXxx().

Mutators for class attributes should be called setXxx().

(where xxx is the attribute)

Non-copyable Classes

If you want a class to be non-copyable (neither copy constructor nor assignment), inherit from boost::noncopyable rather than implementing this yourself. See boost docs for details.

Naming

Don't start things with underscores. According to Stroustrup's C++ book:

Names starting with an underscore are reserved for special facilities in the implementation and the run-time environment, so such names should not be used in application programs.

Class names are LikeThis, methods are likeThis(), variables are like_this, and constants are LIKE_THIS. Data class members are like_this_.

Enumerations are written as

enum EnumName {
  FOO,
  BAR
} enum_instance;

Note that unless you have a specific reason to set specific values, leave specific values off. These can be written if needed:

enum ExamplePortNumbers {
  DNS = 53,
  DHCP = 68
};

Where to Put Reference and Pointer Operators

In C++, it seems to be more common to not insert a space between the type and the operator:

int* int_var;
int& int_ref;

Sizeof Bool

The C++ standard does not require that sizeof(bool) is one: it is compiler dependent! So it should not be used and sizeof(uint8_t) is recommended instead.

Comments

Multiline comments can be written in C++ or C style (that is, with // or /* */ marks).

/*
 * This is a comment.  It is important probably.
 */

//
// This is a comment.  It is important probably.
//

/* This is also ok. */

// As is this.

Comments at the end of lines should usually be C++ style:

class Foo {
    int bar_length;  // The length of the bar in millimeters.
};

Doxygen Comment Style

When writing a Doxygen special comment block there are several possible styles:

http://www.stack.nl/~dimitri/doxygen/docblocks.html

Doxygen keywords should be prepended with @, not with a backslash. The reason to prefer @ is that backslash may confuse scripts that would go over the code. There is some inconsistency in this regard. There are large parts of older code that still use backslash. It is a matter of personal taste to keep consistency with what is in the file vs. strictly sticking with this principle. Long term we want to use @ everywhere.

We use the C++ style of 3-slashes:

/// A lot of examples are called foo().
///
/// @param baz foo() usually takes an argument
void
foo(Bar baz) {
    ...
}

Make sure inserting a blank line between two function/method declarations or definitions:

class Bad {
   /// @brief Short description for bad1
   void bad1();
   /// @brief Short description for bad2, which may also look for bad1().
   void bad2();
};

class Good {
   /// @brief Short description for good1
   void good1();

   /// @brief Short description for good2, which should be much clearer.
   void good2();
};

Explicit @brief for Doxygen

If you don't use @brief as the first thing in your doxygen comment, then doxygen will turn the first paragraph into a @brief description anyway. However, we include it anyway so that everybody understands that this is the @brief description.

TODO Comments

We sprinkle comments in code with keywords to indicate pending work.

In Kea, ///@todo is preferred. It should be prepended with triple ///, so it will show up on a nicely auto-generated Doxygen todo list. If there is a corresponding ticket, feel free to specify its number in the comment. Unless other wise specified, issue #1234 (closed) means a ticket in the gitlab, available at http://gitlab.isc.org/isc-projects/kea

Methods and Functions

Opening Curly Braces

For methods where the arguments all fit on one line with the curly brace, it should be written on one line:

int
methodName(int argument_one, std::string message) {
    ...
}

Where this is not possible the curly brace should go on a line by itself:

int
methodName(int argument_one, std::string message,
           int another_argument)
{
    ...
}

Virtual Methods

Explicitly add override to method declarations in derived classes:

class Base {
    // this 'virtual' is absolutely necessary
    virtual void foo();
};

class Derived : public Base {
    // 'override' is recommended per this guideline
    // 'virtual' is not necessarily needed here, but it may be added if the
    // class is further derived.
    void foo() override;
};

This way, the compiler will display an error message if the method in the base class is not virtual or a method with the same signature doesn't exist:

error: ‘void Derived::foo()’ marked ‘override’, but does not override

Also, once override is used, if another method in the same translation unit overrides a virtual function, but is not marked accordingly, a compiler warning will be raised:

warning: 'foo' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

override is a C++11 construct. Before C++11 was adopted, virtual was used to signal overriding of methods. There are a few disadvantages with this rule. The compiler doesn't help in enforcing it. There's a false sense of safety around it. And it prevents the developer from declaring the method in the derived class as non-virtual if the design requires it.

Const references

With anything but primitive types (like int or bare pointer), it is better to pass them as const reference when possible, to avoid overhead of calling the copy constructor and copying a lot of data.

This includes smart pointers, some of them can be relatively expensive to copy.

void
function(const boost::shared_ptr<DataType>& param) {
    ...
}

Exception-safe getters and string-production methods

Unless there's a compelling reason to do so neither member value getter methods nor and string-production methods, such as toString(), should throw exceptions. Normally a class member is prevented from ever having an invalid value so there is arguably a getter never has a reason to throw, and string-production methods should always be safe to invoke once a class has been instantiated. Both types of methods are commonly used as log statement arguments where one should not have to worry about catching exceptions.

Namespaces

Namespaces will be lower case: '''isc::dns''', or '''isc::cc'''.

'''using namespace''' should never be used in a header file. They may be used in .cc source files.

Consts

Use const as much as possible. Make class methods const member functions whenever possible.

If a function has a parameter with a "top-level const", make sure the const appears not only in the definition but also in the declaration. For example, if you define the following function in a .cc file:

int
foo(const int param) {
    ...
}

then make sure the const appears in the corresponding declaration in a header file (.h):

int foo(const int param);  // const cannot be omitted

Technically, the latter const is redundant. But !SunStudio C++ compilers have a bug in name mangling that requires the "consistency": http://developers.sun.com/sunstudio/documentation/ss12/mr/READMEs/c++.html#cNameMangling

Unfortunately, we want to be as portable as possible, and so need to work around this by being redundant.

NULL pointer check

Use the "implicit" form when testing to see whether a pointer is 0 (NULL):

    SomeType* ptr;
    if (ptr) { // instead of 'ptr != NULL'
        // do something if the value of ptr is not 0
    }
    if (!ptr) { // instead of 'ptr == NULL'
        // do something if the value of ptr is 0
    }

This is different from what is suggested in the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines]. The guideline was changed for BIND 10 and then adopted in Kea for the following reasons:

The implicit form is more compatible with smart pointers. For example, if we need to write a "non-null check" for a boost::shared_ptr value without its type conversion (to bool) operator, it would look quite ugly:

    boost::shared_ptr<SomeType> ptr;
    if (ptr.get() != NULL) { // ugly
       // do something if the underlying pointer value of ptr is not 0
    }
    if (ptr) { // same effect, more concise
       // ditto
    }

So for smart pointers we definitely want to use the latter, and it's better to use the consistent form for both smart pointers and bare pointers. (And, in any case, it's generally better to minimize direct use of bare pointers).

For some developers the "implicit" form was more intuitive, while others had no strong preference.
This has been one of the most-broken guidelines (perhaps partly because of the incompatibility with the smart pointer usage). If a guideline is so difficult to meet, it probably means we should revisit the guideline rather than pursuing how to enforce it.
The implicit form doesn't weaken code safety.
NULL is usually defined as a macro. In general, it's better to avoid macros in C++ code. And in fact NULL is pure C and formally does not exist in C++...

Note that there are still many NULL checks in the "explicit" form in the source tree as of this writing (April 2013) as we began with the BIND 9 style. New code should follow this guideline, and it's encouraged to update existing code gradually while we update the code for other reasons (bug fix, enhancement, etc). But during the transition period (which will be long) we'll see both forms.

See also a mailing list discussion starting from: https://lists.isc.org/pipermail/bind10-dev/2012-December/004129.html

This guide line was written before nullptr was added to C++. There was a short discussion about switching to nullptr at that time and the conclusion was to not do that. So there should be no nullptr in the Kea internal code (external libraries can have adopted a different position).

Log Statement Safety

It is extremely important to examine all arguments passed into a log statement to ensure they will produce safe values at runtime:

Can the argument (or any part of it) be null? If so is this taken into account?
If the argument invokes any functions, are they exception safe?
If it involves indirection, does this always resolve into a usable value?
If it raises an exception, is the exception caught? This includes double errors, i.e., log statements in an exception handler.
Does it have the intended data type? For example, it is a common oversight that 8-bit integers are displayed as their ASCII character counterpart. To solve this, prepending a unary plus is the recommended solution (cppreference.com: 1. unary plus (promotion). [...] Integral promotion is performed on the operand if it has integral or unscoped enumeration type and determines the type of the result.).

Log statements are less than helpful if they cause the program to segfault or throw.

Google Test Style

Try to use EXPECT_() rather than EXPECT_TRUE():

EXPECT_TRUE(run_time > 1);   // BAD

EXPECT_GT(run_time, 1);      // Good

because when the test fails the latter will provide more detailed information (the values of "actual" and "expected").

Note that this is not always possible, especially when checking an object that has no operator<<() (or more obviously no operator()), which is used to output on failure.

Test naming

Names of tests should be similar to classes and method names for that language. For example:

for gtest C++ tests, you'd use CamelCaseTest as the fixture class name, and testName() as the test name (as it is a C++ method).
for Python unittests, you'd use CamelCaseTest as the fixture class name, and test_something() as the test name (as it is a Python method).

Makefile.am Guidelines

Make portability

Makefiles must never depend on a particular make, for instance the $(<function> ...) construct of GNU make for shell, foreach, wildcard, etc, are forbidden.

Subdirectories

If there is are one or more subdirectories, the first line in the file should be the SUBDIRS entry, e.g.:

SUBDIRS = . tests

If relevant (as is usually the case), the current directory should be included in the list at the appropriate point in the list.

Preprocessor flags / includes

Next should be the setting of the AM_CPPFLAGS, which applies to every module built with C++ in the Makefile.am, e.g.:

AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
AM_CPPFLAGS += $(BOOST_INCLUDES)

This can be overridden on a per-module basis in the file by specifying the variable module_CPPFLAGS. (Note that if the CPPFLAGS environment variable is set, its flags are appended to the command line after AM_CPPFLAGS.)

Directory variables should be relative (i.e., no abs_top_builddir) for efficiency. Third (or more) levels should not be used, e.g.:

NO += -I$(top_srcdir)/src/lib/dns -I$(top_builddir)/src/lib/dns

AM_CXXFLAGS should be initialized to KEA_CXXFLAGS, e.g.:

AM_CXXFLAGS = $(KEA_CXXFLAGS)

Linker flags

When executables are build, AM_LDFLAGS must be conditionally set to static, e.g.:

if USE_STATIC_LINK
AM_LDFLAGS = -static
endif

Library dependencies

With the exception of archives, (dynamic) libraries and executables should be linked with all dependencies in the opposite order of src/lib/Makefile.am for shared libraries (so finishing by libkea-exceptions.la) followed by external dependencies. The idea is to not rely on libtool which has shown some problems handling dependencies.

Dynamic Shared Objects built as modules to be dynamically loaded must have no dependency when static linking is enforced: their undefined symbols should be resolved using executables they are loaded into.

If the libcfgrpt is used for embedding config_report it must be linked first, in particular if it is linked after libprocess some linkers can believe the config_report symbol is resolved by libprocess and does not include its static definition.

User Interface (UI) Guidelines

Kea is a server process, so does not have much that would be considered a user interface. This section discusses what we do have. See Stork coding guidelines for UI guidelines for Stork.

IP address and port formatting

Whenever an IP address and port is output, IETF RFC2396 and RFC2732 should be followed.

For IPv4 addresses, this looks like this:

192.0.2.1:53

For IPv6 addresses, this looks like this:

Imported Code

If you import code from another project, try to continue the style of the imported project if changes need to be made. This is for two reasons, one is to make merging future versions easier. The other is the encouragement of submitting changes upstream.

Importing code is not a decision that can be taken lightly. Please discuss this on a meeting first. Consider the following aspects:

are the license compatible? Will this require adding extra text to COPYING? Will this complicate the licensing even further?
what's the update policy should be? Should we keep updating the code every time there's a new upstream release?
why can't we develop the code on our own?
what are the implication for building? Will this import make Kea compilation more complex, especially for people who don't care about the new feature?

Deprecating features

Sometimes we need to introduce changes that are not backward compatible. If possible we try to support the old way while making the new way possible. When introducing new alternative ("new way") that eventually going to replace an existing option ("old way"), there should be at least one stable series which supports both old and new way. If old syntax is used, a warning should be printed urging people to update their syntax.

For example: The old reservation-mode parameter has been replaced with reservations-global, reservations-in-subnet and reservations-out-of-pool in 1.9.2. A warning should be printed if reservation-mode is used. Such a warning should be coded as soon as realistically possible to give people as much advance warning as possible. As such, this will be implemented in 1.9.2, so early adopters will see it right way. It will be included in 2.0, where users preferring stable will see it. The old way can be removed somewhere in 2.1.x, so 2.2 will not accept it anymore. This way people who use only stable version will have time to migrate. They'll see the warning when moving from 1.8 to 2.0 and will have time until 2.2.

DB schema versions

For a long time Kea had a concept of having major and minor versions of the schema, with minor being bumped for backward compatible changes. The original idea was that it would be possible to move back and forth between releases. This was never implemented and never works, as all the back ends made explicit check that both major and minor must match. Also, it is often questionable what is and what is not a backward incompatible change. Finally, we never had any chance to test this backward compatibility. As such, around June 2021, we decided to move to a simpler scheme.

When a schema is changed, we always bump major version and minor always remains at zero. Technically, we could get rid of the minor version, but we want to keep it for backward compatibility cough. See rationale here.

Historic upgrade scripts cannot be changed, unless there is a very serious problem that, if unfixed, would prevent people from using Kea. An example of a critical problem: the upgrade script fails on new version of MySQL or PostgreSQL and thus prevents Kea from being deployed.

When adding new schema, it is OK to edit it until it is released. Once released and users start using it, you can't edit anymore and any changes require introducing new schema version. If you feel that dealing with new schema upgrade script it too much, maybe the problem you're trying to solve is not significant after all? Remember that schema upgrade is troublesome for potentially thousands of users. Do not update schema, unless you really have to.

Rationale: the whole point of having schema version is to uniquely determine what is in the schema. If we release it schema X and later update the scripts, there will be some deployments with using original schema and others using the modified schema, both identified as X.

Guidelines Adopted by Other Projects

Other projects have their own coding guidelines. Here are some examples of such guidelines. These are reference purposes only; unless explicitly stated we also adopt some part of other guidelines, they are not part of the Kea's coding guidelines.