]>
Commit | Line | Data |
---|---|---|
798d3a52 ZJS |
1 | - 8ch indent, no tabs, except for files in man/ which are 2ch indent, |
2 | and still no tabs | |
60918275 | 3 | |
2708526c LP |
4 | - We prefer /* comments */ over // comments, please. This is not C++, after |
5 | all. (Yes we know that C99 supports both kinds of comments, but still, | |
6 | please!) | |
7 | ||
3fdbc820 LP |
8 | - Don't break code lines too eagerly. We do *not* force line breaks at |
9 | 80ch, all of today's screens should be much larger than that. But | |
1c4e4227 | 10 | then again, don't overdo it, ~119ch should be enough really. |
3fdbc820 | 11 | |
c170f3a4 | 12 | - Variables and functions *must* be static, unless they have a |
f168c273 | 13 | prototype, and are supposed to be exported. |
60918275 | 14 | |
d3a48513 LP |
15 | - structs in MixedCase (with exceptions, such as public API structs), |
16 | variables + functions in lower_case. | |
c170f3a4 LP |
17 | |
18 | - The destructors always unregister the object from the next bigger | |
60918275 LP |
19 | object, not the other way around |
20 | ||
8d0e0ddd | 21 | - To minimize strict aliasing violations, we prefer unions over casting |
60918275 | 22 | |
8d0e0ddd | 23 | - For robustness reasons, destructors should be able to destruct |
60918275 LP |
24 | half-initialized objects, too |
25 | ||
61f33134 | 26 | - Error codes are returned as negative Exxx. e.g. return -EINVAL. There |
8d0e0ddd JE |
27 | are some exceptions: for constructors, it is OK to return NULL on |
28 | OOM. For lookup functions, NULL is fine too for "not found". | |
c170f3a4 LP |
29 | |
30 | Be strict with this. When you write a function that can fail due to | |
31 | more than one cause, it *really* should have "int" as return value | |
32 | for the error code. | |
33 | ||
8e5edf8d | 34 | - Do not bother with error checking whether writing to stdout/stderr |
d3a48513 | 35 | worked. |
c170f3a4 LP |
36 | |
37 | - Do not log errors from "library" code, only do so from "main | |
8e5edf8d | 38 | program" code. (With one exception: it is OK to log with DEBUG level |
d3a48513 | 39 | from any code, with the exception of maybe inner loops). |
c170f3a4 | 40 | |
8e5edf8d | 41 | - Always check OOM. There is no excuse. In program code, you can use |
d3a48513 | 42 | "log_oom()" for then printing a short message, but not in "library" code. |
debf93a4 LP |
43 | |
44 | - Do not issue NSS requests (that includes user name and host name | |
d3a48513 LP |
45 | lookups) from PID 1 as this might trigger deadlocks when those |
46 | lookups involve synchronously talking to services that we would need | |
47 | to start up | |
debf93a4 | 48 | |
8e5edf8d | 49 | - Do not synchronously talk to any other service from PID 1, due to |
d3a48513 | 50 | risk of deadlocks |
c170f3a4 | 51 | |
45df8656 | 52 | - Avoid fixed-size string buffers, unless you really know the maximum |
c170f3a4 | 53 | size and that maximum size is small. They are a source of errors, |
45df8656 JE |
54 | since they possibly result in truncated strings. It is often nicer |
55 | to use dynamic memory, alloca() or VLAs. If you do allocate fixed-size | |
8e5edf8d | 56 | strings on the stack, then it is probably only OK if you either |
d3a48513 LP |
57 | use a maximum size such as LINE_MAX, or count in detail the maximum |
58 | size a string can have. (DECIMAL_STR_MAX and DECIMAL_STR_WIDTH | |
59 | macros are your friends for this!) | |
60 | ||
61 | Or in other words, if you use "char buf[256]" then you are likely | |
62 | doing something wrong! | |
c170f3a4 LP |
63 | |
64 | - Stay uniform. For example, always use "usec_t" for time | |
61f33134 | 65 | values. Do not mix usec and msec, and usec and whatnot. |
c170f3a4 LP |
66 | |
67 | - Make use of _cleanup_free_ and friends. It makes your code much | |
68 | nicer to read! | |
69 | ||
70 | - Be exceptionally careful when formatting and parsing floating point | |
71 | numbers. Their syntax is locale dependent (i.e. "5.000" in en_US is | |
72 | generally understood as 5, while on de_DE as 5000.). | |
73 | ||
74 | - Try to use this: | |
75 | ||
76 | void foo() { | |
77 | } | |
78 | ||
79 | instead of this: | |
80 | ||
81 | void foo() | |
82 | { | |
83 | } | |
84 | ||
8e5edf8d | 85 | But it is OK if you do not. |
c170f3a4 | 86 | |
61f33134 LP |
87 | - Single-line "if" blocks should not be enclosed in {}. Use this: |
88 | ||
89 | if (foobar) | |
90 | waldo(); | |
91 | ||
92 | instead of this: | |
93 | ||
94 | if (foobar) { | |
95 | waldo(); | |
96 | } | |
97 | ||
8e5edf8d | 98 | - Do not write "foo ()", write "foo()". |
c170f3a4 LP |
99 | |
100 | - Please use streq() and strneq() instead of strcmp(), strncmp() where applicable. | |
101 | ||
102 | - Please do not allocate variables on the stack in the middle of code, | |
103 | even if C99 allows it. Wrong: | |
104 | ||
105 | { | |
106 | a = 5; | |
107 | int b; | |
108 | b = a; | |
109 | } | |
110 | ||
111 | Right: | |
112 | ||
113 | { | |
114 | int b; | |
115 | a = 5; | |
116 | b = a; | |
117 | } | |
118 | ||
119 | - Unless you allocate an array, "double" is always the better choice | |
120 | than "float". Processors speak "double" natively anyway, so this is | |
45df8656 | 121 | no speed benefit, and on calls like printf() "float"s get promoted |
c170f3a4 LP |
122 | to "double"s anyway, so there is no point. |
123 | ||
42706f47 LP |
124 | - Do not mix function invocations with variable definitions in one |
125 | line. Wrong: | |
c170f3a4 LP |
126 | |
127 | { | |
128 | int a = foobar(); | |
129 | uint64_t x = 7; | |
130 | } | |
131 | ||
132 | Right: | |
133 | ||
134 | { | |
135 | int a; | |
136 | uint64_t x = 7; | |
137 | ||
138 | a = foobar(); | |
139 | } | |
140 | ||
141 | - Use "goto" for cleaning up, and only use it for that. i.e. you may | |
d3a48513 LP |
142 | only jump to the end of a function, and little else. Never jump |
143 | backwards! | |
c170f3a4 LP |
144 | |
145 | - Think about the types you use. If a value cannot sensibly be | |
8e5edf8d | 146 | negative, do not use "int", but use "unsigned". |
c170f3a4 | 147 | |
fa195fa7 LP |
148 | - Use "char" only for actual characters. Use "uint8_t" or "int8_t" |
149 | when you actually mean a byte-sized signed or unsigned | |
150 | integers. When referring to a generic byte, we generally prefer the | |
151 | unsigned variant "uint8_t". Do not use types based on "short". They | |
152 | *never* make sense. Use ints, longs, long longs, all in | |
153 | unsigned+signed fashion, and the fixed size types | |
154 | uint8_t/uint16_t/uint32_t/uint64_t/int8_t/int16_t/int32_t and so on, | |
155 | as well as size_t, but nothing else. Do not use kernel types like | |
156 | u32 and so on, leave that to the kernel. | |
d3a48513 LP |
157 | |
158 | - Public API calls (i.e. functions exported by our shared libraries) | |
159 | must be marked "_public_" and need to be prefixed with "sd_". No | |
160 | other functions should be prefixed like that. | |
161 | ||
8d0e0ddd | 162 | - In public API calls, you *must* validate all your input arguments for |
d3a48513 | 163 | programming error with assert_return() and return a sensible return |
8d0e0ddd | 164 | code. In all other calls, it is recommended to check for programming |
d3a48513 | 165 | errors with a more brutal assert(). We are more forgiving to public |
96d49011 | 166 | users than for ourselves! Note that assert() and assert_return() |
d3a48513 LP |
167 | really only should be used for detecting programming errors, not for |
168 | runtime errors. assert() and assert_return() by usage of _likely_() | |
8e5edf8d | 169 | inform the compiler that he should not expect these checks to fail, |
d3a48513 LP |
170 | and they inform fellow programmers about the expected validity and |
171 | range of parameters. | |
172 | ||
173 | - Never use strtol(), atoi() and similar calls. Use safe_atoli(), | |
174 | safe_atou32() and suchlike instead. They are much nicer to use in | |
175 | most cases and correctly check for parsing errors. | |
176 | ||
177 | - For every function you add, think about whether it is a "logging" | |
178 | function or a "non-logging" function. "Logging" functions do logging | |
179 | on their own, "non-logging" function never log on their own and | |
180 | expect their callers to log. All functions in "library" code, | |
06b643e7 | 181 | i.e. in src/shared/ and suchlike must be "non-logging". Every time a |
8d0e0ddd | 182 | "logging" function calls a "non-logging" function, it should log |
d3a48513 LP |
183 | about the resulting errors. If a "logging" function calls another |
184 | "logging" function, then it should not generate log messages, so | |
185 | that log messages are not generated twice for the same errors. | |
186 | ||
187 | - Avoid static variables, except for caches and very few other | |
188 | cases. Think about thread-safety! While most of our code is never | |
8d0e0ddd | 189 | used in threaded environments, at least the library code should make |
d3a48513 | 190 | sure it works correctly in them. Instead of doing a lot of locking |
8d0e0ddd | 191 | for that, we tend to prefer using TLS to do per-thread caching (which |
d3a48513 LP |
192 | only works for small, fixed-size cache objects), or we disable |
193 | caching for any thread that is not the main thread. Use | |
194 | is_main_thread() to detect whether the calling thread is the main | |
195 | thread. | |
601185b4 | 196 | |
7f8bf08f | 197 | - Command line option parsing: |
601185b4 ZJS |
198 | - Do not print full help() on error, be specific about the error. |
199 | - Do not print messages to stdout on error. | |
200 | - Do not POSIX_ME_HARDER unless necessary, i.e. avoid "+" in option string. | |
7f8bf08f LP |
201 | |
202 | - Do not write functions that clobber call-by-reference variables on | |
203 | failure. Use temporary variables for these cases and change the | |
204 | passed in variables only on success. | |
dd4540da LP |
205 | |
206 | - When you allocate a file descriptor, it should be made O_CLOEXEC | |
207 | right from the beginning, as none of our files should leak to forked | |
208 | binaries by default. Hence, whenever you open a file, O_CLOEXEC must | |
699eee62 LP |
209 | be specified, right from the beginning. This also applies to |
210 | sockets. Effectively this means that all invocations to: | |
211 | ||
212 | a) open() must get O_CLOEXEC passed | |
213 | b) socket() and socketpair() must get SOCK_CLOEXEC passed | |
214 | c) recvmsg() must get MSG_CMSG_CLOEXEC set | |
215 | d) F_DUPFD_CLOEXEC should be used instead of F_DUPFD, and so on | |
7f8a0d7b | 216 | f) invocations of fopen() should take "e" |
eef46c37 | 217 | |
11c9f1e4 SM |
218 | - We never use the POSIX version of basename() (which glibc defines it in |
219 | libgen.h), only the GNU version (which glibc defines in string.h). | |
220 | The only reason to include libgen.h is because dirname() | |
eef46c37 LP |
221 | is needed. Everytime you need that please immediately undefine |
222 | basename(), and add a comment about it, so that no code ever ends up | |
11c9f1e4 | 223 | using the POSIX version! |
ddb64d82 LP |
224 | |
225 | - Use the bool type for booleans, not integers. One exception: in public | |
226 | headers (i.e those in src/systemd/sd-*.h) use integers after all, as "bool" | |
227 | is C99 and in our public APIs we try to stick to C89 (with a few extension). | |
918315e4 LP |
228 | |
229 | - When you invoke certain calls like unlink(), or mkdir_p() and you | |
230 | know it is safe to ignore the error it might return (because a later | |
231 | call would detect the failure anyway, or because the error is in an | |
232 | error path and you thus couldn't do anything about it anyway), then | |
233 | make this clear by casting the invocation explicitly to (void). Code | |
234 | checks like Coverity understand that, and will not complain about | |
235 | ignored error codes. Hence, please use this: | |
236 | ||
237 | (void) unlink("/foo/bar/baz"); | |
238 | ||
239 | instead of just this: | |
240 | ||
241 | unlink("/foo/bar/baz"); | |
3dbafa39 | 242 | |
10c6258e LP |
243 | Don't cast function calls to (void) that return no error |
244 | conditions. Specifically, the various xyz_unref() calls that return a NULL | |
245 | object shouldn't be cast to (void), since not using the return value does not | |
246 | hide any errors. | |
247 | ||
3dbafa39 LP |
248 | - Don't invoke exit(), ever. It is not replacement for proper error |
249 | handling. Please escalate errors up your call chain, and use normal | |
250 | "return" to exit from the main function of a process. If you | |
251 | fork()ed off a child process, please use _exit() instead of exit(), | |
252 | so that the exit handlers are not run. | |
9ff3e22a LP |
253 | |
254 | - Please never use dup(). Use fcntl(fd, F_DUPFD_CLOEXEC, 3) | |
255 | instead. For two reason: first, you want O_CLOEXEC set on the new fd | |
256 | (see above). Second, dup() will happily duplicate your fd as 0, 1, | |
257 | 2, i.e. stdin, stdout, stderr, should those fds be closed. Given the | |
258 | special semantics of those fds, it's probably a good idea to avoid | |
259 | them. F_DUPFD_CLOEXEC with "3" as parameter avoids them. | |
ba780c11 LP |
260 | |
261 | - When you define a destructor or unref() call for an object, please | |
262 | accept a NULL object and simply treat this as NOP. This is similar | |
263 | to how libc free() works, which accepts NULL pointers and becomes a | |
264 | NOP for them. By following this scheme a lot of if checks can be | |
265 | removed before invoking your destructor, which makes the code | |
266 | substantially more readable and robust. | |
267 | ||
268 | - Related to this: when you define a destructor or unref() call for an | |
269 | object, please make it return the same type it takes and always | |
270 | return NULL from it. This allows writing code like this: | |
271 | ||
272 | p = foobar_unref(p); | |
273 | ||
274 | which will always work regardless if p is initialized or not, and | |
275 | guarantees that p is NULL afterwards, all in just one line. | |
42706f47 LP |
276 | |
277 | - Use alloca(), but never forget that it is not OK to invoke alloca() | |
278 | within a loop or within function call parameters. alloca() memory is | |
279 | released at the end of a function, and not at the end of a {} | |
280 | block. Thus, if you invoke it in a loop, you keep increasing the | |
281 | stack pointer without ever releasing memory again. (VLAs have better | |
282 | behaviour in this case, so consider using them as an alternative.) | |
283 | Regarding not using alloca() within function parameters, see the | |
284 | BUGS section of the alloca(3) man page. | |
a5ecb0ce LP |
285 | |
286 | - Use memzero() or even better zero() instead of memset(..., 0, ...) | |
287 | ||
288 | - Instead of using memzero()/memset() to initialize structs allocated | |
289 | on the stack, please try to use c99 structure initializers. It's | |
290 | short, prettier and actually even faster at execution. Hence: | |
291 | ||
292 | struct foobar t = { | |
293 | .foo = 7, | |
294 | .bar = "bazz", | |
295 | }; | |
296 | ||
297 | instead of: | |
298 | ||
299 | struct foobar t; | |
300 | zero(t); | |
301 | t.foo = 7; | |
302 | t.bar = "bazz"; | |
0fef704c LP |
303 | |
304 | - When returning a return code from main(), please preferably use | |
305 | EXIT_FAILURE and EXIT_SUCCESS as defined by libc. | |
1811232c LP |
306 | |
307 | - The order in which header files are included doesn't matter too | |
54c1f2d7 DH |
308 | much. systemd-internal headers must not rely on an include order, so |
309 | it is safe to include them in any order possible. | |
310 | However, to not clutter global includes, and to make sure internal | |
311 | definitions will not affect global headers, please always include the | |
312 | headers of external components first (these are all headers enclosed | |
313 | in <>), followed by our own exported headers (usually everything | |
314 | that's prefixed by "sd-"), and then followed by internal headers. | |
315 | Furthermore, in all three groups, order all includes alphabetically | |
316 | so duplicate includes can easily be detected. | |
cad69822 LP |
317 | |
318 | - To implement an endless loop, use "for (;;)" rather than "while | |
319 | (1)". The latter is a bit ugly anyway, since you probably really | |
320 | meant "while (true)"... To avoid the discussion what the right | |
321 | always-true expression for an infinite while() loop is our | |
322 | recommendation is to simply write it without any such expression by | |
323 | using "for (;;)". | |
59f448cf LP |
324 | |
325 | - Never use the "off_t" type, and particularly avoid it in public | |
326 | APIs. It's really weirdly defined, as it usually is 64bit and we | |
327 | don't support it any other way, but it could in theory also be | |
328 | 32bit. Which one it is depends on a compiler switch chosen by the | |
329 | compiled program, which hence corrupts APIs using it unless they can | |
330 | also follow the program's choice. Moreover, in systemd we should | |
331 | parse values the same way on all architectures and cannot expose | |
332 | off_t values over D-Bus. To avoid any confusion regarding conversion | |
333 | and ABIs, always use simply uint64_t directly. | |
8ac5aaa9 LP |
334 | |
335 | - Commit message subject lines should be prefixed with an appropriate | |
336 | component name of some kind. For example "journal: ", "nspawn: " and | |
337 | so on. | |
338 | ||
339 | - Do not use "Signed-Off-By:" in your commit messages. That's a kernel | |
340 | thing we don't do in the systemd project. | |
341 | ||
342 | - Avoid leaving long-running child processes around, i.e. fork()s that | |
343 | are not followed quickly by an execv() in the child. Resource | |
344 | management is unclear in this case, and memory CoW will result in | |
c7ddad51 | 345 | unexpected penalties in the parent much much later on. |
8ac5aaa9 LP |
346 | |
347 | - Don't block execution for arbitrary amounts of time using usleep() | |
348 | or a similar call, unless you really know what you do. Just "giving | |
349 | something some time", or so is a lazy excuse. Always wait for the | |
350 | proper event, instead of doing time-based poll loops. | |
c7ddad51 LP |
351 | |
352 | - To determine the length of a constant string "foo", don't bother | |
353 | with sizeof("foo")-1, please use strlen("foo") directly. gcc knows | |
354 | strlen() anyway and turns it into a constant expression if possible. | |
041f793b LP |
355 | |
356 | - If you want to concatenate two or more strings, consider using | |
357 | strjoin() rather than asprintf(), as the latter is a lot | |
358 | slower. This matters particularly in inner loops. | |
ec566e4c LP |
359 | |
360 | - Please avoid using global variables as much as you can. And if you | |
361 | do use them make sure they are static at least, instead of | |
362 | exported. Especially in library-like code it is important to avoid | |
363 | global variables. Why are global variables bad? They usually hinder | |
364 | generic reusability of code (since they break in threaded programs, | |
365 | and usually would require locking there), and as the code using them | |
366 | has side-effects make programs intransparent. That said, there are | |
367 | many cases where they explicitly make a lot of sense, and are OK to | |
368 | use. For example, the log level and target in log.c is stored in a | |
369 | global variable, and that's OK and probably expected by most. Also | |
370 | in many cases we cache data in global variables. If you add more | |
371 | caches like this, please be careful however, and think about | |
372 | threading. Only use static variables if you are sure that | |
373 | thread-safety doesn't matter in your case. Alternatively consider | |
374 | using TLS, which is pretty easy to use with gcc's "thread_local" | |
375 | concept. It's also OK to store data that is inherently global in | |
376 | global variables, for example data parsed from command lines, see | |
377 | below. | |
378 | ||
379 | - If you parse a command line, and want to store the parsed parameters | |
380 | in global variables, please consider prefixing their names with | |
381 | "arg_". We have been following this naming rule in most of our | |
382 | tools, and we should continue to do so, as it makes it easy to | |
383 | identify command line parameter variables, and makes it clear why it | |
384 | is OK that they are global variables. | |
d5af8eea LP |
385 | |
386 | - When exposing public C APIs, be careful what function parameters you make | |
387 | "const". For example, a parameter taking a context object should probably not | |
388 | be "const", even if you are writing an other-wise read-only accessor function | |
389 | for it. The reason is that making it "const" fixates the contract that your | |
390 | call won't alter the object ever, as part of the API. However, that's often | |
391 | quite a promise, given that this even prohibits object-internal caching or | |
392 | lazy initialization of object variables. Moreover it's usually not too useful | |
393 | for client applications. Hence: please be careful and avoid "const" on object | |
394 | parameters, unless you are very sure "const" is appropriate. | |
395 | ||
396 | - Make sure to enforce limits on every user controllable resource. If the user | |
397 | can allocate resources in your code, your code must enforce some form of | |
398 | limits after which it will refuse operation. It's fine if it is hardcoded (at | |
399 | least initially), but it needs to be there. This is particularly important | |
400 | for objects that unprivileged users may allocate, but also matters for | |
401 | everything else any user may allocated. |