compatibility. Since we typically want to allow adding new enum values to an
existing enum type with later API versions, please use the
`_SD_ENUM_FORCE_S64()` macro in the enum definition, which forces the size of
- the enum to be signed 64bit wide.
+ the enum to be signed 64-bit wide.
+
+- Empty lines to separate code blocks are a good thing, please add them
+ abundantly. However, please stick to one at a time, i.e. multiple empty lines
+ immediately following each other are not OK. Also, we try to keep function
+ calls and their immediate error handling together. Hence:
+
+ ```c
+ /* → empty line here is good */
+ r = some_function(…);
+ /* → empty line here would be bad */
+ if (r < 0)
+ return log_error_errno(r, "Some function failed: %m");
+ /* → empty line here is good */
+ ```
+
+- In shell scripts, do not use whitespace after the redirection operator
+ (`>some/file` instead of `> some/file`, `<<EOF` instead of `<< EOF`).
## Code Organization and Semantics
variables if you are sure that thread-safety doesn't matter in your
case. Alternatively, consider using TLS, which is pretty easy to use with
gcc's `thread_local` concept. It's also OK to store data that is inherently
- global in global variables, for example data parsed from command lines, see
+ global in global variables, for example, data parsed from command lines, see
below.
- Our focus is on the GNU libc (glibc), not any other libcs. If other libcs are
the point where they can be initialized. Avoid huge variable declaration
lists at the top of the function.
- As an exception, `r` is typically used for a local state variable, but should
- almost always be declared as the last variable at the top of the function.
+ As an exception, `int r` is typically used for a local state variable, but
+ should almost always be declared as the last variable at the top of the
+ function.
```c
{
t.bar = "bazz";
```
-- To implement an endless loop, use `for (;;)` rather than `while (1)`. The
+- To implement an endless loop, use `for (;;)` rather than `while (1)`. The
latter is a bit ugly anyway, since you probably really meant `while
(true)`. To avoid the discussion what the right always-true expression for an
infinite while loop is, our recommendation is to simply write it without any
## Types
- Think about the types you use. If a value cannot sensibly be negative, do not
- use `int`, but use `unsigned`.
+ use `int`, but use `unsigned`. We prefer `unsigned` form to `unsigned int`.
- Use `char` only for actual characters. Use `uint8_t` or `int8_t` when you
actually mean a byte-sized signed or unsigned integers. When referring to a
i.e. file system objects that are supposed to be regular files whose paths
were specified by the user and hence might actually refer to other types of
file system objects. This is a good idea so that we don't end up blocking on
- 'strange' file nodes, for example if the user pointed us to a FIFO or device
+ 'strange' file nodes, for example, if the user pointed us to a FIFO or device
node which may block when opening. Moreover even for actual regular files
`O_NONBLOCK` has a benefit: it bypasses any mandatory lock that might be in
effect on the regular file. If in doubt consider turning off `O_NONBLOCK`
again after opening.
+- These days we generally prefer `openat()`-style file APIs, i.e. APIs that
+ accept a combination of file descriptor and path string, and where the path
+ (if not absolute) is considered relative to the specified file
+ descriptor. When implementing library calls in similar style, please make
+ sure to imply `AT_EMPTY_PATH` if an empty or `NULL` path argument is
+ specified (and convert that latter to an empty string). This differs from the
+ underlying kernel semantics, where `AT_EMPTY_PATH` must always be specified
+ explicitly, and `NULL` is not accepted as path.
+
## Command Line
- If you parse a command line, and want to store the parsed parameters in
- Do not use `basename()` or `dirname()`. The semantics in corner cases are
full of pitfalls, and the fact that there are two quite different versions of
`basename()` (one POSIX and one GNU, of which the latter is much more useful)
- doesn't make it bette either. Use path_extract_filename() and
+ doesn't make it better either. Use path_extract_filename() and
path_extract_directory() instead.
- Never use `FILENAME_MAX`. Use `PATH_MAX` instead (for checking maximum size
## Committing to git
- Commit message subject lines should be prefixed with an appropriate component
- name of some kind. For example "journal: ", "nspawn: " and so on.
+ name of some kind. For example, "journal: ", "nspawn: " and so on.
- Do not use "Signed-Off-By:" in your commit messages. That's a kernel thing we
don't do in the systemd project.
hard. Moreover, while we rely heavily on GitHub's project management
infrastructure we'd like to keep everything that can reasonably be kept in
the git repository itself in the git repository, so that we can theoretically
- move things elswhere with the least effort possible.
+ move things elsewhere with the least effort possible.
- It's OK to reference GitHub PRs, GitHub issues and git commits from code
comments. Cross-referencing code, issues, and documentation is a good thing.
- Reasonable use of non-ASCII Unicode UTF-8 characters in code comments is
welcome. If your code comment contains an emoji or two this will certainly
brighten the day of the occasional reviewer of your code. Really! 😊
+
+## Threading
+
+- We generally avoid using threads, to the level this is possible. In
+ particular in the service manager/PID 1 threads are not OK to use. This is
+ because you cannot mix memory allocation in threads with use of glibc's
+ `clone()` call, or manual `clone()`/`clone3()` system call wrappers. Only
+ glibc's own `fork()` call will properly synchronize the memory allocation
+ locks around the process clone operation. This means that if a process is
+ cloned via `clone()`/`clone3()` and another thread currently has the
+ `malloc()` lock taken, it will be cloned in locked state to the child, and
+ thus can never be acquired in the child, leading to deadlocks. Hence, when
+ using `clone()`/`clone3()` there are only two ways out: never use threads in the
+ parent, or never do memory allocation in the child. For our uses we need
+ `clone()`/`clone3()` and hence decided to avoid threads. Of course, sometimes the
+ concurrency threads allow is beneficial, however we suggest forking off
+ worker *processes* rather than worker *threads* for this purpose, ideally
+ even with an `execve()` to remove the CoW trap situation `fork()` easily
+ triggers.
+
+- A corollary of the above is: never use `clone()` where a `fork()` would do
+ too. Also consider using `posix_spawn()` which combines `clone()` +
+ `execve()` into one and has nice properties since it avoids becoming a CoW
+ trap by using `CLONE_VFORK` and `CLONE_VM` together.
+
+- While we avoid forking off threads on our own, writing thread-safe code is a
+ good idea where it might end up running inside of libsystemd.so or
+ similar. Hence, use TLS (i.e. `thread_local`) where appropriate, and maybe
+ the occasional `pthread_once()`.
+
+## Tests
+
+- Use the assertion macros from `tests.h` (`ASSERT_GE()`, `ASSERT_OK()`, ...) to
+ make sure a descriptive error is logged when an assertion fails. If no assertion
+ macro exists for your specific use case, please add a new assertion macro in a
+ separate commit.
+
+- When modifying existing tests, please convert the test to use the new assertion
+ macros from `tests.h` if it is not already using those.
projects / thirdparty / systemd.git / blobdiff