mirror of
https://gitlab.freedesktop.org/NetworkManager/NetworkManager
synced 2024-10-15 12:34:55 +00:00
159 lines
6.6 KiB
Plaintext
159 lines
6.6 KiB
Plaintext
Guidelines for Contributing
|
|
===========================
|
|
|
|
|
|
Coding Standard
|
|
---------------
|
|
|
|
The formatting uses clang-format with clang 11.0. Run
|
|
`./contrib/scripts/nm-code-format.sh -i` to reformat the code
|
|
or call `clang-format` yourself.
|
|
You may also call `./contrib/scripts/nm-code-format-container.sh`
|
|
which runs a Fedora 33 container using podman.
|
|
You are welcome to not bother and open a merge request with
|
|
wrong formatting, but note that we then will automatically adjust
|
|
your contribution before merging.
|
|
|
|
Since our coding style is entirely automated, the following are just
|
|
some details of the style we use:
|
|
|
|
* Indent with 4 spaces. (_no_ tabs).
|
|
|
|
* Have no space between the function name and the opening '('.
|
|
- GOOD: `g_strdup(x)`
|
|
- BAD: `g_strdup (x)`
|
|
|
|
* C-style comments
|
|
- GOOD: `f(x); /* comment */`
|
|
- BAD: `f(x); // comment`
|
|
|
|
* Keep assignments in the variable declaration area pretty short.
|
|
- GOOD: `MyObject *object;`
|
|
- BAD: `MyObject *object = complex_and_long_init_function(arg1, arg2, arg3);`
|
|
|
|
* 80-cols is a guideline, don't make the code uncomfortable in order to fit in
|
|
less than 80 cols.
|
|
|
|
* Constants are CAPS_WITH_UNDERSCORES and use the preprocessor.
|
|
- GOOD: `#define MY_CONSTANT 42`
|
|
- BAD: `static const unsigned myConstant = 42;`
|
|
|
|
|
|
Legal
|
|
-----
|
|
|
|
NetworkManager is partly licensed under terms of GNU Lesser General Public License
|
|
version 2 or later (LGPL-2.1+). That is for example the case for libnm.
|
|
For historical reasons, the daemon itself is licensed under terms of GNU General
|
|
Public License, version 2 or later (GPL-2.0+). See the license comment in the source
|
|
files.
|
|
Note that all new contributions to NetworkManager MUST be made under terms of
|
|
LGPL-2.1+, that is also the case for parts that are currently licensed GPL-2.0+.
|
|
The reason for that is that we might eventually relicense everything as LGPL and
|
|
new contributions already must agree with that future change.
|
|
For more details see [RELICENSE.md](RELICENSE.md).
|
|
|
|
|
|
Assertions in NetworkManager code
|
|
---------------------------------
|
|
|
|
There are different kind of assertions. Use the one that is appropriate.
|
|
|
|
1) g_return_*() from glib. This is usually enabled in release builds and
|
|
can be disabled with G_DISABLE_CHECKS define. This uses g_log() with
|
|
G_LOG_LEVEL_CRITICAL level (which allows the program to continue,
|
|
unless G_DEBUG=fatal-criticals or G_DEBUG=fatal-warnings is set). As such,
|
|
this is usually the preferred way for assertions that are supposed to be
|
|
enabled by default.
|
|
|
|
Optimally, after a g_return_*() failure the program can still continue. This is
|
|
also the reason why g_return_*() is preferable over g_assert().
|
|
For example, that is often not given for functions that return a GError, because
|
|
g_return_*() will return failure without setting the error output. That often leads
|
|
to a crash immidiately after, because the caller requires the GError to be set.
|
|
Make a reasonable effort so that an assertion failure may allow the process
|
|
to proceed. But don't put too much effort in it. After all, it's an assertion
|
|
failure that is not supposed to happen either way.
|
|
|
|
2) nm_assert() from NetworkManager. This is disabled by default in release
|
|
builds, but enabled if you build --with-more-assertions. See "WITH_MORE_ASSERTS"
|
|
define. This is preferred for assertions that are expensive to check or
|
|
nor necessary to check frequently. It's also for conditions that can easily
|
|
verified to be true and where future refactoring is unlikley to break that
|
|
condition.
|
|
Use this deliberately and assume it is removed from production builds.
|
|
|
|
3) g_assert() from glib. This is used in unit tests and commonly enabled
|
|
in release builds. It can be disabled with G_DISABLE_ASSERT assert
|
|
define. Since this results in a hard crash on assertion failure, you
|
|
should almost always prefer g_return_*() over this (except in unit tests).
|
|
|
|
4) assert() from <assert.h>. It is usually enabled in release builds and
|
|
can be disabled with NDEBUG define. Don't use it in NetworkManager,
|
|
it's basically like g_assert().
|
|
|
|
5) g_log() from glib. These are always compiled in, depending on the logging level
|
|
these are assertions too. G_LOG_LEVEL_ERROR aborts the program, G_LOG_LEVEL_CRITICAL
|
|
logs a critical warning (like g_return_*(), see G_DEBUG=fatal-criticals)
|
|
and G_LOG_LEVEL_WARNING logs a warning (see G_DEBUG=fatal-warnings).
|
|
G_LOG_LEVEL_DEBUG level is usually not printed, unless G_MESSAGES_DEBUG environment
|
|
is set.
|
|
In general, avoid using g_log() in NetworkManager. We have nm-logging instead
|
|
which logs to syslog/systemd-journald.
|
|
From a library like libnm it might make sense to log warnings (if someting
|
|
is really wrong) or debug messages. But better don't. If it's important,
|
|
find a way to report the notification via the API to the caller. If it's
|
|
not important, keep silent.
|
|
In particular, don't use levels G_LOG_LEVEL_CRITICAL and G_LOG_LEVEL_WARNING because
|
|
these are effectively assertions and we want to run with G_DEBUG=fatal-warnings.
|
|
|
|
6) g_warn_if_*() from glib. These are always compiled in and log a G_LOG_LEVEL_WARNING
|
|
warning. Don't use this.
|
|
|
|
7) G_TYPE_CHECK_INSTANCE_CAST() from glib. Unless building with "WITH_MORE_ASSERTS",
|
|
we set G_DISABLE_CAST_CHECKS. This means, cast macros like NM_DEVICE(ptr)
|
|
translate to plain C pointer casts. Use such cast macros deliberately, in production
|
|
code they are cheap, with more asserts enabled the check that the pointer type is
|
|
suitable.
|
|
|
|
Of course, every assertion failure is a bug, and calling it must have no side effects.
|
|
|
|
Theoretically, you are welcome to disable G_DISABLE_CHECKS and G_DISABLE_ASSERT
|
|
in production builds. In practice, nobody tests such a configuration, so beware.
|
|
|
|
For testing, you also want to run NetworkManager with environment variable
|
|
G_DEBUG=fatal-warnings to crash upon G_LOG_LEVEL_CRITICAL and G_LOG_LEVEL_WARNING
|
|
g_log() message. NetworkManager won't use these levels for regular logging
|
|
but for assertions.
|
|
|
|
|
|
Git Notes (refs/notes/bugs)
|
|
---------------------------
|
|
|
|
There are special notes to annotate git commit messages with information
|
|
about "Fixes" and "cherry picked from". Annotating the history is useful
|
|
if it was not done initially because our scripts can make use of it.
|
|
|
|
The notes it are called "refs/notes/bugs".
|
|
|
|
So configure:
|
|
|
|
```
|
|
$ git config --add 'remote.origin.fetch' 'refs/notes/bugs:refs/notes/bugs'
|
|
$ git config --add 'notes.displayref' 'refs/notes/bugs'
|
|
```
|
|
|
|
For example, set notes with
|
|
|
|
```
|
|
$ git notes --ref refs/notes/bugs add -m "(cherry picked from $COMMIT_SHA)" HEAD
|
|
```
|
|
|
|
You should see the notes in git-log output as well.
|
|
|
|
To resync our local notes use:
|
|
|
|
```
|
|
$ git fetch origin refs/notes/bugs:refs/notes/bugs -f
|
|
```
|