]>
Commit | Line | Data |
---|---|---|
183d7fa5 TH |
1 | LXC Coding Style Guide |
2 | ====================== | |
3 | ||
4 | In general the LXC project follows the Linux kernel coding style. There are | |
5 | however are a few differences, these are outlined in this document. | |
6 | ||
7 | The Linux kernel coding style guide can be found within the kernel tree: | |
8 | ||
9 | Documentation/process/coding-style.rst | |
10 | ||
11 | It can be accessed online too: | |
12 | ||
13 | https://www.kernel.org/doc/html/latest/process/coding-style.html | |
14 | ||
7419c83f | 15 | ## 1) General Notes |
4a308d66 CB |
16 | |
17 | - The coding style guide refers to new code. But legacy code can be cleaned up | |
18 | and we are happy to take those patches. | |
19 | - Just because there is still code in LXC that doesn't adhere to the coding | |
20 | standards outlined here does not license not adhering to the coding style. In | |
21 | other words: please stick to the coding style. | |
22 | - Maintainers are free to ignore rules specified here when merging pull | |
23 | requests. This guideline might seem a little weird but it exits to ease new | |
24 | developers into the code base and to prevent unnecessary bikeshedding. If | |
25 | a maintainer feels hat enforcing a specific rule in a given commit would do | |
26 | more harm than good they should always feel free to ignore the rule. | |
27 | ||
28 | Furthermore, when merging pull requests that do not adhere to our coding | |
29 | style maintainers should feel free to grab the commit, adapt it to our coding | |
30 | style and add their Signed-off-by line to it. This is especially helpful to | |
31 | make it easier for first-time contributors and to prevent having pull | |
32 | requests being stuck in the merge queue because of minor details. | |
c67cb619 CB |
33 | - We currently do not provide automatic coding style checks but if a suitable |
34 | tool is found we are happy to integrate it into our test suite. It is | |
35 | possible and recommended to use the `clang-format` binary to check your code. | |
36 | The following options are an approximation of the coding style used here. | |
37 | Simply create a file called `.clang-format` in your home directory with the | |
38 | following options: | |
39 | ```sh | |
40 | cat << EOF > "${HOME}"/.clang-format | |
b629739c | 41 | AlignEscapedNewlines: Left |
c67cb619 CB |
42 | BreakBeforeBraces: Attach |
43 | AlwaysBreakBeforeMultilineStrings: false | |
44 | BreakBeforeBinaryOperators: None | |
45 | MaxEmptyLinesToKeep: 1 | |
46 | PenaltyBreakBeforeFirstCallParameter: 1000000 | |
47 | BinPackArguments: true | |
48 | BinPackParameters: true | |
49 | AllowAllParametersOfDeclarationOnNextLine: false | |
50 | AlignAfterOpenBracket: true | |
51 | SpacesInSquareBrackets: false | |
52 | SpacesInCStyleCastParentheses: false | |
53 | SpaceInEmptyParentheses: false | |
54 | SpaceBeforeParens: ControlStatements | |
55 | SpaceAfterCStyleCast: false | |
56 | SortIncludes: true | |
57 | PenaltyReturnTypeOnItsOwnLine: 10000 | |
58 | PenaltyExcessCharacter: 10 | |
59 | Language: Cpp | |
60 | ForEachMacros: ['lxc_list_for_each', 'lxc_list_for_each_safe'] | |
61 | AllowShortLoopsOnASingleLine: false | |
62 | AllowShortIfStatementsOnASingleLine: false | |
63 | AllowShortFunctionsOnASingleLine: None | |
64 | AllowShortCaseLabelsOnASingleLine: false | |
65 | AllowShortBlocksOnASingleLine: false | |
66 | BasedOnStyle: LLVM | |
67 | TabWidth: 8 | |
68 | IndentWidth: 8 | |
69 | UseTab: Always | |
70 | BreakBeforeBraces: Linux | |
71 | AllowShortIfStatementsOnASingleLine: false | |
72 | IndentCaseLabels: false | |
73 | EOF | |
74 | ``` | |
75 | However, it will not handle all cases correctly. For example, most `struct` | |
76 | initializations will not be correct. In such cases please refer to the coding | |
77 | style here. | |
4a308d66 | 78 | |
7419c83f | 79 | ## 2) Only Use Tabs |
4a308d66 CB |
80 | |
81 | - LXC uses tabs. | |
82 | ||
7419c83f | 83 | ## 3) Only use `/* */` Style Comments |
4a308d66 CB |
84 | |
85 | - Any comments that are added must use `/* */`. | |
86 | - All comments should start on the same line as the opening `/*`. | |
87 | - Single-line comments should simply be placed between `/* */`. For example: | |
b0c407f7 | 88 | ```C |
4a308d66 CB |
89 | /* Define pivot_root() if missing from the C library */ |
90 | ``` | |
91 | - Multi-line comments should end with the closing `*/` on a separate line. For | |
92 | example: | |
b0c407f7 | 93 | ```C |
4a308d66 CB |
94 | /* At this point the old-root is mounted on top of our new-root |
95 | * To unmounted it we must not be chdir()ed into it, so escape back | |
96 | * to old-root. | |
97 | */ | |
98 | ``` | |
99 | ||
7419c83f | 100 | ## 4) Try To Wrap At 80chars |
4a308d66 CB |
101 | |
102 | - This is not strictly enforced. It is perfectly valid to sometimes | |
103 | overflow this limit if it helps clarity. Nonetheless, try to stick to it | |
104 | and use common sense to decide when not to. | |
105 | ||
7419c83f | 106 | ## 5) Error Messages |
4a308d66 CB |
107 | |
108 | - Error messages must start with a capital letter and must **not** end with a | |
109 | punctuation sign. | |
110 | - They should be descriptive, without being needlessly long. It is best to just | |
111 | use already existing error messages as examples. | |
112 | - Examples of acceptable error messages are: | |
b0c407f7 | 113 | ```C |
4a308d66 CB |
114 | SYSERROR("Failed to create directory \"%s\"", path); |
115 | WARN("\"/dev\" directory does not exist. Proceeding without autodev being set up"); | |
116 | ``` | |
117 | ||
7419c83f | 118 | ## 6) Return Error Codes |
4a308d66 CB |
119 | |
120 | - When writing a function that can fail in a non-binary way try to return | |
121 | meaningful negative error codes (e.g. `return -EINVAL;`). | |
122 | ||
7419c83f | 123 | ## 7) All Unexported Functions Must Be Declared `static` |
4a308d66 CB |
124 | |
125 | - Functions which are only used in the current file and are not exported | |
126 | within the codebase need to be declared with the `static` attribute. | |
127 | ||
7419c83f | 128 | ## 8) All Exported Functions Must Be Declared `extern` In A Header File |
4a308d66 | 129 | |
39a1e1fa TH |
130 | - Functions declared in header files (`*.h`) should use the `extern` keyword. |
131 | - Functions declared in source files (`*.c`) should not use the `extern` keyword. | |
4a308d66 | 132 | |
7419c83f | 133 | ## 9) Declaring Variables |
4a308d66 CB |
134 | |
135 | - variables should be declared at the top of the function or at the beginning | |
136 | of a new scope but **never** in the middle of a scope | |
137 | 1. uninitialized variables | |
138 | - put base types before complex types | |
139 | - put standard types defined by libc before types defined by LXC | |
140 | - put multiple declarations of the same type on the same line | |
141 | 2. initialized variables | |
142 | - put base types before complex types | |
143 | - put standard types defined by libc before types defined by LXC | |
144 | - put multiple declarations of the same type on the same line | |
145 | - Examples of good declarations can be seen in the following function: | |
b0c407f7 | 146 | ```C |
4a308d66 CB |
147 | int lxc_clear_procs(struct lxc_conf *c, const char *key) |
148 | { | |
149 | struct lxc_list *it, *next; | |
150 | bool all = false; | |
151 | const char *k = NULL; | |
152 | ||
153 | if (strcmp(key, "lxc.proc") == 0) | |
154 | all = true; | |
155 | else if (strncmp(key, "lxc.proc.", sizeof("lxc.proc.") - 1) == 0) | |
156 | k = key + sizeof("lxc.proc.") - 1; | |
157 | else | |
158 | return -1; | |
159 | ||
160 | lxc_list_for_each_safe(it, &c->procs, next) { | |
161 | struct lxc_proc *proc = it->elem; | |
162 | ||
163 | if (!all && strcmp(proc->filename, k) != 0) | |
164 | continue; | |
165 | lxc_list_del(it); | |
166 | free(proc->filename); | |
167 | free(proc->value); | |
168 | free(proc); | |
169 | free(it); | |
170 | } | |
171 | ||
172 | return 0; | |
173 | } | |
174 | ``` | |
175 | ||
7419c83f | 176 | ## 10) Functions Not Returning Booleans Must Assign Return Value Before Performing Checks |
4a308d66 CB |
177 | |
178 | - When checking whether a function not returning booleans was successful or not | |
179 | the returned value must be assigned before it is checked (`str{n}cmp()` | |
180 | functions being one notable exception). For example: | |
b0c407f7 | 181 | ```C |
4a308d66 CB |
182 | /* assign value to "ret" first */ |
183 | ret = mount(sourcepath, cgpath, "cgroup", remount_flags, NULL); | |
184 | /* check whether function was successful */ | |
185 | if (ret < 0) { | |
186 | SYSERROR("Failed to remount \"%s\" ro", cgpath); | |
187 | free(sourcepath); | |
188 | return -1; | |
189 | } | |
190 | ``` | |
191 | Functions returning booleans can be checked directly. For example: | |
b0c407f7 | 192 | ```C |
4a308d66 CB |
193 | extern bool lxc_string_in_array(const char *needle, const char **haystack); |
194 | ||
195 | /* check right away */ | |
196 | if (lxc_string_in_array("ns", (const char **)h->subsystems)) | |
197 | continue; | |
198 | ``` | |
199 | ||
7419c83f | 200 | ## 11) Non-Boolean Functions That Behave Like Boolean Functions Must Explicitly Check Against A Value |
ed06b69c CB |
201 | |
202 | - This rule mainly exists for `str{n}cmp()` type functions. In most cases they | |
203 | are used like a boolean function to check whether a string matches or not. | |
204 | But they return an integer. It is perfectly fine to check `str{n}cmp()` | |
205 | functions directly but you must compare explicitly against a value. That is | |
206 | to say, while they are conceptually boolean functions they shouldn't be | |
207 | treated as such since they don't really behave like boolean functions. So | |
208 | `if (!str{n}cmp())` and `if (str{n}cmp())` checks must not be used. Good | |
209 | examples are found in the following functions: | |
b0c407f7 | 210 | ```C |
ed06b69c CB |
211 | static int set_config_hooks(const char *key, const char *value, |
212 | struct lxc_conf *lxc_conf, void *data) | |
213 | ||
214 | char *copy; | |
215 | ||
216 | if (lxc_config_value_empty(value)) | |
217 | return lxc_clear_hooks(lxc_conf, key); | |
218 | ||
219 | if (strcmp(key + 4, "hook") == 0) { | |
220 | ERROR("lxc.hook must not have a value"); | |
221 | return -1; | |
222 | } | |
223 | ||
224 | copy = strdup(value); | |
225 | if (!copy) | |
226 | return -1; | |
227 | ||
228 | if (strcmp(key + 9, "pre-start") == 0) | |
229 | return add_hook(lxc_conf, LXCHOOK_PRESTART, copy); | |
230 | else if (strcmp(key + 9, "start-host") == 0) | |
231 | return add_hook(lxc_conf, LXCHOOK_START_HOST, copy); | |
232 | else if (strcmp(key + 9, "pre-mount") == 0) | |
233 | return add_hook(lxc_conf, LXCHOOK_PREMOUNT, copy); | |
234 | else if (strcmp(key + 9, "autodev") == 0) | |
235 | return add_hook(lxc_conf, LXCHOOK_AUTODEV, copy); | |
236 | else if (strcmp(key + 9, "mount") == 0) | |
237 | return add_hook(lxc_conf, LXCHOOK_MOUNT, copy); | |
238 | else if (strcmp(key + 9, "start") == 0) | |
239 | return add_hook(lxc_conf, LXCHOOK_START, copy); | |
240 | else if (strcmp(key + 9, "stop") == 0) | |
241 | return add_hook(lxc_conf, LXCHOOK_STOP, copy); | |
242 | else if (strcmp(key + 9, "post-stop") == 0) | |
243 | return add_hook(lxc_conf, LXCHOOK_POSTSTOP, copy); | |
244 | else if (strcmp(key + 9, "clone") == 0) | |
245 | return add_hook(lxc_conf, LXCHOOK_CLONE, copy); | |
246 | else if (strcmp(key + 9, "destroy") == 0) | |
247 | return add_hook(lxc_conf, LXCHOOK_DESTROY, copy); | |
248 | ||
249 | free(copy); | |
250 | return -1; | |
251 | } | |
252 | ``` | |
253 | ||
7419c83f | 254 | ## 12) Do Not Use C99 Variable Length Arrays (VLA) |
4a308d66 CB |
255 | |
256 | - They are made optional and there is no guarantee that future C standards | |
257 | will support them. | |
258 | ||
7419c83f | 259 | ## 13) Use Standard libc Macros When Exiting |
4a308d66 CB |
260 | |
261 | - libc provides `EXIT_FAILURE` and `EXIT_SUCCESS`. Use them whenever possible | |
262 | in the child of `fork()`ed process or when exiting from a `main()` function. | |
263 | ||
7419c83f | 264 | ## 14) Use `goto`s |
4a308d66 CB |
265 | |
266 | `goto`s are an essential language construct of C and are perfect to perform | |
267 | cleanup operations or simplify the logic of functions. However, here are the | |
268 | rules to use them: | |
269 | - use descriptive `goto` labels. | |
270 | For example, if you know that this label is only used as an error path you | |
271 | should use something like `on_error` instead of `out` as label name. | |
272 | - **only** jump downwards unless you are handling `EAGAIN` errors and want to | |
273 | avoid `do-while` constructs. | |
274 | - An example of a good usage of `goto` is: | |
b0c407f7 | 275 | ```C |
4a308d66 CB |
276 | static int set_config_idmaps(const char *key, const char *value, |
277 | struct lxc_conf *lxc_conf, void *data) | |
278 | { | |
279 | unsigned long hostid, nsid, range; | |
280 | char type; | |
281 | int ret; | |
282 | struct lxc_list *idmaplist = NULL; | |
283 | struct id_map *idmap = NULL; | |
284 | ||
285 | if (lxc_config_value_empty(value)) | |
286 | return lxc_clear_idmaps(lxc_conf); | |
287 | ||
288 | idmaplist = malloc(sizeof(*idmaplist)); | |
289 | if (!idmaplist) | |
290 | goto on_error; | |
291 | ||
292 | idmap = malloc(sizeof(*idmap)); | |
293 | if (!idmap) | |
294 | goto on_error; | |
295 | memset(idmap, 0, sizeof(*idmap)); | |
296 | ||
297 | ret = parse_idmaps(value, &type, &nsid, &hostid, &range); | |
298 | if (ret < 0) { | |
299 | ERROR("Failed to parse id mappings"); | |
300 | goto on_error; | |
301 | } | |
302 | ||
303 | INFO("Read uid map: type %c nsid %lu hostid %lu range %lu", type, nsid, hostid, range); | |
304 | if (type == 'u') | |
305 | idmap->idtype = ID_TYPE_UID; | |
306 | else if (type == 'g') | |
307 | idmap->idtype = ID_TYPE_GID; | |
308 | else | |
309 | goto on_error; | |
310 | ||
311 | idmap->hostid = hostid; | |
312 | idmap->nsid = nsid; | |
313 | idmap->range = range; | |
314 | idmaplist->elem = idmap; | |
315 | lxc_list_add_tail(&lxc_conf->id_map, idmaplist); | |
316 | ||
317 | if (!lxc_conf->root_nsuid_map && idmap->idtype == ID_TYPE_UID) | |
318 | if (idmap->nsid == 0) | |
319 | lxc_conf->root_nsuid_map = idmap; | |
320 | ||
321 | ||
322 | if (!lxc_conf->root_nsgid_map && idmap->idtype == ID_TYPE_GID) | |
323 | if (idmap->nsid == 0) | |
324 | lxc_conf->root_nsgid_map = idmap; | |
325 | ||
326 | idmap = NULL; | |
327 | ||
328 | return 0; | |
329 | ||
330 | on_error: | |
331 | free(idmaplist); | |
332 | free(idmap); | |
333 | ||
334 | return -1; | |
335 | } | |
336 | ``` | |
337 | ||
7419c83f | 338 | ## 15) Use Booleans instead of integers |
4a308d66 CB |
339 | |
340 | - When something can be conceptualized in a binary way use a boolean not | |
341 | an integer. | |
342 | ||
7419c83f | 343 | ## 16) Cleanup Functions Must Handle The Object's Null Type And Being Passed Already Cleaned Up Objects |
4a308d66 CB |
344 | |
345 | - If you implement a custom cleanup function to e.g. free a complex type | |
346 | you declared you must ensure that the object's null type is handled and | |
347 | treated as a NOOP. For example: | |
b0c407f7 | 348 | ```C |
4a308d66 CB |
349 | void lxc_free_array(void **array, lxc_free_fn element_free_fn) |
350 | { | |
351 | void **p; | |
352 | for (p = array; p && *p; p++) | |
353 | element_free_fn(*p); | |
354 | free((void*)array); | |
355 | } | |
356 | ``` | |
357 | - Cleanup functions should also expect to be passed already cleaned up objects. | |
358 | One way to handle this cleanly is to initialize the cleaned up variable to | |
359 | a special value that signals the function that the element has already been | |
360 | freed on the next call. For example, the following function cleans up file | |
361 | descriptors and sets the already closed file descriptors to `-EBADF`. On the | |
362 | next call it can simply check whether the file descriptor is positive and | |
363 | move on if it isn't: | |
b0c407f7 | 364 | ```C |
4a308d66 CB |
365 | static void lxc_put_attach_clone_payload(struct attach_clone_payload *p) |
366 | { | |
367 | if (p->ipc_socket >= 0) { | |
368 | shutdown(p->ipc_socket, SHUT_RDWR); | |
369 | close(p->ipc_socket); | |
370 | p->ipc_socket = -EBADF; | |
371 | } | |
372 | ||
373 | if (p->pty_fd >= 0) { | |
374 | close(p->pty_fd); | |
375 | p->pty_fd = -EBADF; | |
376 | } | |
377 | ||
378 | if (p->init_ctx) { | |
379 | lxc_proc_put_context_info(p->init_ctx); | |
380 | p->init_ctx = NULL; | |
381 | } | |
382 | } | |
383 | ``` | |
384 | ||
7419c83f | 385 | ## 17) Cast to `(void)` When Intentionally Ignoring Return Values |
4a308d66 CB |
386 | |
387 | - There are cases where you do not care about the return value of a function. | |
388 | Please cast the return value to `(void)` when doing so. | |
389 | - Standard library functions or functions which are known to be ignored by | |
390 | default do not need to be cast to `(void)`. Classical candidates are | |
391 | `close()` and `fclose()`. | |
392 | - A good example is: | |
b0c407f7 | 393 | ```C |
4a308d66 CB |
394 | for (i = 0; hierarchies[i]; i++) { |
395 | char *fullpath; | |
396 | char *path = hierarchies[i]->fullcgpath; | |
397 | ||
398 | ret = chowmod(path, destuid, nsgid, 0755); | |
399 | if (ret < 0) | |
400 | return -1; | |
401 | ||
402 | /* failures to chown() these are inconvenient but not | |
403 | * detrimental we leave these owned by the container launcher, | |
404 | * so that container root can write to the files to attach. we | |
405 | * chmod() them 664 so that container systemd can write to the | |
406 | * files (which systemd in wily insists on doing). | |
407 | */ | |
408 | ||
409 | if (hierarchies[i]->version == cgroup_super_magic) { | |
410 | fullpath = must_make_path(path, "tasks", null); | |
411 | (void)chowmod(fullpath, destuid, nsgid, 0664); | |
412 | free(fullpath); | |
413 | } | |
414 | ||
415 | fullpath = must_make_path(path, "cgroup.procs", null); | |
416 | (void)chowmod(fullpath, destuid, 0, 0664); | |
417 | free(fullpath); | |
418 | ||
419 | if (hierarchies[i]->version != cgroup2_super_magic) | |
420 | continue; | |
421 | ||
422 | fullpath = must_make_path(path, "cgroup.subtree_control", null); | |
423 | (void)chowmod(fullpath, destuid, nsgid, 0664); | |
424 | free(fullpath); | |
425 | ||
426 | fullpath = must_make_path(path, "cgroup.threads", null); | |
427 | (void)chowmod(fullpath, destuid, nsgid, 0664); | |
428 | free(fullpath); | |
429 | } | |
430 | ``` | |
431 | ||
7419c83f | 432 | ## 18) Use `for (;;)` instead of `while (1)` or `while (true)` |
4a308d66 CB |
433 | |
434 | - Let's be honest, it is really the only sensible way to do this. | |
435 | ||
7419c83f | 436 | ## 19) Use The Set Of Supported DCO Statements |
4a308d66 CB |
437 | |
438 | - Signed-off-by: Random J Developer <random@developer.org> | |
439 | - You did write this code or have the right to contribute it to LXC. | |
440 | - Acked-by: Random J Developer <random@developer.org> | |
441 | - You did read the code and think it is correct. This is usually only used by | |
442 | maintainers or developers that have made significant contributions and can | |
443 | vouch for the correctness of someone else's code. | |
444 | - Reviewed-by: Random J Developer <random@developer.org> | |
445 | - You did review the code and vouch for its correctness, i.e. you'd be | |
446 | prepared to fix bugs it might cause. This is usually only used by | |
447 | maintainers or developers that have made significant contributions and can | |
448 | vouch for the correctness of someone else's code. | |
449 | - Co-developed-by: Random J Developer <random@developer.org> | |
450 | - The code can not be reasonably attributed to a single developer, i.e. | |
451 | you worked on this together. | |
452 | - Tested-by: Random J Developer <random@developer.org> | |
453 | - You verified that the code fixes a given bug or is behaving as advertised. | |
454 | - Reported-by: Random J Developer <random@developer.org> | |
455 | - You found and reported the bug. | |
456 | - Suggested-by: Random J Developer <random@developer.org> | |
457 | - You wrote the code but someone contributed the idea. This line is usually | |
458 | overlooked but it is a sign of good etiquette and coding ethics: if someone | |
459 | helped you solve a problem or had a clever idea do not silently claim it by | |
460 | slapping your Signed-off-by underneath. Be honest and add a Suggested-by. | |
461 | ||
7419c83f | 462 | ## 20) Commit Message Outline |
4a308d66 CB |
463 | |
464 | - You **must** stick to the 80chars limit especially in the title of the commit | |
465 | message. | |
466 | - Please use English commit messages only. | |
467 | - use meaningful commit messages. | |
468 | - Use correct spelling and grammar. | |
469 | If you are not a native speaker and/or feel yourself struggling with this it | |
470 | is perfectly fine to point this out and there's no need to apologize. Usually | |
471 | developers will be happy to pull your branch and adopt the commit message. | |
472 | - Please always use the affected file (without the file type suffix) or module | |
473 | as a prefix in the commit message. | |
474 | - Examples of good commit messages are: | |
b0c407f7 | 475 | ```Diff |
4a308d66 CB |
476 | commit b87243830e3b5e95fa31a17cf1bfebe55353bf13 |
477 | Author: Felix Abecassis <fabecassis@nvidia.com> | |
478 | Date: Fri Feb 2 06:19:13 2018 -0800 | |
479 | ||
480 | hooks: change the semantic of NVIDIA_VISIBLE_DEVICES="" | |
481 | ||
482 | With LXC, you can override the value of an environment variable to | |
483 | null, but you can't unset an existing variable. | |
484 | ||
485 | The NVIDIA hook was previously activated when NVIDIA_VISIBLE_DEVICES | |
486 | was set to null. As a result, it was not possible to disable the hook | |
487 | by overriding the environment variable in the configuration. | |
488 | ||
489 | The hook can now be disabled by setting NVIDIA_VISIBLE_DEVICES to | |
490 | null or to the new special value "void". | |
491 | ||
492 | Signed-off-by: Felix Abecassis <fabecassis@nvidia.com> | |
493 | ||
494 | ||
495 | commit d6337a5f9dc7311af168aa3d586fdf239f5a10d3 | |
496 | Author: Christian Brauner <christian.brauner@ubuntu.com> | |
497 | Date: Wed Jan 31 16:25:11 2018 +0100 | |
498 | ||
499 | cgroups: get controllers on the unified hierarchy | |
500 | ||
501 | Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> | |
502 | ||
503 | ``` | |
7419c83f | 504 | ## 21) Use `_exit()` To Terminate `fork()`ed Child Processes |
4ad78f87 CB |
505 | |
506 | - When `fork()`ing off a child process use `_exit()` to terminate it instead of | |
507 | `exit()`. The `exit()` function is not thread-safe and thus not suited for | |
508 | the shared library which must ensure that it is thread-safe. | |
b629739c | 509 | |
7419c83f | 510 | ## 22) Keep Arrays of `struct`s Aligned Horizontally When Initializing |
b629739c CB |
511 | |
512 | - Arrays of `struct`s are: | |
b0c407f7 | 513 | ```C |
b629739c CB |
514 | struct foo_struct { |
515 | int n; | |
516 | int m; | |
517 | int p; | |
518 | }; | |
519 | ||
520 | struct foo_struct new_instance[] = { | |
521 | { 1, 2, 3 }, | |
522 | { 4, 5, 6 }, | |
523 | { 7, 8, 9 }, | |
524 | }; | |
525 | ``` | |
526 | - Leave a single space after the opening `{` and before closing `}` of the | |
527 | largest member of the last column. | |
528 | - Always leave a single space between the largest member of the current column | |
529 | and the member in the next column. | |
530 | - A good example is | |
b0c407f7 | 531 | ```C |
b629739c CB |
532 | struct signame { |
533 | int num; | |
534 | const char *name; | |
535 | }; | |
536 | ||
537 | static const struct signame signames[] = { | |
538 | { SIGHUP, "HUP" }, | |
539 | { SIGINT, "INT" }, | |
540 | { SIGQUIT, "QUIT" }, | |
541 | { SIGILL, "ILL" }, | |
542 | { SIGABRT, "ABRT" }, | |
543 | { SIGFPE, "FPE" }, | |
544 | { SIGKILL, "KILL" }, | |
545 | { SIGSEGV, "SEGV" }, | |
546 | { SIGPIPE, "PIPE" }, | |
547 | { SIGALRM, "ALRM" }, | |
548 | { SIGTERM, "TERM" }, | |
549 | { SIGUSR1, "USR1" }, | |
550 | { SIGUSR2, "USR2" }, | |
551 | { SIGCHLD, "CHLD" }, | |
552 | { SIGCONT, "CONT" }, | |
553 | { SIGSTOP, "STOP" }, | |
554 | { SIGTSTP, "TSTP" }, | |
555 | { SIGTTIN, "TTIN" }, | |
556 | { SIGTTOU, "TTOU" }, | |
557 | #ifdef SIGTRAP | |
558 | { SIGTRAP, "TRAP" }, | |
559 | #endif | |
560 | #ifdef SIGIOT | |
561 | { SIGIOT, "IOT" }, | |
562 | #endif | |
563 | #ifdef SIGEMT | |
564 | { SIGEMT, "EMT" }, | |
565 | #endif | |
566 | #ifdef SIGBUS | |
567 | { SIGBUS, "BUS" }, | |
568 | #endif | |
569 | #ifdef SIGSTKFLT | |
570 | { SIGSTKFLT, "STKFLT" }, | |
571 | #endif | |
572 | #ifdef SIGCLD | |
573 | { SIGCLD, "CLD" }, | |
574 | #endif | |
575 | #ifdef SIGURG | |
576 | { SIGURG, "URG" }, | |
577 | #endif | |
578 | #ifdef SIGXCPU | |
579 | { SIGXCPU, "XCPU" }, | |
580 | #endif | |
581 | #ifdef SIGXFSZ | |
582 | { SIGXFSZ, "XFSZ" }, | |
583 | #endif | |
584 | #ifdef SIGVTALRM | |
585 | { SIGVTALRM, "VTALRM" }, | |
586 | #endif | |
587 | #ifdef SIGPROF | |
588 | { SIGPROF, "PROF" }, | |
589 | #endif | |
590 | #ifdef SIGWINCH | |
591 | { SIGWINCH, "WINCH" }, | |
592 | #endif | |
593 | #ifdef SIGIO | |
594 | { SIGIO, "IO" }, | |
595 | #endif | |
596 | #ifdef SIGPOLL | |
597 | { SIGPOLL, "POLL" }, | |
598 | #endif | |
599 | #ifdef SIGINFO | |
600 | { SIGINFO, "INFO" }, | |
601 | #endif | |
602 | #ifdef SIGLOST | |
603 | { SIGLOST, "LOST" }, | |
604 | #endif | |
605 | #ifdef SIGPWR | |
606 | { SIGPWR, "PWR" }, | |
607 | #endif | |
608 | #ifdef SIGUNUSED | |
609 | { SIGUNUSED, "UNUSED" }, | |
610 | #endif | |
611 | #ifdef SIGSYS | |
612 | { SIGSYS, "SYS" }, | |
613 | #endif | |
614 | }; | |
615 | ``` | |
a3759c1b | 616 | |
7419c83f | 617 | ## 23) Use `strlcpy()` instead of `strncpy()` |
a3759c1b CB |
618 | |
619 | When copying strings always use `strlcpy()` instead of `strncpy()`. The | |
620 | advantage of `strlcpy()` is that it will always append a `\0` byte to the | |
621 | string. | |
622 | ||
623 | Unless you have a valid reason to accept truncation you must check whether | |
624 | truncation has occurred, treat it as an error, and handle the error | |
625 | appropriately. | |
77f6262f | 626 | |
7419c83f | 627 | ## 24) Use `strlcat()` instead of `strncat()` |
77f6262f CB |
628 | |
629 | When concatenating strings always use `strlcat()` instead of `strncat()`. The | |
630 | advantage of `strlcat()` is that it will always append a `\0` byte to the | |
631 | string. | |
632 | ||
633 | Unless you have a valid reason to accept truncation you must check whether | |
634 | truncation has occurred, treat it as an error, and handle the error | |
635 | appropriately. | |
cf0fd972 CB |
636 | |
637 | ## 25) Use `__fallthrough__` in switch statements | |
638 | ||
639 | If LXC detects that the compiler is new enough it will tell it to check | |
640 | `switch` statements for non-documented fallthroughs. Please always place | |
641 | a `__fallthrough__` after a `case` which falls through the next one. | |
642 | ||
643 | ```c | |
644 | int lxc_attach_run_command(void *payload) | |
645 | { | |
646 | int ret = -1; | |
647 | lxc_attach_command_t *cmd = payload; | |
648 | ||
649 | ret = execvp(cmd->program, cmd->argv); | |
650 | if (ret < 0) { | |
651 | switch (errno) { | |
652 | case ENOEXEC: | |
653 | ret = 126; | |
654 | break; | |
655 | case ENOENT: | |
656 | ret = 127; | |
657 | break; | |
658 | } | |
659 | } | |
660 | ||
661 | SYSERROR("Failed to exec \"%s\"", cmd->program); | |
662 | return ret; | |
663 | } | |
664 | ``` | |
af32408a CB |
665 | |
666 | ## 24) Never use `fgets()` | |
667 | ||
668 | LXC does not allow the use of `fgets()`. Use `getline()` or other methods | |
669 | instead. | |
670 | ||
671 | ## 25) Never allocate memory on the stack | |
672 | ||
673 | This specifically forbids any usage of `alloca()` in the codebase. | |
674 | ||
675 | ## 26) Use cleanup macros supported by `gcc` and `clang` | |
676 | ||
677 | LXC has switched from manually cleaning up resources to using cleanup macros | |
678 | supported by `gcc` and `clang`: | |
679 | ```c | |
680 | __attribute__((__cleanup__(<my-cleanup-function-wrapper>))) | |
681 | ``` | |
682 | We do not allow manually cleanups anymore if there are appropriate macros. | |
683 | Currently the following macros are supported: | |
684 | ```c | |
685 | /* close file descriptor */ | |
686 | __do_close_prot_errno | |
687 | ||
688 | /* free allocated memory */ | |
689 | __do_free __attribute__((__cleanup__(__auto_free__))) | |
690 | ||
691 | /* close FILEs */ | |
692 | __do_fclose __attribute__((__cleanup__(__auto_fclose__))) | |
693 | ||
694 | /* close DIRs */ | |
695 | __do_closedir __attribute__((__cleanup__(__auto_closedir__))) | |
696 | ``` | |
697 | For example: | |
698 | ```c | |
699 | void remount_all_slave(void) | |
700 | { | |
701 | __do_free char *line = NULL; | |
702 | __do_fclose FILE *f = NULL; | |
703 | __do_close_prot_errno int memfd = -EBADF, mntinfo_fd = -EBADF; | |
704 | int ret; | |
705 | ssize_t copied; | |
706 | size_t len = 0; | |
707 | ||
708 | mntinfo_fd = open("/proc/self/mountinfo", O_RDONLY | O_CLOEXEC); | |
709 | if (mntinfo_fd < 0) { | |
710 | SYSERROR("Failed to open \"/proc/self/mountinfo\""); | |
711 | return; | |
712 | } | |
713 | ||
714 | memfd = memfd_create(".lxc_mountinfo", MFD_CLOEXEC); | |
715 | if (memfd < 0) { | |
716 | char template[] = P_tmpdir "/.lxc_mountinfo_XXXXXX"; | |
717 | ||
718 | if (errno != ENOSYS) { | |
719 | SYSERROR("Failed to create temporary in-memory file"); | |
720 | return; | |
721 | } | |
722 | ||
723 | memfd = lxc_make_tmpfile(template, true); | |
724 | if (memfd < 0) { | |
725 | WARN("Failed to create temporary file"); | |
726 | return; | |
727 | } | |
728 | } | |
729 | ||
730 | again: | |
731 | copied = lxc_sendfile_nointr(memfd, mntinfo_fd, NULL, LXC_SENDFILE_MAX); | |
732 | if (copied < 0) { | |
733 | if (errno == EINTR) | |
734 | goto again; | |
735 | ||
736 | SYSERROR("Failed to copy \"/proc/self/mountinfo\""); | |
737 | return; | |
738 | } | |
739 | ||
740 | ret = lseek(memfd, 0, SEEK_SET); | |
741 | if (ret < 0) { | |
742 | SYSERROR("Failed to reset file descriptor offset"); | |
743 | return; | |
744 | } | |
745 | ||
746 | f = fdopen(memfd, "r"); | |
747 | if (!f) { | |
748 | SYSERROR("Failed to open copy of \"/proc/self/mountinfo\" to mark all shared. Continuing"); | |
749 | return; | |
750 | } | |
751 | ||
752 | /* | |
753 | * After a successful fdopen() memfd will be closed when calling | |
754 | * fclose(f). Calling close(memfd) afterwards is undefined. | |
755 | */ | |
756 | move_fd(memfd); | |
757 | ||
758 | while (getline(&line, &len, f) != -1) { | |
759 | char *opts, *target; | |
760 | ||
761 | target = get_field(line, 4); | |
762 | if (!target) | |
763 | continue; | |
764 | ||
765 | opts = get_field(target, 2); | |
766 | if (!opts) | |
767 | continue; | |
768 | ||
769 | null_endofword(opts); | |
770 | if (!strstr(opts, "shared")) | |
771 | continue; | |
772 | ||
773 | null_endofword(target); | |
774 | ret = mount(NULL, target, NULL, MS_SLAVE, NULL); | |
775 | if (ret < 0) { | |
776 | SYSERROR("Failed to make \"%s\" MS_SLAVE", target); | |
777 | ERROR("Continuing..."); | |
778 | continue; | |
779 | } | |
780 | TRACE("Remounted \"%s\" as MS_SLAVE", target); | |
781 | } | |
782 | TRACE("Remounted all mount table entries as MS_SLAVE"); | |
783 | } | |
784 | ``` |