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