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 - Do not issue NSS requests (that includes user name and host name
 101   lookups) from PID 1 as this might trigger deadlocks when those
 102   lookups involve synchronously talking to services that we would need
 103   to start up.
 104
 105 - Do not synchronously talk to any other service from PID 1, due to
 106   risk of deadlocks.
 107
 108 - Be exceptionally careful when formatting and parsing floating point
 109   numbers. Their syntax is locale dependent (i.e. `5.000` in en_US is
 110   generally understood as 5, while in de_DE as 5000.).
 111
 112 - Do not mix function invocations with variable definitions in one
 113   line. Wrong:
 114
 115   ```c
 116   {
 117           int a = foobar();
 118           uint64_t x = 7;
 119   }
 120   ```
 121
 122   Right:
 123
 124   ```c
 125   {
 126           int a;
 127           uint64_t x = 7;
 128
 129           a = foobar();
 130   }
 131   ```
 132
 133 - Use `goto` for cleaning up, and only use it for that. i.e. you may
 134   only jump to the end of a function, and little else. Never jump
 135   backwards!
 136
 137 - Public API calls (i.e. functions exported by our shared libraries)
 138   must be marked `_public_` and need to be prefixed with `sd_`. No
 139   other functions should be prefixed like that.
 140
 141 - In public API calls, you **must** validate all your input arguments for
 142   programming error with `assert_return()` and return a sensible return
 143   code. In all other calls, it is recommended to check for programming
 144   errors with a more brutal `assert()`. We are more forgiving to public
 145   users than for ourselves! Note that `assert()` and `assert_return()`
 146   really only should be used for detecting programming errors, not for
 147   runtime errors. `assert()` and `assert_return()` by usage of `_likely_()`
 148   inform the compiler that he should not expect these checks to fail,
 149   and they inform fellow programmers about the expected validity and
 150   range of parameters.
 151
 152 - For every function you add, think about whether it is a "logging"
 153   function or a "non-logging" function. "Logging" functions do logging
 154   on their own, "non-logging" function never log on their own and
 155   expect their callers to log. All functions in "library" code,
 156   i.e. in `src/shared/` and suchlike must be "non-logging". Every time a
 157   "logging" function calls a "non-logging" function, it should log
 158   about the resulting errors. If a "logging" function calls another
 159   "logging" function, then it should not generate log messages, so
 160   that log messages are not generated twice for the same errors.
 161
 162 - If possible, do a combined log & return operation:
 163
 164   ```c
 165   r = operation(...);
 166   if (r < 0)
 167           return log_(error|warning|notice|...)_errno(r, "Failed to ...: %m");
 168   ```
 169
 170   If the error value is "synthetic", i.e. it was not received from
 171   the called function, use `SYNTHETIC_ERRNO` wrapper to tell the logging
 172   system to not log the errno value, but still return it:
 173
 174   ```c
 175   n = read(..., s, sizeof s);
 176   if (n != sizeof s)
 177           return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to read ...");
 178   ```
 179
 180 - Avoid static variables, except for caches and very few other
 181   cases. Think about thread-safety! While most of our code is never
 182   used in threaded environments, at least the library code should make
 183   sure it works correctly in them. Instead of doing a lot of locking
 184   for that, we tend to prefer using TLS to do per-thread caching (which
 185   only works for small, fixed-size cache objects), or we disable
 186   caching for any thread that is not the main thread. Use
 187   `is_main_thread()` to detect whether the calling thread is the main
 188   thread.
 189
 190 - Command line option parsing:
 191   - Do not print full `help()` on error, be specific about the error.
 192   - Do not print messages to stdout on error.
 193   - Do not POSIX_ME_HARDER unless necessary, i.e. avoid `+` in option string.
 194
 195 - Do not write functions that clobber call-by-reference variables on
 196   failure. Use temporary variables for these cases and change the
 197   passed in variables only on success.
 198
 199 - When you allocate a file descriptor, it should be made `O_CLOEXEC`
 200   right from the beginning, as none of our files should leak to forked
 201   binaries by default. Hence, whenever you open a file, `O_CLOEXEC` must
 202   be specified, right from the beginning. This also applies to
 203   sockets. Effectively, this means that all invocations to:
 204
 205   - `open()` must get `O_CLOEXEC` passed,
 206   - `socket()` and `socketpair()` must get `SOCK_CLOEXEC` passed,
 207   - `recvmsg()` must get `MSG_CMSG_CLOEXEC` set,
 208   - `F_DUPFD_CLOEXEC` should be used instead of `F_DUPFD`, and so on,
 209   - invocations of `fopen()` should take `e`.
 210
 211
 212 - When you invoke certain calls like `unlink()`, or `mkdir_p()` and you
 213   know it is safe to ignore the error it might return (because a later
 214   call would detect the failure anyway, or because the error is in an
 215   error path and you thus couldn't do anything about it anyway), then
 216   make this clear by casting the invocation explicitly to `(void)`. Code
 217   checks like Coverity understand that, and will not complain about
 218   ignored error codes. Hence, please use this:
 219
 220   ```c
 221   (void) unlink("/foo/bar/baz");
 222   ```
 223
 224   instead of just this:
 225
 226   ```c
 227   unlink("/foo/bar/baz");
 228   ```
 229
 230   Don't cast function calls to `(void)` that return no error
 231   conditions. Specifically, the various `xyz_unref()` calls that return a `NULL`
 232   object shouldn't be cast to `(void)`, since not using the return value does not
 233   hide any errors.
 234
 235 - When you define a destructor or `unref()` call for an object, please
 236   accept a `NULL` object and simply treat this as NOP. This is similar
 237   to how libc `free()` works, which accepts `NULL` pointers and becomes a
 238   NOP for them. By following this scheme a lot of `if` checks can be
 239   removed before invoking your destructor, which makes the code
 240   substantially more readable and robust.
 241
 242 - Related to this: when you define a destructor or `unref()` call for an
 243   object, please make it return the same type it takes and always
 244   return `NULL` from it. This allows writing code like this:
 245
 246   ```c
 247   p = foobar_unref(p);
 248   ```
 249
 250   which will always work regardless if `p` is initialized or not, and
 251   guarantees that `p` is `NULL` afterwards, all in just one line.
 252
 253 - Instead of using `memzero()`/`memset()` to initialize structs allocated
 254   on the stack, please try to use c99 structure initializers. It's
 255   short, prettier and actually even faster at execution. Hence:
 256
 257   ```c
 258   struct foobar t = {
 259           .foo = 7,
 260           .bar = "bazz",
 261   };
 262   ```
 263
 264   instead of:
 265
 266   ```c
 267   struct foobar t;
 268   zero(t);
 269   t.foo = 7;
 270   t.bar = "bazz";
 271   ```
 272
 273 - When returning a return code from `main()`, please preferably use
 274   `EXIT_FAILURE` and `EXIT_SUCCESS` as defined by libc.
 275
 276 - The order in which header files are included doesn't matter too
 277   much. systemd-internal headers must not rely on an include order, so
 278   it is safe to include them in any order possible.
 279   However, to not clutter global includes, and to make sure internal
 280   definitions will not affect global headers, please always include the
 281   headers of external components first (these are all headers enclosed
 282   in <>), followed by our own exported headers (usually everything
 283   that's prefixed by `sd-`), and then followed by internal headers.
 284   Furthermore, in all three groups, order all includes alphabetically
 285   so duplicate includes can easily be detected.
 286
 287 - To implement an endless loop, use `for (;;)` rather than `while (1)`.
 288   The latter is a bit ugly anyway, since you probably really
 289   meant `while (true)`. To avoid the discussion what the right
 290   always-true expression for an infinite while loop is, our
 291   recommendation is to simply write it without any such expression by
 292   using `for (;;)`.
 293
 294 - Commit message subject lines should be prefixed with an appropriate
 295   component name of some kind. For example "journal: ", "nspawn: " and
 296   so on.
 297
 298 - Do not use "Signed-Off-By:" in your commit messages. That's a kernel
 299   thing we don't do in the systemd project.
 300
 301 - Avoid leaving long-running child processes around, i.e. `fork()`s that
 302   are not followed quickly by an `execv()` in the child. Resource
 303   management is unclear in this case, and memory CoW will result in
 304   unexpected penalties in the parent much, much later on.
 305
 306 - Don't block execution for arbitrary amounts of time using `usleep()`
 307   or a similar call, unless you really know what you do. Just "giving
 308   something some time", or so is a lazy excuse. Always wait for the
 309   proper event, instead of doing time-based poll loops.
 310
 311 - To determine the length of a constant string `"foo"`, don't bother with
 312   `sizeof("foo")-1`, please use `strlen()` instead (both gcc and clang optimize
 313   the call away for fixed strings). The only exception is when declaring an
 314   array. In that case use STRLEN, which evaluates to a static constant and
 315   doesn't force the compiler to create a VLA.
 316
 317 - Please avoid using global variables as much as you can. And if you
 318   do use them make sure they are static at least, instead of
 319   exported. Especially in library-like code it is important to avoid
 320   global variables. Why are global variables bad? They usually hinder
 321   generic reusability of code (since they break in threaded programs,
 322   and usually would require locking there), and as the code using them
 323   has side-effects make programs non-transparent. That said, there are
 324   many cases where they explicitly make a lot of sense, and are OK to
 325   use. For example, the log level and target in `log.c` is stored in a
 326   global variable, and that's OK and probably expected by most. Also
 327   in many cases we cache data in global variables. If you add more
 328   caches like this, please be careful however, and think about
 329   threading. Only use static variables if you are sure that
 330   thread-safety doesn't matter in your case. Alternatively, consider
 331   using TLS, which is pretty easy to use with gcc's `thread_local`
 332   concept. It's also OK to store data that is inherently global in
 333   global variables, for example data parsed from command lines, see
 334   below.
 335
 336 - If you parse a command line, and want to store the parsed parameters
 337   in global variables, please consider prefixing their names with
 338   `arg_`. We have been following this naming rule in most of our
 339   tools, and we should continue to do so, as it makes it easy to
 340   identify command line parameter variables, and makes it clear why it
 341   is OK that they are global variables.
 342
 343 - When exposing public C APIs, be careful what function parameters you make
 344   `const`. For example, a parameter taking a context object should probably not
 345   be `const`, even if you are writing an otherwise read-only accessor function
 346   for it. The reason is that making it `const` fixates the contract that your
 347   call won't alter the object ever, as part of the API. However, that's often
 348   quite a promise, given that this even prohibits object-internal caching or
 349   lazy initialization of object variables. Moreover, it's usually not too useful
 350   for client applications. Hence, please be careful and avoid `const` on object
 351   parameters, unless you are very sure `const` is appropriate.
 352
 353 - Make sure to enforce limits on every user controllable resource. If the user
 354   can allocate resources in your code, your code must enforce some form of
 355   limits after which it will refuse operation. It's fine if it is hard-coded (at
 356   least initially), but it needs to be there. This is particularly important
 357   for objects that unprivileged users may allocate, but also matters for
 358   everything else any user may allocated.
 359
 360 - You might wonder what kind of common code belongs in `src/shared/` and what
 361   belongs in `src/basic/`. The split is like this: anything that is used to
 362   implement the public shared object we provide (sd-bus, sd-login, sd-id128,
 363   nss-systemd, nss-mymachines, nss-resolve, nss-myhostname, pam_systemd), must
 364   be located in `src/basic` (those objects are not allowed to link to
 365   libsystemd-shared.so). Conversely, anything which is shared between multiple
 366   components and does not need to be in `src/basic/`, should be in
 367   `src/shared/`.
 368
 369   To summarize:
 370
 371   `src/basic/`
 372   - may be used by all code in the tree
 373   - may not use any code outside of `src/basic/`
 374
 375   `src/libsystemd/`
 376   - may be used by all code in the tree, except for code in `src/basic/`
 377   - may not use any code outside of `src/basic/`, `src/libsystemd/`
 378
 379   `src/shared/`
 380   - may be used by all code in the tree, except for code in `src/basic/`,
 381     `src/libsystemd/`, `src/nss-*`, `src/login/pam_systemd.*`, and files under
 382     `src/journal/` that end up in `libjournal-client.a` convenience library.
 383   - may not use any code outside of `src/basic/`, `src/libsystemd/`, `src/shared/`
 384
 385 - Our focus is on the GNU libc (glibc), not any other libcs. If other libcs are
 386   incompatible with glibc it's on them. However, if there are equivalent POSIX
 387   and Linux/GNU-specific APIs, we generally prefer the POSIX APIs. If there
 388   aren't, we are happy to use GNU or Linux APIs, and expect non-GNU
 389   implementations of libc to catch up with glibc.
 390
 391 - Whenever installing a signal handler, make sure to set `SA_RESTART` for it, so
 392   that interrupted system calls are automatically restarted, and we minimize
 393   hassles with handling `EINTR` (in particular as `EINTR` handling is pretty broken
 394   on Linux).
 395
 396 - When applying C-style unescaping as well as specifier expansion on the same
 397   string, always apply the C-style unescaping fist, followed by the specifier
 398   expansion. When doing the reverse, make sure to escape `%` in specifier-style
 399   first (i.e. `%` → `%%`), and then do C-style escaping where necessary.
 400
 401 - It's a good idea to use `O_NONBLOCK` when opening 'foreign' regular files, i.e.
 402   file system objects that are supposed to be regular files whose paths where
 403   specified by the user and hence might actually refer to other types of file
 404   system objects. This is a good idea so that we don't end up blocking on
 405   'strange' file nodes, for example if the user pointed us to a FIFO or device
 406   node which may block when opening. Moreover even for actual regular files
 407   `O_NONBLOCK` has a benefit: it bypasses any mandatory lock that might be in
 408   effect on the regular file. If in doubt consider turning off `O_NONBLOCK` again
 409   after opening.
 410
 411 ## Memory Allocation
 412
 413 - Always check OOM. There is no excuse. In program code, you can use
 414   `log_oom()` for then printing a short message, but not in "library" code.
 415
 416 - Avoid fixed-size string buffers, unless you really know the maximum size and
 417   that maximum size is small. They are a source of errors, since they possibly
 418   result in truncated strings. It is often nicer to use dynamic memory,
 419   `alloca()` or VLAs. If you do allocate fixed-size strings on the stack, then
 420   it is probably only OK if you either use a maximum size such as `LINE_MAX`,
 421   or count in detail the maximum size a string can have. (`DECIMAL_STR_MAX` and
 422   `DECIMAL_STR_WIDTH` macros are your friends for this!)
 423
 424   Or in other words, if you use `char buf[256]` then you are likely doing
 425   something wrong!
 426
 427 - Make use of `_cleanup_free_` and friends. It makes your code much nicer to
 428   read (and shorter)!
 429
 430 - Use `alloca()`, but never forget that it is not OK to invoke `alloca()`
 431   within a loop or within function call parameters. `alloca()` memory is
 432   released at the end of a function, and not at the end of a `{}` block. Thus,
 433   if you invoke it in a loop, you keep increasing the stack pointer without
 434   ever releasing memory again. (VLAs have better behavior in this case, so
 435   consider using them as an alternative.)  Regarding not using `alloca()`
 436   within function parameters, see the BUGS section of the `alloca(3)` man page.
 437
 438 - If you want to concatenate two or more strings, consider using `strjoina()`
 439   or `strjoin()` rather than `asprintf()`, as the latter is a lot slower. This
 440   matters particularly in inner loops (but note that `strjoina()` cannot be
 441   used there).
 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!