... | ... | @@ -6,7 +6,7 @@ This page documents some coding guidelines for the Kea project. See [Stork codin |
|
|
|
|
|
Those apply to all code in all languages.
|
|
|
|
|
|
## Line length
|
|
|
## Line Length
|
|
|
|
|
|
Source code not exceeding 80 columns is preferred. This is derived from
|
|
|
the [wiki:BIND9CodingGuidelines BIND 9 coding guidelines], mainly for
|
... | ... | @@ -30,7 +30,7 @@ if (JS_DefineProperty(cx, o, "data", |
|
|
0, 0, JSPROP_ENUMERATE) != 0) {
|
|
|
```
|
|
|
|
|
|
## ChangeLog entries
|
|
|
## 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
|
... | ... | @@ -38,19 +38,19 @@ as a patch, the original author should be asked how he/she would like to be cred |
|
|
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
|
|
|
## Testing and Documenting 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](https://datatracker.ietf.org/doc/html/rfc5737) and [RFC3849](https://datatracker.ietf.org/doc/html/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
|
|
|
|
|
|
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
|
|
|
## 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:
|
|
|
Kea is written in C++14. 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
|
... | ... | @@ -76,7 +76,7 @@ We use python 3. Follow [PEP8](https://www.python.org/dev/peps/pep-0008/) if pos |
|
|
|
|
|
# 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.
|
|
|
We use C++14 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
|
|
|
|
... | ... | @@ -109,12 +109,14 @@ 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:
|
|
|
|
|
|
```cpp
|
|
|
#include <util/threads/thread.h>
|
|
|
#include <dns/name.h>
|
|
|
```
|
|
|
|
|
|
Incorrect:
|
|
|
|
|
|
```cpp
|
|
|
#include <thread.h>
|
|
|
#include "name.h"
|
... | ... | @@ -123,6 +125,7 @@ Incorrect: |
|
|
## Curly Braces
|
|
|
|
|
|
Always add braces even for a single-line block:
|
|
|
|
|
|
```cpp
|
|
|
if (something_holds) {
|
|
|
perform something;
|
... | ... | @@ -156,6 +159,7 @@ This was derived from the BIND 9 coding guideline. It's known this style may lo |
|
|
### Curly Braces for Catch
|
|
|
|
|
|
A catch statement should have braces on a single line, like this:
|
|
|
|
|
|
```cpp
|
|
|
.
|
|
|
.
|
... | ... | @@ -207,7 +211,7 @@ 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 (++/--)
|
|
|
### Increment and Decrement Operators (++/--)
|
|
|
|
|
|
Use the prefix form of increment/decrement operators by default:
|
|
|
|
... | ... | @@ -248,6 +252,7 @@ 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:
|
|
|
|
|
|
```cpp
|
|
|
class Foo {
|
|
|
public:
|
... | ... | @@ -277,6 +282,7 @@ Don't start things with underscores. According to Stroustrup's C++ book: |
|
|
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
|
|
|
|
|
|
```cpp
|
|
|
enum EnumName {
|
|
|
FOO,
|
... | ... | @@ -285,6 +291,7 @@ enum EnumName { |
|
|
```
|
|
|
|
|
|
Note that unless you have a specific reason to set specific values, leave specific values off. These can be written if needed:
|
|
|
|
|
|
```cpp
|
|
|
enum ExamplePortNumbers {
|
|
|
DNS = 53,
|
... | ... | @@ -303,6 +310,7 @@ 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.
|
|
|
|
... | ... | @@ -358,6 +366,7 @@ foo(Bar baz) { |
|
|
|
|
|
Make sure inserting a blank line between two function/method
|
|
|
declarations or definitions:
|
|
|
|
|
|
```cpp
|
|
|
class Bad {
|
|
|
/// @brief Short description for bad1
|
... | ... | @@ -376,6 +385,7 @@ class Good { |
|
|
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.
|
... | ... | @@ -450,7 +460,7 @@ 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
|
|
|
## 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.
|
|
|
|
... | ... | @@ -463,7 +473,16 @@ function(const boost::shared_ptr<DataType>& param) { |
|
|
}
|
|
|
```
|
|
|
|
|
|
## Exception-safe getters and string-production methods
|
|
|
## Working with boost::asio::io_context
|
|
|
|
|
|
Use the ``isc::asiolink::IOService`` wrapper wherever possible.
|
|
|
|
|
|
Before cleaning up objects that own an ``IOService``:
|
|
|
1. Call `IOService::stop()` to finish all existing invocations to `run()`, `run_one()`, `poll()` or `poll_one()`.
|
|
|
2. Call `IOService::restart()` as a way to make the next step viable.
|
|
|
3. Call `IOService::poll()` to finish any remaining handlers.
|
|
|
|
|
|
## 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.
|
|
|
|
... | ... | @@ -496,10 +515,11 @@ Technically, the latter `const` is redundant. But !SunStudio C++ compilers have |
|
|
|
|
|
Unfortunately, we want to be as portable as possible, and so need to work around this by being redundant.
|
|
|
|
|
|
## NULL pointer check
|
|
|
## NULL Pointer Check
|
|
|
|
|
|
Use the "implicit" form when testing to see whether a pointer is 0
|
|
|
(`NULL`):
|
|
|
|
|
|
```cpp
|
|
|
SomeType* ptr;
|
|
|
if (ptr) { // instead of 'ptr != NULL'
|
... | ... | @@ -516,6 +536,7 @@ This is different from what is suggested in the [wiki:BIND9CodingGuidelines BIND |
|
|
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:
|
|
|
|
|
|
```cpp
|
|
|
boost::shared_ptr<SomeType> ptr;
|
|
|
if (ptr.get() != NULL) { // ugly
|
... | ... | @@ -584,7 +605,7 @@ Note that this is not always possible, especially when checking an |
|
|
object that has no operator<<() (or more obviously no operator<op>()),
|
|
|
which is used to output on failure.
|
|
|
|
|
|
## Test naming
|
|
|
## Test Naming
|
|
|
|
|
|
Names of tests should be similar to classes and method names for that language. For example:
|
|
|
|
... | ... | @@ -593,7 +614,7 @@ Names of tests should be similar to classes and method names for that language. |
|
|
|
|
|
# Makefile.am Guidelines
|
|
|
|
|
|
## Make portability
|
|
|
## 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.
|
|
|
|
... | ... | @@ -601,6 +622,7 @@ Makefiles must never depend on a particular make, for instance the `$(<function> |
|
|
|
|
|
If there is are one or more subdirectories, the first line in the file
|
|
|
should be the SUBDIRS entry, e.g.:
|
|
|
|
|
|
```
|
|
|
SUBDIRS = . tests
|
|
|
```
|
... | ... | @@ -608,10 +630,11 @@ 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
|
|
|
## Preprocessor Flags and 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)
|
... | ... | @@ -624,6 +647,7 @@ 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
|
|
|
```
|
... | ... | @@ -633,16 +657,17 @@ AM_CXXFLAGS should be initialized to KEA_CXXFLAGS, e.g.: |
|
|
AM_CXXFLAGS = $(KEA_CXXFLAGS)
|
|
|
```
|
|
|
|
|
|
## Linker flags
|
|
|
## 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
|
|
|
## 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
|
... | ... | @@ -661,7 +686,7 @@ by libprocess and does not include its static definition. |
|
|
|
|
|
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](https://gitlab.isc.org/isc-projects/stork/-/wikis/Processes/coding-guidelines) for UI guidelines for Stork.
|
|
|
|
|
|
# IP address and port formatting
|
|
|
# IP Address and Port Formatting
|
|
|
|
|
|
Whenever an IP address and port is output, IETF [RFC2396](https://datatracker.ietf.org/doc/html/rfc2396) and [RFC2732](https://datatracker.ietf.org/doc/html/rfc2732) should be followed.
|
|
|
|
... | ... | @@ -684,14 +709,14 @@ Importing code is not a decision that can be taken lightly. Please discuss this |
|
|
- 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
|
|
|
# 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
|
|
|
# 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.
|
|
|
|
... | ... | |