CODING_STYLE.md

   1 #### General Notes
   2
   3 - The coding style guide refers to new code. But legacy code can be cleaned up
   4   and we are happy to take those patches.
   5 - Just because there is still code in LXC that doesn't adhere to the coding
   6   standards outlined here does not license not adhering to the coding style. In
   7   other words: please stick to the coding style.
   8 - Maintainers are free to ignore rules specified here when merging pull
   9   requests. This guideline might seem a little weird but it exits to ease new
  10   developers into the code base and to prevent unnecessary bikeshedding. If
  11   a maintainer feels hat enforcing a specific rule in a given commit would do
  12   more harm than good they should always feel free to ignore the rule.
  13
  14   Furthermore, when merging pull requests that do not adhere to our coding
  15   style maintainers should feel free to grab the commit, adapt it to our coding
  16   style and add their Signed-off-by line to it. This is especially helpful to
  17   make it easier for first-time contributors and to prevent having pull
  18   requests being stuck in the merge queue because of minor details.
  19 - We currently do not provide automatic coding style checks but if a suitable
  20   tool is found we are happy to integrate it into our test suite. It is
  21   possible and recommended to use the `clang-format` binary to check your code.
  22   The following options are an approximation of the coding style used here.
  23   Simply create a file called `.clang-format` in your home directory with the
  24   following options:
  25   ```sh
  26   cat << EOF > "${HOME}"/.clang-format
  27   BreakBeforeBraces: Attach
  28   AlwaysBreakBeforeMultilineStrings: false
  29   BreakBeforeBinaryOperators: None
  30   MaxEmptyLinesToKeep: 1
  31   PenaltyBreakBeforeFirstCallParameter: 1000000
  32   BinPackArguments: true
  33   BinPackParameters: true
  34   AllowAllParametersOfDeclarationOnNextLine: false
  35   AlignAfterOpenBracket: true
  36   SpacesInSquareBrackets: false
  37   SpacesInCStyleCastParentheses: false
  38   SpaceInEmptyParentheses: false
  39   SpaceBeforeParens: ControlStatements
  40   SpaceAfterCStyleCast: false
  41   SortIncludes: true
  42   PenaltyReturnTypeOnItsOwnLine: 10000
  43   PenaltyExcessCharacter: 10
  44   Language: Cpp
  45   ForEachMacros: ['lxc_list_for_each', 'lxc_list_for_each_safe']
  46   AllowShortLoopsOnASingleLine: false
  47   AllowShortIfStatementsOnASingleLine: false
  48   AllowShortFunctionsOnASingleLine: None
  49   AllowShortCaseLabelsOnASingleLine: false
  50   AllowShortBlocksOnASingleLine: false
  51   BasedOnStyle: LLVM
  52   TabWidth: 8
  53   IndentWidth: 8
  54   UseTab: Always
  55   BreakBeforeBraces: Linux
  56   AllowShortIfStatementsOnASingleLine: false
  57   IndentCaseLabels: false
  58   EOF
  59   ```
  60   However, it will not handle all cases correctly. For example, most `struct`
  61   initializations will not be correct. In such cases please refer to the coding
  62   style here.
  63
  64 #### Only Use Tabs
  65
  66 - LXC uses tabs.
  67
  68 #### Only use `/* */` Style Comments
  69
  70 - Any comments that are added must use `/* */`.
  71 - All comments should start on the same line as the opening `/*`.
  72 - Single-line comments should simply be placed between `/* */`. For example:
  73   ```
  74   /* Define pivot_root() if missing from the C library */
  75   ```
  76 - Multi-line comments should end with the closing `*/` on a separate line. For
  77   example:
  78   ```
  79   /* At this point the old-root is mounted on top of our new-root
  80    * To unmounted it we must not be chdir()ed into it, so escape back
  81    * to old-root.
  82    */
  83   ```
  84
  85 #### Try To Wrap At 80chars
  86
  87 - This is not strictly enforced. It is perfectly valid to sometimes
  88   overflow this limit if it helps clarity. Nonetheless, try to stick to it
  89   and use common sense to decide when not to.
  90
  91 #### Error Messages
  92
  93 - Error messages must start with a capital letter and must **not** end with a
  94   punctuation sign.
  95 - They should be descriptive, without being needlessly long. It is best to just
  96   use already existing error messages as examples.
  97 - Examples of acceptable error messages are:
  98   ```
  99   SYSERROR("Failed to create directory \"%s\"", path);
 100   WARN("\"/dev\" directory does not exist. Proceeding without autodev being set up");
 101   ```
 102
 103 #### Return Error Codes
 104
 105 - When writing a function that can fail in a non-binary way try to return
 106   meaningful negative error codes (e.g. `return -EINVAL;`).
 107
 108 #### All Unexported Functions Must Be Declared `static`
 109
 110 - Functions which are only used in the current file and are not exported
 111   within the codebase need to be declared with the `static` attribute.
 112
 113 #### All Exported Functions Must Be Declared `extern` In A Header File
 114
 115 - Functions which are used in different files in the library should be declared
 116   in a suitable `*.c` file and exposed in a suitable `*.h` file. When defining
 117   the function in the `*.c` file the function signature should not be preceded
 118   by the `extern` keyword. When declaring the function signature in the `*.h`
 119   file it must be preceded by the `extern` keyword. For example:
 120   ```
 121   /* Valid function definition in a *.c file */
 122   ssize_t lxc_write_nointr(int fd, const void* buf, size_t count)
 123   {
 124           ssize_t ret;
 125   again:
 126           ret = write(fd, buf, count);
 127           if (ret < 0 && errno == EINTR)
 128                   goto again;
 129           return ret;
 130   }
 131
 132   /* Valid function declaration in a *.h file */
 133   extern ssize_t lxc_write_nointr(int fd, const void* buf, size_t count);
 134   ```
 135
 136 #### All Names Must Be In lower_case
 137
 138 - All functions and variable names must use lower case.
 139
 140 #### Declaring Variables
 141
 142 - variables should be declared at the top of the function or at the beginning
 143   of a new scope but **never** in the middle of a scope
 144 1. uninitialized variables
 145   - put base types before complex types
 146   - put standard types defined by libc before types defined by LXC
 147   - put multiple declarations of the same type on the same line
 148 2. initialized variables
 149   - put base types before complex types
 150   - put standard types defined by libc before types defined by LXC
 151   - put multiple declarations of the same type on the same line
 152 - Examples of good declarations can be seen in the following function:
 153   ```
 154   int lxc_clear_procs(struct lxc_conf *c, const char *key)
 155   {
 156           struct lxc_list *it, *next;
 157           bool all = false;
 158           const char *k = NULL;
 159
 160           if (strcmp(key, "lxc.proc") == 0)
 161                   all = true;
 162           else if (strncmp(key, "lxc.proc.", sizeof("lxc.proc.") - 1) == 0)
 163                   k = key + sizeof("lxc.proc.") - 1;
 164           else
 165                   return -1;
 166
 167           lxc_list_for_each_safe(it, &c->procs, next) {
 168                   struct lxc_proc *proc = it->elem;
 169
 170                   if (!all && strcmp(proc->filename, k) != 0)
 171                           continue;
 172                   lxc_list_del(it);
 173                   free(proc->filename);
 174                   free(proc->value);
 175                   free(proc);
 176                   free(it);
 177           }
 178
 179           return 0;
 180   }
 181     ```
 182
 183 #### Single-line `if` blocks should not be enclosed in `{}`
 184
 185 - This also affects `if-else` ladders if and only if all constituting
 186   conditions are
 187   single-line conditions. If there is at least one non-single-line
 188   condition `{}` must be used.
 189 - For example:
 190   ```
 191   /* no brackets needed */
 192   if (size > INT_MAX)
 193           return -EFBIG;
 194
 195   /* The else branch has more than one-line and so needs {}. This entails that
 196    * the if branch also needs to have {}.
 197    */
 198   if ( errno == EROFS ) {
 199           WARN("Warning: Read Only file system while creating %s", path);
 200   } else {
 201           SYSERROR("Error creating %s", path);
 202           return -1;
 203   }
 204
 205   /* also fine */
 206   for (i = 0; list[i]; i++)
 207         if (strcmp(list[i], entry) == 0)
 208                 return true;
 209
 210   /* also fine */
 211   if (ret < 0)
 212           WARN("Failed to set FD_CLOEXEC flag on slave fd %d of "
 213                "pty device \"%s\": %s", pty_info->slave,
 214                pty_info->name, strerror(errno));
 215
 216   /* also fine */
 217   if (ret == 0)
 218           for (i = 0; i < sizeof(limit_opt)/sizeof(limit_opt[0]); ++i) {
 219                   if (strcmp(res, limit_opt[i].name) == 0)
 220                           return limit_opt[i].value;
 221           }
 222   ```
 223
 224 #### Functions Not Returning Booleans Must Assign Return Value Before Performing Checks
 225
 226 - When checking whether a function not returning booleans was successful or not
 227   the returned value must be assigned before it is checked (`str{n}cmp()`
 228   functions being one notable exception). For example:
 229   ```
 230   /* assign value to "ret" first */
 231   ret = mount(sourcepath, cgpath, "cgroup", remount_flags, NULL);
 232   /* check whether function was successful */
 233   if (ret < 0) {
 234           SYSERROR("Failed to remount \"%s\" ro", cgpath);
 235           free(sourcepath);
 236           return -1;
 237   }
 238   ```
 239   Functions returning booleans can be checked directly. For example:
 240   ```
 241   extern bool lxc_string_in_array(const char *needle, const char **haystack);
 242
 243   /* check right away */
 244   if (lxc_string_in_array("ns", (const char **)h->subsystems))
 245           continue;
 246   ```
 247
 248 #### Non-Boolean Functions That Behave Like Boolean Functions Must Explicitly Check Against A Value
 249
 250 - This rule mainly exists for `str{n}cmp()` type functions. In most cases they
 251   are used like a boolean function to check whether a string matches or not.
 252   But they return an integer. It is perfectly fine to check `str{n}cmp()`
 253   functions directly but you must compare explicitly against a value. That is
 254   to say, while they are conceptually boolean functions they shouldn't be
 255   treated as such since they don't really behave like boolean functions. So
 256   `if (!str{n}cmp())` and `if (str{n}cmp())` checks must not be used. Good
 257   examples are found in the following functions:
 258   ```
 259   static int set_config_hooks(const char *key, const char *value,
 260                               struct lxc_conf *lxc_conf, void *data)
 261
 262           char *copy;
 263
 264           if (lxc_config_value_empty(value))
 265                   return lxc_clear_hooks(lxc_conf, key);
 266
 267           if (strcmp(key + 4, "hook") == 0) {
 268                   ERROR("lxc.hook must not have a value");
 269                   return -1;
 270           }
 271
 272           copy = strdup(value);
 273           if (!copy)
 274                   return -1;
 275
 276           if (strcmp(key + 9, "pre-start") == 0)
 277                   return add_hook(lxc_conf, LXCHOOK_PRESTART, copy);
 278           else if (strcmp(key + 9, "start-host") == 0)
 279                   return add_hook(lxc_conf, LXCHOOK_START_HOST, copy);
 280           else if (strcmp(key + 9, "pre-mount") == 0)
 281                   return add_hook(lxc_conf, LXCHOOK_PREMOUNT, copy);
 282           else if (strcmp(key + 9, "autodev") == 0)
 283                   return add_hook(lxc_conf, LXCHOOK_AUTODEV, copy);
 284           else if (strcmp(key + 9, "mount") == 0)
 285                   return add_hook(lxc_conf, LXCHOOK_MOUNT, copy);
 286           else if (strcmp(key + 9, "start") == 0)
 287                   return add_hook(lxc_conf, LXCHOOK_START, copy);
 288           else if (strcmp(key + 9, "stop") == 0)
 289                   return add_hook(lxc_conf, LXCHOOK_STOP, copy);
 290           else if (strcmp(key + 9, "post-stop") == 0)
 291                   return add_hook(lxc_conf, LXCHOOK_POSTSTOP, copy);
 292           else if (strcmp(key + 9, "clone") == 0)
 293                   return add_hook(lxc_conf, LXCHOOK_CLONE, copy);
 294           else if (strcmp(key + 9, "destroy") == 0)
 295                   return add_hook(lxc_conf, LXCHOOK_DESTROY, copy);
 296
 297           free(copy);
 298           return -1;
 299   }
 300   ```
 301
 302 #### Do Not Use C99 Variable Length Arrays (VLA)
 303
 304 - They are made optional and there is no guarantee that future C standards
 305   will support them.
 306
 307 #### Use Standard libc Macros When Exiting
 308
 309 - libc provides `EXIT_FAILURE` and `EXIT_SUCCESS`. Use them whenever possible
 310   in the child of `fork()`ed process or when exiting from a `main()` function.
 311
 312 #### Use `goto`s
 313
 314 `goto`s are an essential language construct of C and are perfect to perform
 315 cleanup operations or simplify the logic of functions. However, here are the
 316 rules to use them:
 317 - use descriptive `goto` labels.
 318   For example, if you know that this label is only used as an error path you
 319   should use something like `on_error` instead of `out` as label name.
 320 - **only** jump downwards unless you are handling `EAGAIN` errors and want to
 321   avoid `do-while` constructs.
 322 - An example of a good usage of `goto` is:
 323   ```
 324   static int set_config_idmaps(const char *key, const char *value,
 325                              struct lxc_conf *lxc_conf, void *data)
 326   {
 327           unsigned long hostid, nsid, range;
 328           char type;
 329           int ret;
 330           struct lxc_list *idmaplist = NULL;
 331           struct id_map *idmap = NULL;
 332
 333           if (lxc_config_value_empty(value))
 334                   return lxc_clear_idmaps(lxc_conf);
 335
 336           idmaplist = malloc(sizeof(*idmaplist));
 337           if (!idmaplist)
 338                   goto on_error;
 339
 340           idmap = malloc(sizeof(*idmap));
 341           if (!idmap)
 342                   goto on_error;
 343           memset(idmap, 0, sizeof(*idmap));
 344
 345           ret = parse_idmaps(value, &type, &nsid, &hostid, &range);
 346           if (ret < 0) {
 347                   ERROR("Failed to parse id mappings");
 348                   goto on_error;
 349           }
 350
 351           INFO("Read uid map: type %c nsid %lu hostid %lu range %lu", type, nsid, hostid, range);
 352           if (type == 'u')
 353                   idmap->idtype = ID_TYPE_UID;
 354           else if (type == 'g')
 355                   idmap->idtype = ID_TYPE_GID;
 356           else
 357                   goto on_error;
 358
 359           idmap->hostid = hostid;
 360           idmap->nsid = nsid;
 361           idmap->range = range;
 362           idmaplist->elem = idmap;
 363           lxc_list_add_tail(&lxc_conf->id_map, idmaplist);
 364
 365           if (!lxc_conf->root_nsuid_map && idmap->idtype == ID_TYPE_UID)
 366                   if (idmap->nsid == 0)
 367                           lxc_conf->root_nsuid_map = idmap;
 368
 369
 370           if (!lxc_conf->root_nsgid_map && idmap->idtype == ID_TYPE_GID)
 371                   if (idmap->nsid == 0)
 372                           lxc_conf->root_nsgid_map = idmap;
 373
 374           idmap = NULL;
 375
 376           return 0;
 377
 378   on_error:
 379           free(idmaplist);
 380           free(idmap);
 381
 382           return -1;
 383   }
 384   ```
 385
 386 #### Use Booleans instead of integers
 387
 388 - When something can be conceptualized in a binary way use a boolean not
 389   an integer.
 390
 391 #### Cleanup Functions Must Handle The Object's Null Type And Being Passed Already Cleaned Up Objects
 392
 393 - If you implement a custom cleanup function to e.g. free a complex type
 394   you declared you must ensure that the object's null type is handled and
 395   treated as a NOOP. For example:
 396   ```
 397   void lxc_free_array(void **array, lxc_free_fn element_free_fn)
 398   {
 399           void **p;
 400           for (p = array; p && *p; p++)
 401                   element_free_fn(*p);
 402           free((void*)array);
 403   }
 404   ```
 405 - Cleanup functions should also expect to be passed already cleaned up objects.
 406   One way to handle this cleanly is to initialize the cleaned up variable to
 407   a special value that signals the function that the element has already been
 408   freed on the next call. For example, the following function cleans up file
 409   descriptors and sets the already closed file descriptors to `-EBADF`. On the
 410   next call it can simply check whether the file descriptor is positive and
 411   move on if it isn't:
 412   ```
 413   static void lxc_put_attach_clone_payload(struct attach_clone_payload *p)
 414   {
 415           if (p->ipc_socket >= 0) {
 416                   shutdown(p->ipc_socket, SHUT_RDWR);
 417                   close(p->ipc_socket);
 418                   p->ipc_socket = -EBADF;
 419           }
 420
 421           if (p->pty_fd >= 0) {
 422                   close(p->pty_fd);
 423                   p->pty_fd = -EBADF;
 424           }
 425
 426           if (p->init_ctx) {
 427                   lxc_proc_put_context_info(p->init_ctx);
 428                   p->init_ctx = NULL;
 429           }
 430   }
 431   ```
 432
 433 ### Cast to `(void)` When Intentionally Ignoring Return Values
 434
 435 - There are cases where you do not care about the return value of a function.
 436   Please cast the return value to `(void)` when doing so.
 437 - Standard library functions or functions which are known to be ignored by
 438   default do not need to be cast to `(void)`. Classical candidates are
 439   `close()` and `fclose()`.
 440 - A good example is:
 441   ```
 442   for (i = 0; hierarchies[i]; i++) {
 443           char *fullpath;
 444           char *path = hierarchies[i]->fullcgpath;
 445
 446           ret = chowmod(path, destuid, nsgid, 0755);
 447           if (ret < 0)
 448                   return -1;
 449
 450           /* failures to chown() these are inconvenient but not
 451            * detrimental we leave these owned by the container launcher,
 452            * so that container root can write to the files to attach.  we
 453            * chmod() them 664 so that container systemd can write to the
 454            * files (which systemd in wily insists on doing).
 455            */
 456
 457           if (hierarchies[i]->version == cgroup_super_magic) {
 458                   fullpath = must_make_path(path, "tasks", null);
 459                   (void)chowmod(fullpath, destuid, nsgid, 0664);
 460                   free(fullpath);
 461           }
 462
 463           fullpath = must_make_path(path, "cgroup.procs", null);
 464           (void)chowmod(fullpath, destuid, 0, 0664);
 465           free(fullpath);
 466
 467           if (hierarchies[i]->version != cgroup2_super_magic)
 468                   continue;
 469
 470           fullpath = must_make_path(path, "cgroup.subtree_control", null);
 471           (void)chowmod(fullpath, destuid, nsgid, 0664);
 472           free(fullpath);
 473
 474           fullpath = must_make_path(path, "cgroup.threads", null);
 475           (void)chowmod(fullpath, destuid, nsgid, 0664);
 476           free(fullpath);
 477   }
 478   ```
 479
 480 #### Use `for (;;)` instead of `while (1)` or `while (true)`
 481
 482 - Let's be honest, it is really the only sensible way to do this.
 483
 484 #### Use The Set Of Supported DCO Statements
 485
 486 - Signed-off-by: Random J Developer <random@developer.org>
 487   - You did write this code or have the right to contribute it to LXC.
 488 - Acked-by: Random J Developer <random@developer.org>
 489   - You did read the code and think it is correct. This is usually only used by
 490     maintainers or developers that have made significant contributions and can
 491     vouch for the correctness of someone else's code.
 492 - Reviewed-by: Random J Developer <random@developer.org>
 493   - You did review the code and vouch for its correctness, i.e. you'd be
 494     prepared to fix bugs it might cause. This is usually only used by
 495     maintainers or developers that have made significant contributions and can
 496     vouch for the correctness of someone else's code.
 497 - Co-developed-by: Random J Developer <random@developer.org>
 498   - The code can not be reasonably attributed to a single developer, i.e.
 499     you worked on this together.
 500 - Tested-by: Random J Developer <random@developer.org>
 501   - You verified that the code fixes a given bug or is behaving as advertised.
 502 - Reported-by: Random J Developer <random@developer.org>
 503   - You found and reported the bug.
 504 - Suggested-by: Random J Developer <random@developer.org>
 505   - You wrote the code but someone contributed the idea. This line is usually
 506     overlooked but it is a sign of good etiquette and coding ethics: if someone
 507     helped you solve a problem or had a clever idea do not silently claim it by
 508     slapping your Signed-off-by underneath. Be honest and add a Suggested-by.
 509
 510 #### Commit Message Outline
 511
 512 - You **must** stick to the 80chars limit especially in the title of the commit
 513   message.
 514 - Please use English commit messages only.
 515 - use meaningful commit messages.
 516 - Use correct spelling and grammar.
 517   If you are not a native speaker and/or feel yourself struggling with this it
 518   is perfectly fine to point this out and there's no need to apologize. Usually
 519   developers will be happy to pull your branch and adopt the commit message.
 520 - Please always use the affected file (without the file type suffix) or module
 521   as a prefix in the commit message.
 522 - Examples of good commit messages are:
 523   ```
 524   commit b87243830e3b5e95fa31a17cf1bfebe55353bf13
 525   Author: Felix Abecassis <fabecassis@nvidia.com>
 526   Date:   Fri Feb 2 06:19:13 2018 -0800
 527
 528       hooks: change the semantic of NVIDIA_VISIBLE_DEVICES=""
 529
 530       With LXC, you can override the value of an environment variable to
 531       null, but you can't unset an existing variable.
 532
 533       The NVIDIA hook was previously activated when NVIDIA_VISIBLE_DEVICES
 534       was set to null. As a result, it was not possible to disable the hook
 535       by overriding the environment variable in the configuration.
 536
 537       The hook can now be disabled by setting NVIDIA_VISIBLE_DEVICES to
 538       null or to the new special value "void".
 539
 540       Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>
 541
 542
 543   commit d6337a5f9dc7311af168aa3d586fdf239f5a10d3
 544   Author: Christian Brauner <christian.brauner@ubuntu.com>
 545   Date:   Wed Jan 31 16:25:11 2018 +0100
 546
 547       cgroups: get controllers on the unified hierarchy
 548
 549       Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
 550
 551   ```
 552 #### Use `_exit()` To Terminate `fork()`ed Child Processes
 553
 554 - When `fork()`ing off a child process use `_exit()` to terminate it instead of
 555   `exit()`. The `exit()` function is not thread-safe and thus not suited for
 556   the shared library which must ensure that it is thread-safe.