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 invoke certain calls like `unlink()`, or `mkdir_p()` and you
 200   know it is safe to ignore the error it might return (because a later
 201   call would detect the failure anyway, or because the error is in an
 202   error path and you thus couldn't do anything about it anyway), then
 203   make this clear by casting the invocation explicitly to `(void)`. Code
 204   checks like Coverity understand that, and will not complain about
 205   ignored error codes. Hence, please use this:
 206
 207   ```c
 208   (void) unlink("/foo/bar/baz");
 209   ```
 210
 211   instead of just this:
 212
 213   ```c
 214   unlink("/foo/bar/baz");
 215   ```
 216
 217   Don't cast function calls to `(void)` that return no error
 218   conditions. Specifically, the various `xyz_unref()` calls that return a `NULL`
 219   object shouldn't be cast to `(void)`, since not using the return value does not
 220   hide any errors.
 221
 222 - When you define a destructor or `unref()` call for an object, please
 223   accept a `NULL` object and simply treat this as NOP. This is similar
 224   to how libc `free()` works, which accepts `NULL` pointers and becomes a
 225   NOP for them. By following this scheme a lot of `if` checks can be
 226   removed before invoking your destructor, which makes the code
 227   substantially more readable and robust.
 228
 229 - Related to this: when you define a destructor or `unref()` call for an
 230   object, please make it return the same type it takes and always
 231   return `NULL` from it. This allows writing code like this:
 232
 233   ```c
 234   p = foobar_unref(p);
 235   ```
 236
 237   which will always work regardless if `p` is initialized or not, and
 238   guarantees that `p` is `NULL` afterwards, all in just one line.
 239
 240 - Instead of using `memzero()`/`memset()` to initialize structs allocated
 241   on the stack, please try to use c99 structure initializers. It's
 242   short, prettier and actually even faster at execution. Hence:
 243
 244   ```c
 245   struct foobar t = {
 246           .foo = 7,
 247           .bar = "bazz",
 248   };
 249   ```
 250
 251   instead of:
 252
 253   ```c
 254   struct foobar t;
 255   zero(t);
 256   t.foo = 7;
 257   t.bar = "bazz";
 258   ```
 259
 260 - When returning a return code from `main()`, please preferably use
 261   `EXIT_FAILURE` and `EXIT_SUCCESS` as defined by libc.
 262
 263 - The order in which header files are included doesn't matter too
 264   much. systemd-internal headers must not rely on an include order, so
 265   it is safe to include them in any order possible.
 266   However, to not clutter global includes, and to make sure internal
 267   definitions will not affect global headers, please always include the
 268   headers of external components first (these are all headers enclosed
 269   in <>), followed by our own exported headers (usually everything
 270   that's prefixed by `sd-`), and then followed by internal headers.
 271   Furthermore, in all three groups, order all includes alphabetically
 272   so duplicate includes can easily be detected.
 273
 274 - To implement an endless loop, use `for (;;)` rather than `while (1)`.
 275   The latter is a bit ugly anyway, since you probably really
 276   meant `while (true)`. To avoid the discussion what the right
 277   always-true expression for an infinite while loop is, our
 278   recommendation is to simply write it without any such expression by
 279   using `for (;;)`.
 280
 281 - Commit message subject lines should be prefixed with an appropriate
 282   component name of some kind. For example "journal: ", "nspawn: " and
 283   so on.
 284
 285 - Do not use "Signed-Off-By:" in your commit messages. That's a kernel
 286   thing we don't do in the systemd project.
 287
 288 - Avoid leaving long-running child processes around, i.e. `fork()`s that
 289   are not followed quickly by an `execv()` in the child. Resource
 290   management is unclear in this case, and memory CoW will result in
 291   unexpected penalties in the parent much, much later on.
 292
 293 - Don't block execution for arbitrary amounts of time using `usleep()`
 294   or a similar call, unless you really know what you do. Just "giving
 295   something some time", or so is a lazy excuse. Always wait for the
 296   proper event, instead of doing time-based poll loops.
 297
 298 - To determine the length of a constant string `"foo"`, don't bother with
 299   `sizeof("foo")-1`, please use `strlen()` instead (both gcc and clang optimize
 300   the call away for fixed strings). The only exception is when declaring an
 301   array. In that case use STRLEN, which evaluates to a static constant and
 302   doesn't force the compiler to create a VLA.
 303
 304 - Please avoid using global variables as much as you can. And if you
 305   do use them make sure they are static at least, instead of
 306   exported. Especially in library-like code it is important to avoid
 307   global variables. Why are global variables bad? They usually hinder
 308   generic reusability of code (since they break in threaded programs,
 309   and usually would require locking there), and as the code using them
 310   has side-effects make programs non-transparent. That said, there are
 311   many cases where they explicitly make a lot of sense, and are OK to
 312   use. For example, the log level and target in `log.c` is stored in a
 313   global variable, and that's OK and probably expected by most. Also
 314   in many cases we cache data in global variables. If you add more
 315   caches like this, please be careful however, and think about
 316   threading. Only use static variables if you are sure that
 317   thread-safety doesn't matter in your case. Alternatively, consider
 318   using TLS, which is pretty easy to use with gcc's `thread_local`
 319   concept. It's also OK to store data that is inherently global in
 320   global variables, for example data parsed from command lines, see
 321   below.
 322
 323 - If you parse a command line, and want to store the parsed parameters
 324   in global variables, please consider prefixing their names with
 325   `arg_`. We have been following this naming rule in most of our
 326   tools, and we should continue to do so, as it makes it easy to
 327   identify command line parameter variables, and makes it clear why it
 328   is OK that they are global variables.
 329
 330 - When exposing public C APIs, be careful what function parameters you make
 331   `const`. For example, a parameter taking a context object should probably not
 332   be `const`, even if you are writing an otherwise read-only accessor function
 333   for it. The reason is that making it `const` fixates the contract that your
 334   call won't alter the object ever, as part of the API. However, that's often
 335   quite a promise, given that this even prohibits object-internal caching or
 336   lazy initialization of object variables. Moreover, it's usually not too useful
 337   for client applications. Hence, please be careful and avoid `const` on object
 338   parameters, unless you are very sure `const` is appropriate.
 339
 340 - Make sure to enforce limits on every user controllable resource. If the user
 341   can allocate resources in your code, your code must enforce some form of
 342   limits after which it will refuse operation. It's fine if it is hard-coded (at
 343   least initially), but it needs to be there. This is particularly important
 344   for objects that unprivileged users may allocate, but also matters for
 345   everything else any user may allocated.
 346
 347 - You might wonder what kind of common code belongs in `src/shared/` and what
 348   belongs in `src/basic/`. The split is like this: anything that is used to
 349   implement the public shared object we provide (sd-bus, sd-login, sd-id128,
 350   nss-systemd, nss-mymachines, nss-resolve, nss-myhostname, pam_systemd), must
 351   be located in `src/basic` (those objects are not allowed to link to
 352   libsystemd-shared.so). Conversely, anything which is shared between multiple
 353   components and does not need to be in `src/basic/`, should be in
 354   `src/shared/`.
 355
 356   To summarize:
 357
 358   `src/basic/`
 359   - may be used by all code in the tree
 360   - may not use any code outside of `src/basic/`
 361
 362   `src/libsystemd/`
 363   - may be used by all code in the tree, except for code in `src/basic/`
 364   - may not use any code outside of `src/basic/`, `src/libsystemd/`
 365
 366   `src/shared/`
 367   - may be used by all code in the tree, except for code in `src/basic/`,
 368     `src/libsystemd/`, `src/nss-*`, `src/login/pam_systemd.*`, and files under
 369     `src/journal/` that end up in `libjournal-client.a` convenience library.
 370   - may not use any code outside of `src/basic/`, `src/libsystemd/`, `src/shared/`
 371
 372 - Our focus is on the GNU libc (glibc), not any other libcs. If other libcs are
 373   incompatible with glibc it's on them. However, if there are equivalent POSIX
 374   and Linux/GNU-specific APIs, we generally prefer the POSIX APIs. If there
 375   aren't, we are happy to use GNU or Linux APIs, and expect non-GNU
 376   implementations of libc to catch up with glibc.
 377
 378 - Whenever installing a signal handler, make sure to set `SA_RESTART` for it, so
 379   that interrupted system calls are automatically restarted, and we minimize
 380   hassles with handling `EINTR` (in particular as `EINTR` handling is pretty broken
 381   on Linux).
 382
 383 - When applying C-style unescaping as well as specifier expansion on the same
 384   string, always apply the C-style unescaping fist, followed by the specifier
 385   expansion. When doing the reverse, make sure to escape `%` in specifier-style
 386   first (i.e. `%` → `%%`), and then do C-style escaping where necessary.
 387
 388 ## Memory Allocation
 389
 390 - Always check OOM. There is no excuse. In program code, you can use
 391   `log_oom()` for then printing a short message, but not in "library" code.
 392
 393 - Avoid fixed-size string buffers, unless you really know the maximum size and
 394   that maximum size is small. They are a source of errors, since they possibly
 395   result in truncated strings. It is often nicer to use dynamic memory,
 396   `alloca()` or VLAs. If you do allocate fixed-size strings on the stack, then
 397   it is probably only OK if you either use a maximum size such as `LINE_MAX`,
 398   or count in detail the maximum size a string can have. (`DECIMAL_STR_MAX` and
 399   `DECIMAL_STR_WIDTH` macros are your friends for this!)
 400
 401   Or in other words, if you use `char buf[256]` then you are likely doing
 402   something wrong!
 403
 404 - Make use of `_cleanup_free_` and friends. It makes your code much nicer to
 405   read (and shorter)!
 406
 407 - Use `alloca()`, but never forget that it is not OK to invoke `alloca()`
 408   within a loop or within function call parameters. `alloca()` memory is
 409   released at the end of a function, and not at the end of a `{}` block. Thus,
 410   if you invoke it in a loop, you keep increasing the stack pointer without
 411   ever releasing memory again. (VLAs have better behavior in this case, so
 412   consider using them as an alternative.)  Regarding not using `alloca()`
 413   within function parameters, see the BUGS section of the `alloca(3)` man page.
 414
 415 - If you want to concatenate two or more strings, consider using `strjoina()`
 416   or `strjoin()` rather than `asprintf()`, as the latter is a lot slower. This
 417   matters particularly in inner loops (but note that `strjoina()` cannot be
 418   used there).
 419
 420 ## Types
 421
 422 - Think about the types you use. If a value cannot sensibly be negative, do not
 423   use `int`, but use `unsigned`.
 424
 425 - Use `char` only for actual characters. Use `uint8_t` or `int8_t` when you
 426   actually mean a byte-sized signed or unsigned integers. When referring to a
 427   generic byte, we generally prefer the unsigned variant `uint8_t`. Do not use
 428   types based on `short`. They *never* make sense. Use `int`, `long`, `long
 429   long`, all in unsigned and signed fashion, and the fixed-size types
 430   `uint8_t`, `uint16_t`, `uint32_t`, `uint64_t`, `int8_t`, `int16_t`, `int32_t`
 431   and so on, as well as `size_t`, but nothing else. Do not use kernel types
 432   like `u32` and so on, leave that to the kernel.
 433
 434 - Stay uniform. For example, always use `usec_t` for time values. Do not mix
 435   `usec` and `msec`, and `usec` and whatnot.
 436
 437 - Never use the `off_t` type, and particularly avoid it in public APIs. It's
 438   really weirdly defined, as it usually is 64-bit and we don't support it any
 439   other way, but it could in theory also be 32-bit. Which one it is depends on
 440   a compiler switch chosen by the compiled program, which hence corrupts APIs
 441   using it unless they can also follow the program's choice. Moreover, in
 442   systemd we should parse values the same way on all architectures and cannot
 443   expose `off_t` values over D-Bus. To avoid any confusion regarding conversion
 444   and ABIs, always use simply `uint64_t` directly.
 445
 446 - Unless you allocate an array, `double` is always a better choice than
 447   `float`. Processors speak `double` natively anyway, so there is no speed
 448   benefit, and on calls like `printf()` `float`s get promoted to `double`s
 449   anyway, so there is no point.
 450
 451 - Use the bool type for booleans, not integers. One exception: in public
 452   headers (i.e those in `src/systemd/sd-*.h`) use integers after all, as `bool`
 453   is C99 and in our public APIs we try to stick to C89 (with a few extension).
 454
 455 ## File Descriptors
 456
 457 - When you allocate a file descriptor, it should be made `O_CLOEXEC` right from
 458   the beginning, as none of our files should leak to forked binaries by
 459   default. Hence, whenever you open a file, `O_CLOEXEC` must be specified,
 460   right from the beginning. This also applies to sockets. Effectively, this
 461   means that all invocations to:
 462
 463   - `open()` must get `O_CLOEXEC` passed,
 464   - `socket()` and `socketpair()` must get `SOCK_CLOEXEC` passed,
 465   - `recvmsg()` must get `MSG_CMSG_CLOEXEC` set,
 466   - `F_DUPFD_CLOEXEC` should be used instead of `F_DUPFD`, and so on,
 467   - invocations of `fopen()` should take `e`.
 468
 469 - It's a good idea to use `O_NONBLOCK` when opening 'foreign' regular files,
 470   i.e.  file system objects that are supposed to be regular files whose paths
 471   where specified by the user and hence might actually refer to other types of
 472   file system objects. This is a good idea so that we don't end up blocking on
 473   'strange' file nodes, for example if the user pointed us to a FIFO or device
 474   node which may block when opening. Moreover even for actual regular files
 475   `O_NONBLOCK` has a benefit: it bypasses any mandatory lock that might be in
 476   effect on the regular file. If in doubt consider turning off `O_NONBLOCK`
 477   again after opening.
 478
 479 ## Referencing Concepts
 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 ## Functions to Avoid
 494
 495 - Use `memzero()` or even better `zero()` instead of `memset(..., 0, ...)`
 496
 497 - Please use `streq()` and `strneq()` instead of `strcmp()`, `strncmp()` where
 498   applicable (i.e. wherever you just care about equality/inequality, not about
 499   the sorting order).
 500
 501 - Never use `strtol()`, `atoi()` and similar calls. Use `safe_atoli()`,
 502   `safe_atou32()` and suchlike instead. They are much nicer to use in most
 503   cases and correctly check for parsing errors.
 504
 505 - `htonl()`/`ntohl()` and `htons()`/`ntohs()` are weird. Please use `htobe32()`
 506   and `htobe16()` instead, it's much more descriptive, and actually says what
 507   really is happening, after all `htonl()` and `htons()` don't operate on
 508   `long`s and `short`s as their name would suggest, but on `uint32_t` and
 509   `uint16_t`. Also, "network byte order" is just a weird name for "big endian",
 510   hence we might want to call it "big endian" right-away.
 511
 512 - Please never use `dup()`. Use `fcntl(fd, F_DUPFD_CLOEXEC, 3)` instead. For
 513   two reason: first, you want `O_CLOEXEC` set on the new `fd` (see
 514   above). Second, `dup()` will happily duplicate your `fd` as 0, 1, 2,
 515   i.e. stdin, stdout, stderr, should those `fd`s be closed. Given the special
 516   semantics of those `fd`s, it's probably a good idea to avoid
 517   them. `F_DUPFD_CLOEXEC` with `3` as parameter avoids them.
 518
 519 - Don't use `fgets()`, it's too hard to properly handle errors such as overly
 520   long lines. Use `read_line()` instead, which is our own function that handles
 521   this much nicer.
 522
 523 - Don't invoke `exit()`, ever. It is not replacement for proper error
 524   handling. Please escalate errors up your call chain, and use normal `return`
 525   to exit from the main function of a process. If you `fork()`ed off a child
 526   process, please use `_exit()` instead of `exit()`, so that the exit handlers
 527   are not run.
 528
 529 - We never use the POSIX version of `basename()` (which glibc defines it in
 530   `libgen.h`), only the GNU version (which glibc defines in `string.h`).  The
 531   only reason to include `libgen.h` is because `dirname()` is needed. Every
 532   time you need that please immediately undefine `basename()`, and add a
 533   comment about it, so that no code ever ends up using the POSIX version!