Introduction
The OpenVPN 2.4+ coding style is based on the Allman style, e.g.:
void myfunction(int a, int b) { const char *dummy = "Hello world!"; if (a == b) { something_equals(dummy); } else { does_not_equal(dummy); } }
Summarizing:
- Indentation is 4 spaces, no tabs ever.
- Opening and closing brackets get their own line, and must match indentation.
- Line length is 80 characters (soft limit) but may be extended up to 120 if wrapping makes the result less readable / more ugly
- C99 is allowed. E.g.
for (int i = 0; i < max; i++). - Only use
/* */-style comments
Line wrapping
The maximum line length is 80 characters. When statements exceed this length, wrap them by aligning with the appropriate (, or otherwise using a single indent (ie 4 spaces) on the new line. For example:
void my_function(void) { if (variable_with_artificially_long_name_1 != 0 && variable_with_artificially_long_name_2 != 0) { return variable_with_artificially_long_name_1 + variable_with_artificially_long_name_2 + variable_with_artificially_long_name_3; } }
If you find yourself continuously exceeding the line limit, you might want to consider breaking up your function into smaller functions to reduce nesting.
Single lines may use up to 120 characters if the resulting code is more readable than if wrapped. But this should be an exception, no whole code blocks should be written with "more than 80 as a general norm".
Some additional secure coding style rules
This are style rules, not general secure coding guide lines. See e.g. https://www.securecoding.cert.org/confluence/display/c/SEI+CERT+C+Coding+Standard for useful guidelines on writing secure C.
All branches must use braces
For example:
if (a) { return true; }
We wouldn't be the first to not spot missing braces in an if statement, like what happened in Apple's goto fail; bug. Always using braces prevents this kind of mistake.
Cases in a switch statement should break or explicitly fall through
The only exception to this rule are empty case statements, to not overly clutter the code with comments.
For example:
switch (a) { case ONE: case TWO: one_or_two = true; /* Fall through */ case THREE: now_do_something(); break; default: ASSERT(0); }
This makes the intent immediately clear to the reader / reviewer.
Portability concerns
Printing time_t and suseconds_t values
POSIX specifies that time_t is an integer type but does not specify its size. The underlying type might be "int", "long" or "long long", some systems might make it unsigned too. This makes it hard to portably print time_t values using printf-like APIs, as a mismatch between the format string and the actual time_t size can lead to crashes or bogus representations.
The safest way is to print time_t values after casting to int64_t and using PRIi64 as format specifier. Casting to a 64 bits wide int avoids y2038 problems and "PRIi64" is preferred over %lld as the latter is not supported in the windows runtime targeted by mingw.
Similarly, POSIX specifies suseconds_t as a signed integer, but does not specify its size. Casting it as a long is enough.
struct timeval now; gettimeofday(&now, NULL); printf("%"PRIi64".%06ld\n", (int64_t)now.tv_sec, (long)now.tv_usec);
Windows specific
OpenVPN core uses UTF-8 encoding for strings. These may be converted to and from UTF-16 used in Windows as required. Declare all strings explicitly as char * or wchar_t *. Use of TCHAR is not allowed. Call Windows API functions using their explicit ANSI or wide character variants: e.g., CreateFileW or CreateFileA instead of CreateFile. The core executable for Windows is built without -DUNICODE and -D_UNICODE.
Sources for Windows-only executables and libraries such as openvpnserv.exe, tapctl.exe and openvpnmscia.dll partially depend on UNICODE (and in some cases _UNICODE) defined. Also there is a widespread use of TCHAR as well as function names that resolve to single-byte or wide character variants depending on whether UNICODE is defined or not.
However, in spite of appearances to the contrary, non-unicode builds are no longer supported. In new code declare wide strings as wchar_t * or WCHAR *. One may implicitly assume that all existing TCHAR would evaluate to WCHAR. For runtime and API calls use explicit function names instead of depending on UNICODE and/or _UNICODE are defined or not.
In particular, do not use _tprintf family of stdio functions. Instead, use wprintf or printf variants as required. Use "%ls" as the print format specifications for wide strings. For narrow strings, "%s" may be used in printf family of functions, but in wprintf family use "%hs". Do not use "%S".
For example, given the existing function,
MsgToEventLog(DWORD flags, LPCTSTR lpszMsg, ...);
a new code snippet that calls it would be written as
const wchar_t *username; const char *config; ... MsgtoEvenLog(M_SYSERR, L"Failed to authorize profile <%hs> for user <%ls>", config, username);
Uncrustify rules
The most fit tool for our purpose (because it can be told to just leave certain constructs alone, while changing others) seems to be
uncrustify -c dev-tools/uncrustify.conf tun.c
... but it has a zillion of options (600)... our config-in-the-making:
# Use Allman-style indent_columns=4 indent_braces=false indent_else_if=false indent_switch_case=4 indent_label=1 nl_if_brace=add nl_brace_else=add nl_elseif_brace=add nl_else_brace=add nl_else_if=remove nl_for_brace=add nl_while_brace=add nl_switch_brace=add nl_fdef_brace=add nl_do_brace=add sp_func_proto_paren=Remove sp_func_def_paren=Remove sp_func_call_paren=Remove sp_sizeof_paren=Remove # No tabs, spaces only indent_with_tabs=0 align_with_tabs=false cmt_convert_tab_to_spaces=true # Do not put spaces between the # and preprocessor statements pp_space=remove # Various whitespace fiddling sp_assign=add sp_before_sparen=add sp_inside_sparen=remove sp_cond_colon=add sp_cond_question=add sp_bool=add sp_else_brace=add sp_brace_else=add pos_arith=Lead pos_bool=Lead nl_func_type_name=add nl_before_case=true nl_assign_leave_one_liners=true nl_enum_leave_one_liners=true nl_brace_fparen=add nl_max=4 nl_after_func_proto=2 # Always use scoping braces for conditionals mod_full_brace_if=add mod_full_brace_if_chain=false mod_full_brace_while=add mod_full_brace_for=add mod_full_brace_do=add # Annotate #else and #endif statements mod_add_long_ifdef_endif_comment=20 mod_add_long_ifdef_else_comment=5 # Misc cleanup mod_remove_extra_semicolon=true # leave blank at end of empty for() statements sp_after_semi_for_empty=Add # Use C-style comments (/* .. */) cmt_c_nl_end=true cmt_star_cont=true cmt_cpp_to_c=true # Use "char **a"-style pointer stars/dereferences sp_before_ptr_star=Add sp_between_ptr_star=Remove sp_after_ptr_star=Remove sp_before_byref=Add sp_after_byref=Remove
(For reference: ident and astyle have been tried, but had either limitations or bugs that made them unusable for us.)
