wiki:CodeStyle

Version 26 (modified by Selva Nair, 6 years ago) (diff)

See commit 9ba36639abcac4367c8227d2dd87b18fb56267c4

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.
  • 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.

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 should 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);

Crustify 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 config 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=1nl_if_brace=add
nl_brace_else=add
nl_elseif_brace=add
nl_else_brace=add
nl_else_if=remove
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

# 80 character line width
code_width=80
ls_code_width=false

# 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

# 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

# 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.)