RepoMirrors/haproxy
1
0
Fork 0
mirror of http://git.haproxy.org/git/haproxy.git/ synced 2024-12-24 05:32:21 +00:00
Code Issues Packages Projects Releases Wiki Activity
345ebcfc01
haproxy/doc/coding-style.txt
Willy Tarreau 02ec3fe669 DOC: coding-style: update a few rules about pointers 
It's really annoying to see that in 2020 we're still facing bugs caused
by dangling pointers in the code that result from poorly written rules
about how these pointers are supposed to be handled, set and reset. Let's
add a few supposedly obvious (but apparently not) rules about how pointers
have to be used through out the code in hope to make such bad practices
disappear (or at least have something to point the authors to after
reviewing their code).
2020-11-18 19:59:38 +01:00

1567 lines
52 KiB
Plaintext
Raw Blame History

2020/07/07 - HAProxy coding style - Willy Tarreau <w@1wt.eu>
------------------------------------------------------------
A number of contributors are often embarrassed with coding style issues, they
don't always know if they're doing it right, especially since the coding style
has elvoved along the years. What is explained here is not necessarily what is
applied in the code, but new code should as much as possible conform to this
style. Coding style fixes happen when code is replaced. It is useless to send
patches to fix coding style only, they will be rejected, unless they belong to
a patch series which needs these fixes prior to get code changes. Also, please
avoid fixing coding style in the same patches as functional changes, they make
code review harder.
A good way to quickly validate your patch before submitting it is to pass it
through the Linux kernel's checkpatch.pl utility which can be downloaded here :
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl
Running it with the following options relaxes its checks to accommodate to the
extra degree of freedom that is tolerated in HAProxy's coding style compared to
the stricter style used in the kernel :
checkpatch.pl -q --max-line-length=160 --no-tree --no-signoff \
--ignore=LEADING_SPACE,CODE_INDENT,DEEP_INDENTATION \
--ignore=ELSE_AFTER_BRACE < patch
You can take its output as hints instead of strict rules, but in general its
output will be accurate and it may even spot some real bugs.
When modifying a file, you must accept the terms of the license of this file
which is recalled at the top of the file, or is explained in the LICENSE file,
or if not stated, defaults to LGPL version 2.1 or later for files in the
'include' directory, and GPL version 2 or later for all other files.
When adding a new file, you must add a copyright banner at the top of the
file with your real name, e-mail address and a reminder of the license.
Contributions under incompatible licenses or too restrictive licenses might
get rejected. If in doubt, please apply the principle above for existing files.
All code examples below will intentionally be prefixed with " | " to mark
where the code aligns with the first column, and tabs in this document will be
represented as a series of 8 spaces so that it displays the same everywhere.
1) Indentation and alignment
----------------------------
1.1) Indentation
----------------
Indentation and alignment are two completely different things that people often
get wrong. Indentation is used to mark a sub-level in the code. A sub-level
means that a block is executed in the context of another block (eg: a function
or a condition) :
| main(int argc, char **argv)
| {
| int i;
|
| if (argc < 2)
| exit(1);
| }
In the example above, the code belongs to the main() function and the exit()
call belongs to the if statement. Indentation is made with tabs (\t, ASCII 9),
which allows any developer to configure their preferred editor to use their
own tab size and to still get the text properly indented. Exactly one tab is
used per sub-level. Tabs may only appear at the beginning of a line or after
another tab. It is illegal to put a tab after some text, as it mangles displays
in a different manner for different users (particularly when used to align
comments or values after a #define). If you're tempted to put a tab after some
text, then you're doing it wrong and you need alignment instead (see below).
Note that there are places where the code was not properly indented in the
past. In order to view it correctly, you may have to set your tab size to 8
characters.
1.2) Alignment
--------------
Alignment is used to continue a line in a way to makes things easier to group
together. By definition, alignment is character-based, so it uses spaces. Tabs
would not work because for one tab there would not be as many characters on all
displays. For instance, the arguments in a function declaration may be broken
into multiple lines using alignment spaces :
| int http_header_match2(const char *hdr, const char *end,
| const char *name, int len)
| {
| ...
| }
In this example, the "const char *name" part is aligned with the first
character of the group it belongs to (list of function arguments). Placing it
here makes it obvious that it's one of the function's arguments. Multiple lines
are easy to handle this way. This is very common with long conditions too :
| if ((len < eol - sol) &&
| (sol[len] == ':') &&
| (strncasecmp(sol, name, len) == 0)) {
| ctx->del = len;
| }
If we take again the example above marking tabs with "[-Tabs-]" and spaces
with "#", we get this :
| [-Tabs-]if ((len < eol - sol) &&
| [-Tabs-]####(sol[len] == ':') &&
| [-Tabs-]####(strncasecmp(sol, name, len) == 0)) {
| [-Tabs-][-Tabs-]ctx->del = len;
| [-Tabs-]}
It is worth noting that some editors tend to confuse indentations and alignment.
Emacs is notoriously known for this brokenness, and is responsible for almost
all of the alignment mess. The reason is that Emacs only counts spaces, tries
to fill as many as possible with tabs and completes with spaces. Once you know
it, you just have to be careful, as alignment is not used much, so generally it
is just a matter of replacing the last tab with 8 spaces when this happens.
Indentation should be used everywhere there is a block or an opening brace. It
is not possible to have two consecutive closing braces on the same column, it
means that the innermost was not indented.
Right :
| main(int argc, char **argv)
| {
| if (argc > 1) {
| printf("Hello\n");
| }
| exit(0);
| }
Wrong :
| main(int argc, char **argv)
| {
| if (argc > 1) {
| printf("Hello\n");
| }
| exit(0);
| }
A special case applies to switch/case statements. Due to my editor's settings,
I've been used to align "case" with "switch" and to find it somewhat logical
since each of the "case" statements opens a sublevel belonging to the "switch"
statement. But indenting "case" after "switch" is accepted too. However in any
case, whatever follows the "case" statement must be indented, whether or not it
contains braces :
| switch (*arg) {
| case 'A': {
| int i;
| for (i = 0; i < 10; i++)
| printf("Please stop pressing 'A'!\n");
| break;
| }
| case 'B':
| printf("You pressed 'B'\n");
| break;
| case 'C':
| case 'D':
| printf("You pressed 'C' or 'D'\n");
| break;
| default:
| printf("I don't know what you pressed\n");
| }
2) Braces
---------
Braces are used to delimit multiple-instruction blocks. In general it is
preferred to avoid braces around single-instruction blocks as it reduces the
number of lines :
Right :
| if (argc >= 2)
| exit(0);
Wrong :
| if (argc >= 2) {
| exit(0);
| }
But it is not that strict, it really depends on the context. It happens from
time to time that single-instruction blocks are enclosed within braces because
it makes the code more symmetrical, or more readable. Example :
| if (argc < 2) {
| printf("Missing argument\n");
| exit(1);
| } else {
| exit(0);
| }
Braces are always needed to declare a function. A function's opening brace must
be placed at the beginning of the next line :
Right :
| int main(int argc, char **argv)
| {
| exit(0);
| }
Wrong :
| int main(int argc, char **argv) {
| exit(0);
| }
Note that a large portion of the code still does not conforms to this rule, as
it took years to get all authors to adapt to this more common standard which
is now preferred, as it avoids visual confusion when function declarations are
broken on multiple lines :
Right :
| int foo(const char *hdr, const char *end,
| const char *name, const char *err,
| int len)
| {
| int i;
Wrong :
| int foo(const char *hdr, const char *end,
| const char *name, const char *err,
| int len) {
| int i;
Braces should always be used where there might be an ambiguity with the code
later. The most common example is the stacked "if" statement where an "else"
may be added later at the wrong place breaking the code, but it also happens
with comments or long arguments in function calls. In general, if a block is
more than one line long, it should use braces.
Dangerous code waiting of a victim :
| if (argc < 2)
| /* ret must not be negative here */
| if (ret < 0)
| return -1;
Wrong change :
| if (argc < 2)
| /* ret must not be negative here */
| if (ret < 0)
| return -1;
| else
| return 0;
It will do this instead of what your eye seems to tell you :
| if (argc < 2)
| /* ret must not be negative here */
| if (ret < 0)
| return -1;
| else
| return 0;
Right :
| if (argc < 2) {
| /* ret must not be negative here */
| if (ret < 0)
| return -1;
| }
| else
| return 0;
Similarly dangerous example :
| if (ret < 0)
| /* ret must not be negative here */
| complain();
| init();
Wrong change to silent the annoying message :
| if (ret < 0)
| /* ret must not be negative here */
| //complain();
| init();
... which in fact means :
| if (ret < 0)
| init();
3) Breaking lines
-----------------
There is no strict rule for line breaking. Some files try to stick to the 80
column limit, but given that various people use various tab sizes, it does not
make much sense. Also, code is sometimes easier to read with less lines, as it
represents less surface on the screen (since each new line adds its tabs and
spaces). The rule is to stick to the average line length of other lines. If you
are working in a file which fits in 80 columns, try to keep this goal in mind.
If you're in a function with 120-chars lines, there is no reason to add many
short lines, so you can make longer lines.
In general, opening a new block should lead to a new line. Similarly, multiple
instructions should be avoided on the same line. But some constructs make it
more readable when those are perfectly aligned :
A copy-paste bug in the following construct will be easier to spot :
| if (omult % idiv == 0) { omult /= idiv; idiv = 1; }
| if (idiv % omult == 0) { idiv /= omult; omult = 1; }
| if (imult % odiv == 0) { imult /= odiv; odiv = 1; }
| if (odiv % imult == 0) { odiv /= imult; imult = 1; }
than in this one :
| if (omult % idiv == 0) {
| omult /= idiv;
| idiv = 1;
| }
| if (idiv % omult == 0) {
| idiv /= omult;
| omult = 1;
| }
| if (imult % odiv == 0) {
| imult /= odiv;
| odiv = 1;
| }
| if (odiv % imult == 0) {
| odiv /= imult;
| imult = 1;
| }
What is important is not to mix styles. For instance there is nothing wrong
with having many one-line "case" statements as long as most of them are this
short like below :
| switch (*arg) {
| case 'A': ret = 1; break;
| case 'B': ret = 2; break;
| case 'C': ret = 4; break;
| case 'D': ret = 8; break;
| default : ret = 0; break;
| }
Otherwise, prefer to have the "case" statement on its own line as in the
example in section 1.2 about alignment. In any case, avoid to stack multiple
control statements on the same line, so that it will never be the needed to
add two tab levels at once :
Right :
| switch (*arg) {
| case 'A':
| if (ret < 0)
| ret = 1;
| break;
| default : ret = 0; break;
| }
Wrong :
| switch (*arg) {
| case 'A': if (ret < 0)
| ret = 1;
| break;
| default : ret = 0; break;
| }
Right :
| if (argc < 2)
| if (ret < 0)
| return -1;
or Right :
| if (argc < 2)
| if (ret < 0) return -1;
but Wrong :
| if (argc < 2) if (ret < 0) return -1;
When complex conditions or expressions are broken into multiple lines, please
do ensure that alignment is perfectly appropriate, and group all main operators
on the same side (which you're free to choose as long as it does not change for
every block. Putting binary operators on the right side is preferred as it does
not mangle with alignment but various people have their preferences.
Right :
| if ((txn->flags & TX_NOT_FIRST) &&
| ((req->flags & BF_FULL) ||
| req->r < req->lr ||
| req->r > req->data + req->size - global.tune.maxrewrite)) {
| return 0;
| }
Right :
| if ((txn->flags & TX_NOT_FIRST)
| && ((req->flags & BF_FULL)
| || req->r < req->lr
| || req->r > req->data + req->size - global.tune.maxrewrite)) {
| return 0;
| }
Wrong :
| if ((txn->flags & TX_NOT_FIRST) &&
| ((req->flags & BF_FULL) ||
| req->r < req->lr
| || req->r > req->data + req->size - global.tune.maxrewrite)) {
| return 0;
| }
If it makes the result more readable, parenthesis may even be closed on their
own line in order to align with the opening one. Note that should normally not
be needed because such code would be too complex to be digged into.
The "else" statement may either be merged with the closing "if" brace or lie on
its own line. The later is preferred but it adds one extra line to each control
block which is annoying in short ones. However, if the "else" is followed by an
"if", then it should really be on its own line and the rest of the if/else
blocks must follow the same style.
Right :
| if (a < b) {
| return a;
| }
| else {
| return b;
| }
Right :
| if (a < b) {
| return a;
| } else {
| return b;
| }
Right :
| if (a < b) {
| return a;
| }
| else if (a != b) {
| return b;
| }
| else {
| return 0;
| }
Wrong :
| if (a < b) {
| return a;
| } else if (a != b) {
| return b;
| } else {
| return 0;
| }
Wrong :
| if (a < b) {
| return a;
| }
| else if (a != b) {
| return b;
| } else {
| return 0;
| }
4) Spacing
----------
Correctly spacing code is very important. When you have to spot a bug at 3am,
you need it to be clear. When you expect other people to review your code, you
want it to be clear and don't want them to get nervous when trying to find what
you did.
Always place spaces around all binary or ternary operators, commas, as well as
after semi-colons and opening braces if the line continues :
Right :
| int ret = 0;
| /* if (x >> 4) { x >>= 4; ret += 4; } */
| ret += (x >> 4) ? (x >>= 4, 4) : 0;
| val = ret + ((0xFFFFAA50U >> (x << 1)) & 3) + 1;
Wrong :
| int ret=0;
| /* if (x>>4) {x>>=4;ret+=4;} */
| ret+=(x>>4)?(x>>=4,4):0;
| val=ret+((0xFFFFAA50U>>(x<<1))&3)+1;
Never place spaces after unary operators (&, *, -, !, ~, ++, --) nor cast, as
they might be confused with they binary counterpart, nor before commas or
semicolons :
Right :
| bit = !!(~len++ ^ -(unsigned char)*x);
Wrong :
| bit = ! ! (~len++ ^ - (unsigned char) * x) ;
Note that "sizeof" is a unary operator which is sometimes considered as a
language keyword, but in no case it is a function. It does not require
parenthesis so it is sometimes followed by spaces and sometimes not when
there are no parenthesis. Most people do not really care as long as what
is written is unambiguous.
Braces opening a block must be preceded by one space unless the brace is
placed on the first column :
Right :
| if (argc < 2) {
| }
Wrong :
| if (argc < 2){
| }
Do not add unneeded spaces inside parenthesis, they just make the code less
readable.
Right :
| if (x < 4 && (!y || !z))
| break;
Wrong :
| if ( x < 4 && ( !y || !z ) )
| break;
Language keywords must all be followed by a space. This is true for control
statements (do, for, while, if, else, return, switch, case), and for types
(int, char, unsigned). As an exception, the last type in a cast does not take
a space before the closing parenthesis). The "default" statement in a "switch"
construct is generally just followed by the colon. However the colon after a
"case" or "default" statement must be followed by a space.
Right :
| if (nbargs < 2) {
| printf("Missing arg at %c\n", *(char *)ptr);
| for (i = 0; i < 10; i++) beep();
| return 0;
| }
| switch (*arg) {
Wrong :
| if(nbargs < 2){
| printf("Missing arg at %c\n", *(char*)ptr);
| for(i = 0; i < 10; i++)beep();
| return 0;
| }
| switch(*arg) {
Function calls are different, the opening parenthesis is always coupled to the
function name without any space. But spaces are still needed after commas :
Right :
| if (!init(argc, argv))
| exit(1);
Wrong :
| if (!init (argc,argv))
| exit(1);
5) Excess or lack of parenthesis
--------------------------------
Sometimes there are too many parenthesis in some formulas, sometimes there are
too few. There are a few rules of thumb for this. The first one is to respect
the compiler's advice. If it emits a warning and asks for more parenthesis to
avoid confusion, follow the advice at least to shut the warning. For instance,
the code below is quite ambiguous due to its alignment :
| if (var1 < 2 || var2 < 2 &&
| var3 != var4) {
| /* fail */
| return -3;
| }
Note that this code does :
| if (var1 < 2 || (var2 < 2 && var3 != var4)) {
| /* fail */
| return -3;
| }
But maybe the author meant :
| if ((var1 < 2 || var2 < 2) && var3 != var4) {
| /* fail */
| return -3;
| }
A second rule to put parenthesis is that people don't always know operators
precedence too well. Most often they have no issue with operators of the same
category (eg: booleans, integers, bit manipulation, assignment) but once these
operators are mixed, it causes them all sort of issues. In this case, it is
wise to use parenthesis to avoid errors. One common error concerns the bit
shift operators because they're used to replace multiplies and divides but
don't have the same precedence :
The expression :
| x = y * 16 + 5;
becomes :
| x = y << 4 + 5;
which is wrong because it is equivalent to :
| x = y << (4 + 5);
while the following was desired instead :
| x = (y << 4) + 5;
It is generally fine to write boolean expressions based on comparisons without
any parenthesis. But on top of that, integer expressions and assignments should
then be protected. For instance, there is an error in the expression below
which should be safely rewritten :
Wrong :
| if (var1 > 2 && var1 < 10 ||
| var1 > 2 + 256 && var2 < 10 + 256 ||
| var1 > 2 + 1 << 16 && var2 < 10 + 2 << 16)
| return 1;
Right (may remove a few parenthesis depending on taste) :
| if ((var1 > 2 && var1 < 10) ||
| (var1 > (2 + 256) && var2 < (10 + 256)) ||
| (var1 > (2 + (1 << 16)) && var2 < (10 + (1 << 16))))
| return 1;
The "return" statement is not a function, so it takes no argument. It is a
control statement which is followed by the expression to be returned. It does
not need to be followed by parenthesis :
Wrong :
| int ret0()
| {
| return(0);
| }
Right :
| int ret0()
| {
| return 0;
| }
Parenthesisis are also found in type casts. Type casting should be avoided as
much as possible, especially when it concerns pointer types. Casting a pointer
disables the compiler's type checking and is the best way to get caught doing
wrong things with data not the size you expect. If you need to manipulate
multiple data types, you can use a union instead. If the union is really not
convenient and casts are easier, then try to isolate them as much as possible,
for instance when initializing function arguments or in another function. Not
proceeding this way causes huge risks of not using the proper pointer without
any notification, which is especially true during copy-pastes.
Wrong :
| void *check_private_data(void *arg1, void *arg2)
| {
| char *area;
|
| if (*(int *)arg1 > 1000)
| return NULL;
| if (memcmp(*(const char *)arg2, "send(", 5) != 0))
| return NULL;
| area = malloc(*(int *)arg1);
| if (!area)
| return NULL;
| memcpy(area, *(const char *)arg2 + 5, *(int *)arg1);
| return area;
| }
Right :
| void *check_private_data(void *arg1, void *arg2)
| {
| char *area;
| int len = *(int *)arg1;
| const char *msg = arg2;
|
| if (len > 1000)
| return NULL;
| if (memcmp(msg, "send(", 5) != 0)
| return NULL;
| area = malloc(len);
| if (!area)
| return NULL;
| memcpy(area, msg + 5, len);
| return area;
| }
6) Ambiguous comparisons with zero or NULL
------------------------------------------
In C, '0' has no type, or it has the type of the variable it is assigned to.
Comparing a variable or a return value with zero means comparing with the
representation of zero for this variable's type. For a boolean, zero is false.
For a pointer, zero is NULL. Very often, to make things shorter, it is fine to
use the '!' unary operator to compare with zero, as it is shorter and easier to
remind or understand than a plain '0'. Since the '!' operator is read "not", it
helps read code faster when what follows it makes sense as a boolean, and it is
often much more appropriate than a comparison with zero which makes an equal
sign appear at an undesirable place. For instance :
| if (!isdigit(*c) && !isspace(*c))
| break;
is easier to understand than :
| if (isdigit(*c) == 0 && isspace(*c) == 0)
| break;
For a char this "not" operator can be reminded as "no remaining char", and the
absence of comparison to zero implies existence of the tested entity, hence the
simple strcpy() implementation below which automatically stops once the last
zero is copied :
| void my_strcpy(char *d, const char *s)
| {
| while ((*d++ = *s++));
| }
Note the double parenthesis in order to avoid the compiler telling us it looks
like an equality test.
For a string or more generally any pointer, this test may be understood as an
existence test or a validity test, as the only pointer which will fail to
validate equality is the NULL pointer :
| area = malloc(1000);
| if (!area)
| return -1;
However sometimes it can fool the reader. For instance, strcmp() precisely is
one of such functions whose return value can make one think the opposite due to
its name which may be understood as "if strings compare...". Thus it is strongly
recommended to perform an explicit comparison with zero in such a case, and it
makes sense considering that the comparison's operator is the same that is
wanted to compare the strings (note that current config parser lacks a lot in
this regards) :
strcmp(a, b) == 0 <=> a == b
strcmp(a, b) != 0 <=> a != b
strcmp(a, b) < 0 <=> a < b
strcmp(a, b) > 0 <=> a > b
Avoid this :
| if (strcmp(arg, "test"))
| printf("this is not a test\n");
|
| if (!strcmp(arg, "test"))
| printf("this is a test\n");
Prefer this :
| if (strcmp(arg, "test") != 0)
| printf("this is not a test\n");
|
| if (strcmp(arg, "test") == 0)
| printf("this is a test\n");
7) System call returns
----------------------
This is not directly a matter of coding style but more of bad habits. It is
important to check for the correct value upon return of syscalls. The proper
return code indicating an error is described in its man page. There is no
reason to consider wider ranges than what is indicated. For instance, it is
common to see such a thing :
| if ((fd = open(file, O_RDONLY)) < 0)
| return -1;
This is wrong. The man page says that -1 is returned if an error occurred. It
does not suggest that any other negative value will be an error. It is possible
that a few such issues have been left in existing code. They are bugs for which
fixes are accepted, even though they're currently harmless since open() is not
known for returning negative values at the moment.
8) Declaring new types, names and values
----------------------------------------
Please refrain from using "typedef" to declare new types, they only obfuscate
the code. The reader never knows whether he's manipulating a scalar type or a
struct. For instance it is not obvious why the following code fails to build :
| int delay_expired(timer_t exp, timer_us_t now)
| {
| return now >= exp;
| }
With the types declared in another file this way :
| typedef unsigned int timer_t;
| typedef struct timeval timer_us_t;
This cannot work because we're comparing a scalar with a struct, which does
not make sense. Without a typedef, the function would have been written this
way without any ambiguity and would not have failed :
| int delay_expired(unsigned int exp, struct timeval *now)
| {
| return now >= exp->tv_sec;
| }
Declaring special values may be done using enums. Enums are a way to define
structured integer values which are related to each other. They are perfectly
suited for state machines. While the first element is always assigned the zero
value, not everybody knows that, especially people working with multiple
languages all the day. For this reason it is recommended to explicitly force
the first value even if it's zero. The last element should be followed by a
comma if it is planned that new elements might later be added, this will make
later patches shorter. Conversely, if the last element is placed in order to
get the number of possible values, it must not be followed by a comma and must
be preceded by a comment :
| enum {
| first = 0,
| second,
| third,
| fourth,
| };
| enum {
| first = 0,
| second,
| third,
| fourth,
| /* nbvalues must always be placed last */
| nbvalues
| };
Structure names should be short enough not to mangle function declarations,
and explicit enough to avoid confusion (which is the most important thing).
Wrong :
| struct request_args { /* arguments on the query string */
| char *name;
| char *value;
| struct misc_args *next;
| };
Right :
| struct qs_args { /* arguments on the query string */
| char *name;
| char *value;
| struct qs_args *next;
| }
When declaring new functions or structures, please do not use CamelCase, which
is a style where upper and lower case are mixed in a single word. It causes a
lot of confusion when words are composed from acronyms, because it's hard to
stick to a rule. For instance, a function designed to generate an ISN (initial
sequence number) for a TCP/IP connection could be called :
- generateTcpipIsn()
- generateTcpIpIsn()
- generateTcpIpISN()
- generateTCPIPISN()
etc...
None is right, none is wrong, these are just preferences which might change
along the code. Instead, please use an underscore to separate words. Lowercase
is preferred for the words, but if acronyms are upcased it's not dramatic. The
real advantage of this method is that it creates unambiguous levels even for
short names.
Valid examples :
- generate_tcpip_isn()
- generate_tcp_ip_isn()
- generate_TCPIP_ISN()
- generate_TCP_IP_ISN()
Another example is easy to understand when 3 arguments are involved in naming
the function :
Wrong (naming conflict) :
| /* returns A + B * C */
| int mulABC(int a, int b, int c)
| {
| return a + b * c;
| }
|
| /* returns (A + B) * C */
| int mulABC(int a, int b, int c)
| {
| return (a + b) * c;
| }
Right (unambiguous naming) :
| /* returns A + B * C */
| int mul_a_bc(int a, int b, int c)
| {
| return a + b * c;
| }
|
| /* returns (A + B) * C */
| int mul_ab_c(int a, int b, int c)
| {
| return (a + b) * c;
| }
Whenever you manipulate pointers, try to declare them as "const", as it will
save you from many accidental misuses and will only cause warnings to be
emitted when there is a real risk. In the examples below, it is possible to
call my_strcpy() with a const string only in the first declaration. Note that
people who ignore "const" are often the ones who cast a lot and who complain
from segfaults when using strtok() !
Right :
| void my_strcpy(char *d, const char *s)
| {
| while ((*d++ = *s++));
| }
|
| void say_hello(char *dest)
| {
| my_strcpy(dest, "hello\n");
| }
Wrong :
| void my_strcpy(char *d, char *s)
| {
| while ((*d++ = *s++));
| }
|
| void say_hello(char *dest)
| {
| my_strcpy(dest, "hello\n");
| }
9) Getting macros right
-----------------------
It is very common for macros to do the wrong thing when used in a way their
author did not have in mind. For this reason, macros must always be named with
uppercase letters only. This is the only way to catch the developer's eye when
using them, so that they double-check whether they are taking a risk or not. First,
macros must never ever be terminated by a semi-colon, or they will close the
wrong block once in a while. For instance, the following will cause a build
error before the "else" due to the double semi-colon :
Wrong :
| #define WARN printf("warning\n");
| ...
| if (a < 0)
| WARN;
| else
| a--;
Right :
| #define WARN printf("warning\n")
If multiple instructions are needed, then use a do { } while (0) block, which
is the only construct which respects *exactly* the semantics of a single
instruction :
| #define WARN do { printf("warning\n"); log("warning\n"); } while (0)
| ...
|
| if (a < 0)
| WARN;
| else
| a--;
Second, do not put unprotected control statements in macros, they will
definitely cause bugs :
Wrong :
| #define WARN if (verbose) printf("warning\n")
| ...
| if (a < 0)
| WARN;
| else
| a--;
Which is equivalent to the undesired form below :
| if (a < 0)
| if (verbose)
| printf("warning\n");
| else
| a--;
Right way to do it :
| #define WARN do { if (verbose) printf("warning\n"); } while (0)
| ...
| if (a < 0)
| WARN;
| else
| a--;
Which is equivalent to :
| if (a < 0)
| do { if (verbose) printf("warning\n"); } while (0);
| else
| a--;
Macro parameters must always be surrounded by parenthesis, and must never be
duplicated in the same macro unless explicitly stated. Also, macros must not be
defined with operators without surrounding parenthesis. The MIN/MAX macros are
a pretty common example of multiple misuses, but this happens as early as when
using bit masks. Most often, in case of any doubt, try to use inline functions
instead.
Wrong :
| #define MIN(a, b) a < b ? a : b
|
| /* returns 2 * min(a,b) + 1 */
| int double_min_p1(int a, int b)
| {
| return 2 * MIN(a, b) + 1;
| }
What this will do :
| int double_min_p1(int a, int b)
| {
| return 2 * a < b ? a : b + 1;
| }
Which is equivalent to :
| int double_min_p1(int a, int b)
| {
| return (2 * a) < b ? a : (b + 1);
| }
The first thing to fix is to surround the macro definition with parenthesis to
avoid this mistake :
| #define MIN(a, b) (a < b ? a : b)
But this is still not enough, as can be seen in this example :
| /* compares either a or b with c */
| int min_ab_c(int a, int b, int c)
| {
| return MIN(a ? a : b, c);
| }
Which is equivalent to :
| int min_ab_c(int a, int b, int c)
| {
| return (a ? a : b < c ? a ? a : b : c);
| }
Which in turn means a totally different thing due to precedence :
| int min_ab_c(int a, int b, int c)
| {
| return (a ? a : ((b < c) ? (a ? a : b) : c));
| }
This can be fixed by surrounding *each* argument in the macro with parenthesis:
| #define MIN(a, b) ((a) < (b) ? (a) : (b))
But this is still not enough, as can be seen in this example :
| int min_ap1_b(int a, int b)
| {
| return MIN(++a, b);
| }
Which is equivalent to :
| int min_ap1_b(int a, int b)
| {
| return ((++a) < (b) ? (++a) : (b));
| }
Again, this is wrong because "a" is incremented twice if below b. The only way
to fix this is to use a compound statement and to assign each argument exactly
once to a local variable of the same type :
| #define MIN(a, b) ({ typeof(a) __a = (a); typeof(b) __b = (b); \
| ((__a) < (__b) ? (__a) : (__b)); \
| })
At this point, using static inline functions is much cleaner if a single type
is to be used :
| static inline int min(int a, int b)
| {
| return a < b ? a : b;
| }
10) Includes
------------
Includes are as much as possible listed in alphabetically ordered groups :
- the includes more or less system-specific (sys/*, netinet/*, ...)
- the libc-standard includes (those without any path component)
- includes from the local "import" subdirectory
- includes from the local "haproxy" subdirectory
Each section is just visually delimited from the other ones using an empty
line. The two first ones above may be merged into a single section depending on
developer's preference. Please do not copy-paste include statements from other
files. Having too many includes significantly increases build time and makes it
hard to find which ones are needed later. Just include what you need and if
possible in alphabetical order so that when something is missing, it becomes
obvious where to look for it and where to add it.
All files should include <haproxy/api.h> because this is where build options
are prepared.
Haproxy header files are split in two, those exporting the types only (named
with a trailing "-t") and those exporting variables, functions and inline
functions. Types, structures, enums and #defines must go into the types files
which are the only ones that may be included by othertype files. Function
prototypes and inlined functions must go into the main files. This split is
because of inlined functions which cross-reference types from other files,
which cause a chicken-and-egg problem if the functions and types are declared
at the same place.
Include files must be protected against multiple inclusion using the common
#ifndef/#define/#endif trick with a tag derived from the include file and its
location.
11) Comments
------------
Comments are preferably of the standard 'C' form using /* */. The C++ form "//"
are tolerated for very short comments (eg: a word or two) but should be avoided
as much as possible. Multi-line comments are made with each intermediate line
starting with a star aligned with the first one, as in this example :
| /*
| * This is a multi-line
| * comment.
| */
If multiple code lines need a short comment, try to align them so that you can
have multi-line sentences. This is rarely needed, only for really complex
constructs.
Do not tell what you're doing in comments, but explain why you're doing it if
it seems not to be obvious. Also *do* indicate at the top of function what they
accept and what they don't accept. For instance, strcpy() only accepts output
buffers at least as large as the input buffer, and does not support any NULL
pointer. There is nothing wrong with that if the caller knows it.
Wrong use of comments :
| int flsnz8(unsigned int x)
| {
| int ret = 0; /* initialize ret */
| if (x >> 4) { x >>= 4; ret += 4; } /* add 4 to ret if needed */
| return ret + ((0xFFFFAA50U >> (x << 1)) & 3) + 1; /* add ??? */
| }
| ...
| bit = ~len + (skip << 3) + 9; /* update bit */
Right use of comments :
| /* This function returns the position of the highest bit set in the lowest
| * byte of <x>, between 0 and 7. It only works if <x> is non-null. It uses
| * a 32-bit value as a lookup table to return one of 4 values for the
| * highest 16 possible 4-bit values.
| */
| int flsnz8(unsigned int x)
| {
| int ret = 0;
| if (x >> 4) { x >>= 4; ret += 4; }
| return ret + ((0xFFFFAA50U >> (x << 1)) & 3) + 1;
| }
| ...
| bit = ~len + (skip << 3) + 9; /* (skip << 3) + (8 - len), saves 1 cycle */
12) Use of assembly
-------------------
There are many projects where use of assembly code is not welcome. There is no
problem with use of assembly in haproxy, provided that :
a) an alternate C-form is provided for architectures not covered
b) the code is small enough and well commented enough to be maintained
It is important to take care of various incompatibilities between compiler
versions, for instance regarding output and cloberred registers. There are
a number of documentations on the subject on the net. Anyway if you are
fiddling with assembly, you probably know that already.
Example :
| /* gcc does not know when it can safely divide 64 bits by 32 bits. Use this
| * function when you know for sure that the result fits in 32 bits, because
| * it is optimal on x86 and on 64bit processors.
| */
| static inline unsigned int div64_32(unsigned long long o1, unsigned int o2)
| {
| unsigned int result;
| #ifdef __i386__
| asm("divl %2"
| : "=a" (result)
| : "A"(o1), "rm"(o2));
| #else
| result = o1 / o2;
| #endif
| return result;
| }
13) Pointers
------------
A lot could be said about pointers, there's enough to fill entire books. Misuse
of pointers is one of the primary reasons for bugs in haproxy, and this rate
has significantly increased with the use of threads. Moreover, bogus pointers
cause the hardest to analyse bugs, because usually they result in modifications
to reassigned areas or accesses to unmapped areas, and in each case, bugs that
strike very far away from where they were located. Some bugs have already taken
up to 3 weeks of full time analysis, which has a severe impact on the project's
ability to make forward progress on important features. For this reason, code
that doesn't look robust enough or that doesn't follow some of the rules below
will be rejected, and may even be reverted after being merged if the trouble is
detected late!
13.1) No test before freeing
----------------------------
All platforms where haproxy is supported have a well-defined and documented
behavior for free(NULL), which is to do nothing at all. In other words, free()
does test for the pointer's nullity. As such, there is no point in testing
if a pointer is NULL or not before calling free(). And further, you must not
do it, because it adds some confusion to the reader during debugging sessions,
making one think that the code's authors weren't very sure about what they
were doing. This will not cause a bug but will result in your code to get
rejected.
Wrong call to free :
| static inline int blah_free(struct blah *blah)
| {
| if (blah->str1)
| free(blah->str1);
| if (blah->str2)
| free(blah->str2);
| free(blah);
| }
Correct call to free :
| static inline int blah_free(struct blah *blah)
| {
| free(blah->str1);
| free(blah->str2);
| free(blah);
| }
13.2) No dangling pointers
--------------------------
Pointers are very commonly used as booleans: if they're not NULL, then the
area they point to is valid and may be used. This is convenient for many things
and is even emphasized with threads where they can atomically be swapped with
another value (even NULL), and as such provide guaranteed atomic resource
allocation and sharing.
The problem with this is when someone forgets to delete a pointer when an area
is no longer valid, because this may result in the pointer being accessed later
and pointing to a wrong location, one that was reallocated for something else
and causing all sort of nastiness like crashes or memory corruption. Moreover,
thanks to the memory pools, it is extremely likely that a just released pointer
will be reassigned to a similar object with comparable values (flags etc) at
the same positions, making tests apparently succeed for a while. Some such bugs
have gone undetected for several years.
The rule is pretty simple:
+-----------------------------------------------------------------+
| NO REACHABLE POINTER MAY EVER POINT TO AN UNREACHABLE LOCATION. |
+-----------------------------------------------------------------+
By "reachable pointer", here we mean a pointer that is accessible from a
reachable structure or a global variable. This means that any pointer found
anywhere in any structure in the code may always be dereferenced. This can
seem obvious but this is not always enforced.
This means that when freeing an area, the pointer that was used to find that
area must be overwritten with NULL, and all other such pointers must as well
if any. It is one case where one can find more convenient to write the NULL
on the same line as the call to free() to make things easier to check. Be
careful about any potential "if" when doing this.
Wrong use of free :
| static inline int blah_recycle(struct blah *blah)
| {
| free(blah->str1);
| free(blah->str2);
| }
Correct use of free :
| static inline int blah_recycle(struct blah *blah)
| {
| free(blah->str1); blah->str1 = NULL;
| free(blah->str2); blah->str2 = NULL;
| }
Sometimes the code doesn't permit this to be done. It is not a matter of code
but a matter of architecture. Example:
Initialization:
| static struct foo *foo_init()
| {
| struct foo *foo;
| struct bar *bar;
|
| foo = pool_alloc(foo_head);
| bar = pool_alloc(bar_head);
| if (!foo || !bar)
| goto fail;
| foo->bar = bar;
| ...
| }
Scheduled task 1:
| static inline int foo_timeout(struct foo *foo)
| {
| free(foo->bar);
| free(foo);
| }
Scheduled task 2:
| static inline int bar_timeout(struct bar *bar)
| {
| free(bar);
| }
Here it's obvious that if "bar" times out, it will be freed but its pointer in
"foo" will remain here, and if foo times out just after, it will lead to a
double free. Or worse, if another instance allocates a pointer and receives bar
again, when foo times out, it will release the old bar pointer which now points
to a new object, and the code using that new object will crash much later, or
even worse, will share the same area as yet another instance having inherited
that pointer again.
Here this simply means that the data model is wrong. If bar may be freed alone,
it MUST have a pointer to foo so that bar->foo->bar is set to NULL to let foo
finish its life peacefully. This also means that the code dealing with foo must
be written in a way to support bar's leaving.
13.3) Don't abuse pointers as booleans
--------------------------------------
Given the common use of a pointer to know if the area it points to is valid,
there is a big incentive in using such pointers as booleans to describe
something a bit higher level, like "is the user authenticated". This must not
be done. The reason stems from the points above. Initially this perfectly
matches and the code is simple. Then later some extra options need to be added,
and more pointers are needed, all allocated together. At this point they all
start to become their own booleans, supposedly always equivalent, but if that
were true, they would be a single area with a single pointer. And things start
to fall apart with some code areas relying on one pointer for the condition and
other ones relying on other pointers. Pointers may be substituted with "flags"
or "present in list" etc here. And from this point, things quickly degrade with
pointers needing to remain set even if pointing to wrong areas, just for the
sake of not being NULL and not breaking some assumptions. At this point the
bugs are already there and the code is not trustable anymore.
The only way to avoid this is to strictly respect this rule: pointers do not
represent a functionality but a storage area. Of course it is very frequent to
consider that if an optional string is not set, a feature is not enabled. This
can be fine to some extents. But as soon as any slightest condition is added
anywhere into the mux, the code relying on the pointer must be replaced with
something else so that the pointer may live its own life and be released (and
reset) earlier if needed.
13.4) Mixing const and non-const
--------------------------------
Something often encountered, especially when assembling error messages, is
functions that collect strings, assemble them into larger messages and free
everything. The problem here is that if strings are defined as variables, there
will rightfully be build warnings when reporting string constants such as bare
keywords or messages, and if strings are defined as constants, it is not
possible to free them. The temptation is sometimes huge to force some free()
calls on casted strings. Do not do that! It will inevitably lead to someone
getting caught passing a constant string that will make the process crash (if
lucky). Document the expectations, indicate that all arguments must be freeable
and that the caller must be capable of strdup(), and make your function support
NULLs and docuemnt it (so that callers can deal with a failing strdup() on
allocation error).
One valid alternative is to use a secondary channel to indicate whether the
message may be freed or not. A flag in a complex structure can be used for this
purpose, for example. If you are certain that your strings are aligned to a
certain number of bytes, it can be possible to instrument the code to use the
lowest bit to indicate the need to free (e.g. by always adding one to every
const string). But such a solution will require good enough instrumentation so
that it doesn't constitute a new set of traps.
13.5) No pointer casts
----------------------
Except in rare occasions caused by legacy APIs (e.g. sockaddr) or special cases
which explicitly require a form of aliasing, there is no valid reason for
casting pointers, and usually this is used to hide other problems that will
strike later. The only suitable type of cast is the cast from the generic void*
used to store a context for example. But in C, there is no need to cast to nor
from void*, so this is not required. However those coming from C++ tend to be
used to this practice, and others argue that it makes the intent more visible.
As a corollary, do not abuse void*. Placing void* everywhere to avoid casting
is a bad practice as well. The use of void* is only for generic functions or
structures which do not have a limited set of types supported. When only a few
types are supported, generally their type can be passed using a side channel,
and the void* can be turned into a union that makes the code more readable and
more verifiable.
An alternative in haproxy is to use a pointer to an obj_type enum. Usually it
is placed at the beginning of a structure. It works like a void* except that
the type is read directly from the object. This is convenient when a small set
of remote objects may be attached to another one because a single of them will
match a non-null pointer (e.g. a connection or an applet).
Example:
| static inline int blah_free(struct blah *blah)
| {
| /* only one of them (at most) will not be null */
| pool_free(pool_head_connection, objt_conn(blah->target));
| pool_free(pool_head_appctx, objt_appctx(blah->target));
| pool_free(pool_head_stream, objt_stream(blah->target));
| blah->target = NULL;
| }
13.6) Extreme caution when using non-canonical pointers
-------------------------------------------------------
It can be particularly convenient to embed some logic in the unused bits or
code points of a pointer. Indeed, when it is known by design that a given
pointer will always follow a certain alignment, a few lower bits will always
remain zero, and as such may be used as optional flags. For example, the ebtree
code uses the lowest bit to differentiate left/right attachments to the parent
and node/leaf in branches. It is also known that values very close to NULL will
never represent a valid pointer, and the thread-safe MT_LIST code uses this to
lock visited pointers.
There are a few rules to respect in order to do this:
- the deviations from the canonical pointers must be exhaustively documented
where the pointer type is defined, and the whole control logic with its
implications and possible and impossible cases must be enumerated as well ;
- make sure that the operations will work on every supported platform, which
includes 32-bit platforms where structures may be aligned on as little as
32-bit. 32-bit alignment leaves only two LSB available. When doing so, make
sure the target structures are not labelled with the "packed" attribute, or
that they're always perfectly aligned. All platforms where haproxy runs
have their NULL pointer mapped at address zero, and use page sizes at least
4096 bytes large, leaving all values form 1 to 4095 unused. Anything
outside of this is unsafe. In particular, never use negative numbers to
represent a supposedly invalid address. On 32-bits platforms it will often
correspond to a system address or a special page. Always try a variety of
platforms when doing such a thing.
- the code must not use such pointers as booleans anymore even if it is known
that "it works" because that keeps a doubt open for the reviewer. Only the
canonical pointer may be tested. There can be a rare exception which is if
this is on a critical path where severe performance degradation may result
from this. In this case, *each* of the checks must be duly documented and
the equivalent BUG_ON() instances must be placed to prove the claim.
- some inline functions (or macros) must be used to turn the pointers to/from
their canonical form so that the regular code doesn't have to see the
operations, and so that the representation may be easily adjusted in the
future. A few comments indicating to a human how to turn a pointer back and
forth from inside a debugger will be appreciated, as macros often end up
not being trivially readable nor directly usable.
- do not use int types to cast the pointers, this will only work on 32-bit
platforms. While "long" is usually fine, it is not recommended anymore due
to the Windows platform being LLP64 and having it set to 32 bits. And
"long long" isn't good either for always being 64 bits. More suitable types
are ptrdiff_t or size_t. Note that while those were not available everywhere
in the early days of hparoxy, size_t is now heavily used and known to work
everywhere. And do not perform the operations on the pointers, only on the
integer types (and cast back again). Some compilers such as gcc are
extremely picky about this and wil often emit wrong code when they see
equality conditions they believe is impossible and decide to optimize them
away.
13.7) Pointers in unions
------------------------
Before placing multiple aliasing pointers inside a same union, there MUST be a
SINGLE well-defined way to figure them out from each other. It may be thanks to
a side-channel information (as done in the samples with a defined type), it may
be based on in-area information (as done using obj_types), or any other trusted
solution. In any case, if pointers are mixed with any other type (integer or
float) in a union, there must be a very simple way to distinguish them, and not
a platform-dependent nor compiler-dependent one.
Reference in New Issue View Git Blame Copy Permalink