]>
Commit | Line | Data |
---|---|---|
4a308d66 CB |
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 | ||
ed06b69c | 180 | #### Functions Not Returning Booleans Must Assign Return Value Before Performing Checks |
4a308d66 CB |
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 | ||
ed06b69c CB |
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 | ||
4a308d66 CB |
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 | ||
4a308d66 CB |
436 | #### Use `for (;;)` instead of `while (1)` or `while (true)` |
437 | ||
438 | - Let's be honest, it is really the only sensible way to do this. | |
439 | ||
440 | #### Use The Set Of Supported DCO Statements | |
441 | ||
442 | - Signed-off-by: Random J Developer <random@developer.org> | |
443 | - You did write this code or have the right to contribute it to LXC. | |
444 | - Acked-by: Random J Developer <random@developer.org> | |
445 | - You did read the code and think it is correct. This is usually only used by | |
446 | maintainers or developers that have made significant contributions and can | |
447 | vouch for the correctness of someone else's code. | |
448 | - Reviewed-by: Random J Developer <random@developer.org> | |
449 | - You did review the code and vouch for its correctness, i.e. you'd be | |
450 | prepared to fix bugs it might cause. This is usually only used by | |
451 | maintainers or developers that have made significant contributions and can | |
452 | vouch for the correctness of someone else's code. | |
453 | - Co-developed-by: Random J Developer <random@developer.org> | |
454 | - The code can not be reasonably attributed to a single developer, i.e. | |
455 | you worked on this together. | |
456 | - Tested-by: Random J Developer <random@developer.org> | |
457 | - You verified that the code fixes a given bug or is behaving as advertised. | |
458 | - Reported-by: Random J Developer <random@developer.org> | |
459 | - You found and reported the bug. | |
460 | - Suggested-by: Random J Developer <random@developer.org> | |
461 | - You wrote the code but someone contributed the idea. This line is usually | |
462 | overlooked but it is a sign of good etiquette and coding ethics: if someone | |
463 | helped you solve a problem or had a clever idea do not silently claim it by | |
464 | slapping your Signed-off-by underneath. Be honest and add a Suggested-by. | |
465 | ||
466 | #### Commit Message Outline | |
467 | ||
468 | - You **must** stick to the 80chars limit especially in the title of the commit | |
469 | message. | |
470 | - Please use English commit messages only. | |
471 | - use meaningful commit messages. | |
472 | - Use correct spelling and grammar. | |
473 | If you are not a native speaker and/or feel yourself struggling with this it | |
474 | is perfectly fine to point this out and there's no need to apologize. Usually | |
475 | developers will be happy to pull your branch and adopt the commit message. | |
476 | - Please always use the affected file (without the file type suffix) or module | |
477 | as a prefix in the commit message. | |
478 | - Examples of good commit messages are: | |
479 | ``` | |
480 | commit b87243830e3b5e95fa31a17cf1bfebe55353bf13 | |
481 | Author: Felix Abecassis <fabecassis@nvidia.com> | |
482 | Date: Fri Feb 2 06:19:13 2018 -0800 | |
483 | ||
484 | hooks: change the semantic of NVIDIA_VISIBLE_DEVICES="" | |
485 | ||
486 | With LXC, you can override the value of an environment variable to | |
487 | null, but you can't unset an existing variable. | |
488 | ||
489 | The NVIDIA hook was previously activated when NVIDIA_VISIBLE_DEVICES | |
490 | was set to null. As a result, it was not possible to disable the hook | |
491 | by overriding the environment variable in the configuration. | |
492 | ||
493 | The hook can now be disabled by setting NVIDIA_VISIBLE_DEVICES to | |
494 | null or to the new special value "void". | |
495 | ||
496 | Signed-off-by: Felix Abecassis <fabecassis@nvidia.com> | |
497 | ||
498 | ||
499 | commit d6337a5f9dc7311af168aa3d586fdf239f5a10d3 | |
500 | Author: Christian Brauner <christian.brauner@ubuntu.com> | |
501 | Date: Wed Jan 31 16:25:11 2018 +0100 | |
502 | ||
503 | cgroups: get controllers on the unified hierarchy | |
504 | ||
505 | Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> | |
506 | ||
507 | ``` | |
4ad78f87 CB |
508 | #### Use `_exit()` To Terminate `fork()`ed Child Processes |
509 | ||
510 | - When `fork()`ing off a child process use `_exit()` to terminate it instead of | |
511 | `exit()`. The `exit()` function is not thread-safe and thus not suited for | |
512 | the shared library which must ensure that it is thread-safe. |