docs/CODING_STYLE.md

   1 # Coding style
   2
   3 - 8ch indent, no tabs, except for files in `man/` which are 2ch indent,
   4   and still no tabs.
   5
   6 - We prefer `/* comments */` over `// comments` in code you commit, please. This
   7   way `// comments` are left for developers to use for local, temporary
   8   commenting of code for debug purposes (i.e. uncommittable stuff), making such
   9   comments easily discernible from explanatory, documenting code comments
  10   (i.e. committable stuff).
  11
  12 - Don't break code lines too eagerly. We do **not** force line breaks at 80ch,
  13   all of today's screens should be much larger than that. But then again, don't
  14   overdo it, ~119ch should be enough really. The `.editorconfig`, `.vimrc` and
  15   `.dir-locals.el` files contained in the repository will set this limit up for
  16   you automatically, if you let them (as well as a few other things).
  17
  18 - Variables and functions **must** be static, unless they have a
  19   prototype, and are supposed to be exported.
  20
  21 - structs in `PascalCase` (with exceptions, such as public API structs),
  22   variables and functions in `snake_case`.
  23
  24 - The destructors always deregister the object from the next bigger
  25   object, not the other way around.
  26
  27 - To minimize strict aliasing violations, we prefer unions over casting.
  28
  29 - For robustness reasons, destructors should be able to destruct
  30   half-initialized objects, too.
  31
  32 - Error codes are returned as negative `Exxx`. e.g. `return -EINVAL`. There
  33   are some exceptions: for constructors, it is OK to return `NULL` on
  34   OOM. For lookup functions, `NULL` is fine too for "not found".
  35
  36   Be strict with this. When you write a function that can fail due to
  37   more than one cause, it *really* should have an `int` as the return value
  38   for the error code.
  39
  40 - Do not bother with error checking whether writing to stdout/stderr
  41   worked.
  42
  43 - Do not log errors from "library" code, only do so from "main
  44   program" code. (With one exception: it is OK to log with DEBUG level
  45   from any code, with the exception of maybe inner loops).
  46
  47 - Always check OOM. There is no excuse. In program code, you can use
  48   `log_oom()` for then printing a short message, but not in "library" code.
  49
  50 - Do not issue NSS requests (that includes user name and host name
  51   lookups) from PID 1 as this might trigger deadlocks when those
  52   lookups involve synchronously talking to services that we would need
  53   to start up.
  54
  55 - Do not synchronously talk to any other service from PID 1, due to
  56   risk of deadlocks.
  57
  58 - Avoid fixed-size string buffers, unless you really know the maximum
  59   size and that maximum size is small. They are a source of errors,
  60   since they possibly result in truncated strings. It is often nicer
  61   to use dynamic memory, `alloca()` or VLAs. If you do allocate fixed-size
  62   strings on the stack, then it is probably only OK if you either
  63   use a maximum size such as `LINE_MAX`, or count in detail the maximum
  64   size a string can have. (`DECIMAL_STR_MAX` and `DECIMAL_STR_WIDTH`
  65   macros are your friends for this!)
  66
  67   Or in other words, if you use `char buf[256]` then you are likely
  68   doing something wrong!
  69
  70 - Stay uniform. For example, always use `usec_t` for time
  71   values. Do not mix `usec` and `msec`, and `usec` and whatnot.
  72
  73 - Make use of `_cleanup_free_` and friends. It makes your code much
  74   nicer to read (and shorter)!
  75
  76 - Be exceptionally careful when formatting and parsing floating point
  77   numbers. Their syntax is locale dependent (i.e. `5.000` in en_US is
  78   generally understood as 5, while in de_DE as 5000.).
  79
  80 - Try to use this:
  81
  82   ```c
  83   void foo() {
  84   }
  85   ```
  86
  87   instead of this:
  88
  89   ```c
  90   void foo()
  91   {
  92   }
  93   ```
  94
  95   But it is OK if you do not.
  96
  97 - Single-line `if` blocks should not be enclosed in `{}`. Use this:
  98
  99   ```c
 100   if (foobar)
 101           waldo();
 102   ```
 103
 104   instead of this:
 105
 106   ```c
 107   if (foobar) {
 108           waldo();
 109   }
 110   ```
 111
 112 - Do not write `foo ()`, write `foo()`.
 113
 114 - Please use `streq()` and `strneq()` instead of `strcmp()`, `strncmp()` where
 115   applicable (i.e. wherever you just care about equality/inequality, not about
 116   the sorting order).
 117
 118 - Preferably allocate stack variables on the top of the block:
 119
 120   ```c
 121   {
 122           int a, b;
 123
 124           a = 5;
 125           b = a;
 126   }
 127   ```
 128
 129 - Unless you allocate an array, `double` is always the better choice
 130   than `float`. Processors speak `double` natively anyway, so this is
 131   no speed benefit, and on calls like `printf()` `float`s get promoted
 132   to `double`s anyway, so there is no point.
 133
 134 - Do not mix function invocations with variable definitions in one
 135   line. Wrong:
 136
 137   ```c
 138   {
 139           int a = foobar();
 140           uint64_t x = 7;
 141   }
 142   ```
 143
 144   Right:
 145
 146   ```c
 147   {
 148           int a;
 149           uint64_t x = 7;
 150
 151           a = foobar();
 152   }
 153   ```
 154
 155 - Use `goto` for cleaning up, and only use it for that. i.e. you may
 156   only jump to the end of a function, and little else. Never jump
 157   backwards!
 158
 159 - Think about the types you use. If a value cannot sensibly be
 160   negative, do not use `int`, but use `unsigned`.
 161
 162 - Use `char` only for actual characters. Use `uint8_t` or `int8_t`
 163   when you actually mean a byte-sized signed or unsigned
 164   integers. When referring to a generic byte, we generally prefer the
 165   unsigned variant `uint8_t`. Do not use types based on `short`. They
 166   *never* make sense. Use `int`, `long`, `long long`, all in
 167   unsigned and signed fashion, and the fixed-size types
 168   `uint8_t`, `uint16_t`, `uint32_t`, `uint64_t`, `int8_t`, `int16_t`, `int32_t` and so on,
 169   as well as `size_t`, but nothing else. Do not use kernel types like
 170   `u32` and so on, leave that to the kernel.
 171
 172 - Public API calls (i.e. functions exported by our shared libraries)
 173   must be marked `_public_` and need to be prefixed with `sd_`. No
 174   other functions should be prefixed like that.
 175
 176 - In public API calls, you **must** validate all your input arguments for
 177   programming error with `assert_return()` and return a sensible return
 178   code. In all other calls, it is recommended to check for programming
 179   errors with a more brutal `assert()`. We are more forgiving to public
 180   users than for ourselves! Note that `assert()` and `assert_return()`
 181   really only should be used for detecting programming errors, not for
 182   runtime errors. `assert()` and `assert_return()` by usage of `_likely_()`
 183   inform the compiler that he should not expect these checks to fail,
 184   and they inform fellow programmers about the expected validity and
 185   range of parameters.
 186
 187 - Never use `strtol()`, `atoi()` and similar calls. Use `safe_atoli()`,
 188   `safe_atou32()` and suchlike instead. They are much nicer to use in
 189   most cases and correctly check for parsing errors.
 190
 191 - For every function you add, think about whether it is a "logging"
 192   function or a "non-logging" function. "Logging" functions do logging
 193   on their own, "non-logging" function never log on their own and
 194   expect their callers to log. All functions in "library" code,
 195   i.e. in `src/shared/` and suchlike must be "non-logging". Every time a
 196   "logging" function calls a "non-logging" function, it should log
 197   about the resulting errors. If a "logging" function calls another
 198   "logging" function, then it should not generate log messages, so
 199   that log messages are not generated twice for the same errors.
 200
 201 - Avoid static variables, except for caches and very few other
 202   cases. Think about thread-safety! While most of our code is never
 203   used in threaded environments, at least the library code should make
 204   sure it works correctly in them. Instead of doing a lot of locking
 205   for that, we tend to prefer using TLS to do per-thread caching (which
 206   only works for small, fixed-size cache objects), or we disable
 207   caching for any thread that is not the main thread. Use
 208   `is_main_thread()` to detect whether the calling thread is the main
 209   thread.
 210
 211 - Command line option parsing:
 212   - Do not print full `help()` on error, be specific about the error.
 213   - Do not print messages to stdout on error.
 214   - Do not POSIX_ME_HARDER unless necessary, i.e. avoid `+` in option string.
 215
 216 - Do not write functions that clobber call-by-reference variables on
 217   failure. Use temporary variables for these cases and change the
 218   passed in variables only on success.
 219
 220 - When you allocate a file descriptor, it should be made `O_CLOEXEC`
 221   right from the beginning, as none of our files should leak to forked
 222   binaries by default. Hence, whenever you open a file, `O_CLOEXEC` must
 223   be specified, right from the beginning. This also applies to
 224   sockets. Effectively, this means that all invocations to:
 225
 226   - `open()` must get `O_CLOEXEC` passed,
 227   - `socket()` and `socketpair()` must get `SOCK_CLOEXEC` passed,
 228   - `recvmsg()` must get `MSG_CMSG_CLOEXEC` set,
 229   - `F_DUPFD_CLOEXEC` should be used instead of `F_DUPFD`, and so on,
 230   - invocations of `fopen()` should take `e`.
 231
 232 - We never use the POSIX version of `basename()` (which glibc defines it in
 233   `libgen.h`), only the GNU version (which glibc defines in `string.h`).
 234   The only reason to include `libgen.h` is because `dirname()`
 235   is needed. Every time you need that please immediately undefine
 236   `basename()`, and add a comment about it, so that no code ever ends up
 237   using the POSIX version!
 238
 239 - Use the bool type for booleans, not integers. One exception: in public
 240   headers (i.e those in `src/systemd/sd-*.h`) use integers after all, as `bool`
 241   is C99 and in our public APIs we try to stick to C89 (with a few extension).
 242
 243 - When you invoke certain calls like `unlink()`, or `mkdir_p()` and you
 244   know it is safe to ignore the error it might return (because a later
 245   call would detect the failure anyway, or because the error is in an
 246   error path and you thus couldn't do anything about it anyway), then
 247   make this clear by casting the invocation explicitly to `(void)`. Code
 248   checks like Coverity understand that, and will not complain about
 249   ignored error codes. Hence, please use this:
 250
 251   ```c
 252   (void) unlink("/foo/bar/baz");
 253   ```
 254
 255   instead of just this:
 256
 257   ```c
 258   unlink("/foo/bar/baz");
 259   ```
 260
 261   Don't cast function calls to `(void)` that return no error
 262   conditions. Specifically, the various `xyz_unref()` calls that return a `NULL`
 263   object shouldn't be cast to `(void)`, since not using the return value does not
 264   hide any errors.
 265
 266 - Don't invoke `exit()`, ever. It is not replacement for proper error
 267   handling. Please escalate errors up your call chain, and use normal
 268   `return` to exit from the main function of a process. If you
 269   `fork()`ed off a child process, please use `_exit()` instead of `exit()`,
 270   so that the exit handlers are not run.
 271
 272 - Please never use `dup()`. Use `fcntl(fd, F_DUPFD_CLOEXEC, 3)`
 273   instead. For two reason: first, you want `O_CLOEXEC` set on the new `fd`
 274   (see above). Second, `dup()` will happily duplicate your `fd` as 0, 1,
 275   2, i.e. stdin, stdout, stderr, should those `fd`s be closed. Given the
 276   special semantics of those `fd`s, it's probably a good idea to avoid
 277   them. `F_DUPFD_CLOEXEC` with `3` as parameter avoids them.
 278
 279 - When you define a destructor or `unref()` call for an object, please
 280   accept a `NULL` object and simply treat this as NOP. This is similar
 281   to how libc `free()` works, which accepts `NULL` pointers and becomes a
 282   NOP for them. By following this scheme a lot of `if` checks can be
 283   removed before invoking your destructor, which makes the code
 284   substantially more readable and robust.
 285
 286 - Related to this: when you define a destructor or `unref()` call for an
 287   object, please make it return the same type it takes and always
 288   return `NULL` from it. This allows writing code like this:
 289
 290   ```c
 291   p = foobar_unref(p);
 292   ```
 293
 294   which will always work regardless if `p` is initialized or not, and
 295   guarantees that `p` is `NULL` afterwards, all in just one line.
 296
 297 - Use `alloca()`, but never forget that it is not OK to invoke `alloca()`
 298   within a loop or within function call parameters. `alloca()` memory is
 299   released at the end of a function, and not at the end of a `{}`
 300   block. Thus, if you invoke it in a loop, you keep increasing the
 301   stack pointer without ever releasing memory again. (VLAs have better
 302   behavior in this case, so consider using them as an alternative.)
 303   Regarding not using `alloca()` within function parameters, see the
 304   BUGS section of the `alloca(3)` man page.
 305
 306 - Use `memzero()` or even better `zero()` instead of `memset(..., 0, ...)`
 307
 308 - Instead of using `memzero()`/`memset()` to initialize structs allocated
 309   on the stack, please try to use c99 structure initializers. It's
 310   short, prettier and actually even faster at execution. Hence:
 311
 312   ```c
 313   struct foobar t = {
 314           .foo = 7,
 315           .bar = "bazz",
 316   };
 317   ```
 318
 319   instead of:
 320
 321   ```c
 322   struct foobar t;
 323   zero(t);
 324   t.foo = 7;
 325   t.bar = "bazz";
 326   ```
 327
 328 - When returning a return code from `main()`, please preferably use
 329   `EXIT_FAILURE` and `EXIT_SUCCESS` as defined by libc.
 330
 331 - The order in which header files are included doesn't matter too
 332   much. systemd-internal headers must not rely on an include order, so
 333   it is safe to include them in any order possible.
 334   However, to not clutter global includes, and to make sure internal
 335   definitions will not affect global headers, please always include the
 336   headers of external components first (these are all headers enclosed
 337   in <>), followed by our own exported headers (usually everything
 338   that's prefixed by `sd-`), and then followed by internal headers.
 339   Furthermore, in all three groups, order all includes alphabetically
 340   so duplicate includes can easily be detected.
 341
 342 - To implement an endless loop, use `for (;;)` rather than `while (1)`.
 343   The latter is a bit ugly anyway, since you probably really
 344   meant `while (true)`. To avoid the discussion what the right
 345   always-true expression for an infinite while loop is, our
 346   recommendation is to simply write it without any such expression by
 347   using `for (;;)`.
 348
 349 - Never use the `off_t` type, and particularly avoid it in public
 350   APIs. It's really weirdly defined, as it usually is 64-bit and we
 351   don't support it any other way, but it could in theory also be
 352   32-bit. Which one it is depends on a compiler switch chosen by the
 353   compiled program, which hence corrupts APIs using it unless they can
 354   also follow the program's choice. Moreover, in systemd we should
 355   parse values the same way on all architectures and cannot expose
 356   `off_t` values over D-Bus. To avoid any confusion regarding conversion
 357   and ABIs, always use simply `uint64_t` directly.
 358
 359 - Commit message subject lines should be prefixed with an appropriate
 360   component name of some kind. For example "journal: ", "nspawn: " and
 361   so on.
 362
 363 - Do not use "Signed-Off-By:" in your commit messages. That's a kernel
 364   thing we don't do in the systemd project.
 365
 366 - Avoid leaving long-running child processes around, i.e. `fork()`s that
 367   are not followed quickly by an `execv()` in the child. Resource
 368   management is unclear in this case, and memory CoW will result in
 369   unexpected penalties in the parent much, much later on.
 370
 371 - Don't block execution for arbitrary amounts of time using `usleep()`
 372   or a similar call, unless you really know what you do. Just "giving
 373   something some time", or so is a lazy excuse. Always wait for the
 374   proper event, instead of doing time-based poll loops.
 375
 376 - To determine the length of a constant string `"foo"`, don't bother
 377   with `sizeof("foo")-1`, please use `STRLEN()` instead.
 378
 379 - If you want to concatenate two or more strings, consider using
 380   `strjoin()` rather than `asprintf()`, as the latter is a lot
 381   slower. This matters particularly in inner loops.
 382
 383 - Please avoid using global variables as much as you can. And if you
 384   do use them make sure they are static at least, instead of
 385   exported. Especially in library-like code it is important to avoid
 386   global variables. Why are global variables bad? They usually hinder
 387   generic reusability of code (since they break in threaded programs,
 388   and usually would require locking there), and as the code using them
 389   has side-effects make programs non-transparent. That said, there are
 390   many cases where they explicitly make a lot of sense, and are OK to
 391   use. For example, the log level and target in `log.c` is stored in a
 392   global variable, and that's OK and probably expected by most. Also
 393   in many cases we cache data in global variables. If you add more
 394   caches like this, please be careful however, and think about
 395   threading. Only use static variables if you are sure that
 396   thread-safety doesn't matter in your case. Alternatively, consider
 397   using TLS, which is pretty easy to use with gcc's `thread_local`
 398   concept. It's also OK to store data that is inherently global in
 399   global variables, for example data parsed from command lines, see
 400   below.
 401
 402 - If you parse a command line, and want to store the parsed parameters
 403   in global variables, please consider prefixing their names with
 404   `arg_`. We have been following this naming rule in most of our
 405   tools, and we should continue to do so, as it makes it easy to
 406   identify command line parameter variables, and makes it clear why it
 407   is OK that they are global variables.
 408
 409 - When exposing public C APIs, be careful what function parameters you make
 410   `const`. For example, a parameter taking a context object should probably not
 411   be `const`, even if you are writing an otherwise read-only accessor function
 412   for it. The reason is that making it `const` fixates the contract that your
 413   call won't alter the object ever, as part of the API. However, that's often
 414   quite a promise, given that this even prohibits object-internal caching or
 415   lazy initialization of object variables. Moreover, it's usually not too useful
 416   for client applications. Hence, please be careful and avoid `const` on object
 417   parameters, unless you are very sure `const` is appropriate.
 418
 419 - Make sure to enforce limits on every user controllable resource. If the user
 420   can allocate resources in your code, your code must enforce some form of
 421   limits after which it will refuse operation. It's fine if it is hard-coded (at
 422   least initially), but it needs to be there. This is particularly important
 423   for objects that unprivileged users may allocate, but also matters for
 424   everything else any user may allocated.
 425
 426 - `htonl()`/`ntohl()` and `htons()`/`ntohs()` are weird. Please use `htobe32()` and
 427   `htobe16()` instead, it's much more descriptive, and actually says what really
 428   is happening, after all `htonl()` and `htons()` don't operate on `long`s and
 429   `short`s as their name would suggest, but on `uint32_t` and `uint16_t`. Also,
 430   "network byte order" is just a weird name for "big endian", hence we might
 431   want to call it "big endian" right-away.
 432
 433 - You might wonder what kind of common code belongs in `src/shared/` and what
 434   belongs in `src/basic/`. The split is like this: anything that uses public APIs
 435   we expose (i.e. any of the sd-bus, sd-login, sd-id128, ... APIs) must be
 436   located in `src/shared/`. All stuff that only uses external libraries from
 437   other projects (such as glibc's APIs), or APIs from `src/basic/` itself should
 438   be placed in `src/basic/`. Conversely, `src/libsystemd/` may only use symbols
 439   from `src/basic`, but not from `src/shared/`.
 440
 441   To summarize:
 442
 443   `src/basic/`
 444   - may be used by all code in the tree
 445   - may not use any code outside of `src/basic/`
 446
 447   `src/libsystemd/`
 448   - may be used by all code in the tree, except for code in `src/basic/`
 449   - may not use any code outside of `src/basic/`, `src/libsystemd/`
 450
 451   `src/shared/`
 452   - may be used by all code in the tree, except for code in `src/basic/`, `src/libsystemd/`
 453   - may not use any code outside of `src/basic/`, `src/libsystemd/`, `src/shared/`
 454
 455 - Our focus is on the GNU libc (glibc), not any other libcs. If other libcs are
 456   incompatible with glibc it's on them. However, if there are equivalent POSIX
 457   and Linux/GNU-specific APIs, we generally prefer the POSIX APIs. If there
 458   aren't, we are happy to use GNU or Linux APIs, and expect non-GNU
 459   implementations of libc to catch up with glibc.
 460
 461 - Whenever installing a signal handler, make sure to set `SA_RESTART` for it, so
 462   that interrupted system calls are automatically restarted, and we minimize
 463   hassles with handling `EINTR` (in particular as `EINTR` handling is pretty broken
 464   on Linux).
 465
 466 - When applying C-style unescaping as well as specifier expansion on the same
 467   string, always apply the C-style unescaping fist, followed by the specifier
 468   expansion. When doing the reverse, make sure to escape `%` in specifier-style
 469   first (i.e. `%` → `%%`), and then do C-style escaping where necessary.
 470
 471 - It's a good idea to use `O_NONBLOCK` when opening 'foreign' regular files, i.e.
 472   file system objects that are supposed to be regular files whose paths where
 473   specified by the user and hence might actually refer to other types of file
 474   system objects. This is a good idea so that we don't end up blocking on
 475   'strange' file nodes, for example if the user pointed us to a FIFO or device
 476   node which may block when opening. Moreover even for actual regular files
 477   `O_NONBLOCK` has a benefit: it bypasses any mandatory lock that might be in
 478   effect on the regular file. If in doubt consider turning off `O_NONBLOCK` again
 479   after opening.
 480
 481 - When referring to a configuration file option in the documentation and such,
 482   please always suffix it with `=`, to indicate that it is a configuration file
 483   setting.
 484
 485 - When referring to a command line option in the documentation and such, please
 486   always prefix with `--` or `-` (as appropriate), to indicate that it is a
 487   command line option.
 488
 489 - When referring to a file system path that is a directory, please always
 490   suffix it with `/`, to indicate that it is a directory, not a regular file
 491   (or other file system object).
 492
 493 - Don't use `fgets()`, it's too hard to properly handle errors such as overly
 494   long lines. Use `read_line()` instead, which is our own function that handles
 495   this much nicer.