docs/CODING_STYLE.md

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