Coding standards used in IPMItool

Documentation

Magic numbers

No magic numbers are allowed. Ever. Always define a descriptively named macro and use it. Magic numbers are absolutely non-maintainable. Nobody but the author knows the meaning. Yes, comments can mitigate that. No, that's not a valid workaround.

This is BAD, a criminal offense, deserves capital punishment:

int
some_function(int param1, char param2)
{
 int localvar[27];
 ...
 localvar[16] = (param1 << 7) + 2;
 localvar[4] = (param2 * 3) >> 7 - 2; /* Are these the same 7 and 2 as above? */
 ...
 if (something) {
 return 19;
 }
 ...
 return 19; /* Is this the same 19 as above? What happens
 * if someone changes it here and forgets the above?
 */
}

This on the contrary is fine:

#define ENTITY_MAX_COUNT 27 /* There can't be more than 27 entities as
 * per Super Specification 2.0
 *//* See Super Specification 2.0, section 35.1 */
#define PARAM_SHIFT 7
#define PARAM_OFFSET 2
#define PARAM2_FACTOR 3

#define PARAM1_IDX 16 /* Super specification 2.0, section 35.4 */#define PARAM2_IDX 4 /* Super specification 2.0, section 35.5 */
typedef enum {
 MY_FINE = 0,
 ...
 MY_ERROR = 19,
 ...
} my_error_t;
my_error_t
some_function(int param1, char param2)
{
 int localvar[ENTITY_MAX_COUNT];
 ...
 localvar[PARAM1_IDX] = (param1 << PARAM_SHIFT) + PARAM_OFFSET;
 localvar[PARAM2_IDX] = (param2 * PARAM2_FACTOR) >> PARAM_SHIFT - PARAM_OFFSET;
 ...
 if (something) {
 return MY_ERROR;
 }
 ...
 return MY_ERROR;
}

Comments

At least a bit of documentation is required. Don't comment obvious things (e.g. do not repeat the code in English), but do explain the general idea of your code, the purpose of your variables, and the units they hold. Use descriptive names for your functions and variables.

This is bad:

int t; /* Integer variable t */

And this is good:

int timeout_ms; /* Time to wait (in ms) for the <action> to finish */

Entity names notation

Please stick to lower_snake_case for everything except MACROs (those use UPPER_SNAKE_CASE). Please use context-specific prefixes (like ipmi_ or oem_mycompany_, see below) for all globally visible entities. Please use _t suffix on typedefs:

typedef int[OEM_MYCOMPANY_SOME_DIMENSION] oem_mycompany_sometype_t;

Please avoid using Systems Hungarian notation or other type-specific prefixes or suffixes on variable names except when absolutely necessary. Apps Hungarian is acceptable when justified, but don't abuse it please.

/* BAD */
static
int my_helper_func()
{
 int icounter;
 char *sname;
 float fdivident;
 /* whatever */
}
/* OK */
static
int my_helper_func()
{
 int counter;
 char s_counter[BUFSIZE]; /* For holding the string representation
 * of the counter variable
 */
 char counter_str[BUFSIZE]; /* Another version */
 /* whatever */
}

Function and variable naming

Choosing names

Be descriptive:

/* BAD */
int a;
int myvar1;
int super;
char *ptr;
/* GOOD */
int ipmi_error_counter;
char *ipmi_response_buffer;

Choosing names for OEM-specific entities

For all exported symbols, that is: all non-static global variables, all non-static functions, and all typedefs and macros defined in header files, use oem_<oemname>_ or OEM_<OEMNAME>_ prefix. For non-exported (static symbols and macros defined in .c files) you may use any names you like. Remember that ipmi_ and IPMI_ prefixes may be used by main ipmitool code and other modules. Thus, using those prefixes is not recommended even for symbols with local visibility.

/* BAD for .h files */
#define MYMACRO 1245

int ipmi_my_super_variable;
int my_super_variable;
int ipmi_mycompany_descriptive_variable_name;
/* GOOD for both .h and .c files */
#define OEM_MYCOMPANY_DESCRIPTIVE_MACRO_NAME 1245

int oem_mycompany_descriptive_variable_name;
int oem_acme_get_platform_id(struct ipmi_intf *);
/* GOOD for .c files only */
#define MY_LOCAL_MACRO 4524
static int my_local_var;
static
void
my_helper_function() {
 ...
}

Case style

Please always use snake_case and never ever use CamelCase.

int IPMIvariableForSomePurpose; /* REALLY bad! */
int ipmi_variable_for_some_purpose; /* GOOD */

Indentation and alignment

Only tabulator is allowed for indentation, and spaces should be used for alignment within the same indentation level. Here the ---> symbol represents a tabulator, in further examples this symbol is not used, but tabulators are still implied for indents:

void
function(int arg)
{
--->int variable1 = initializer(param1, param2, param3,
---> param4, param5); /* Indent to the same level,
---> then align with spaces */
--->if (short_condition) { /* Short conditions, the curly is on the same line */
--->--->expression;
--->}
...
--->{
...
--->--->if (SOMETHING > arg && /* Multi-line conditions must */
--->---> SOMETHING_OTHER < arg && /* be aligned inside their */ 
--->---> COMPLETELY_OFF != arg) /* operator's indent level */
--->--->{ /* For multi-line conditions, the opening curly must
--->---> be aligned to `if` on a new line */
--->--->--->do_something(); /* Inner code is always 1 tab deeper */
--->--->}
...
--->}
...
}

This technique makes it irrelevant what size of tabstop is used, the code will always look properly;

Code width

Try to stick to line width of 80 chars considering tab size of 8 characters. If your code is too deeply indented and doesn't fit, it's a sign that it needs refactoring. Some exceptions, if justified, may be accepted though;

Code readability

Readability shouldn't be sacrificed over line width.

Trailing spaces

Absolutely no trailing white spaces or tabulators - no exceptions.

Function call formatting

There is no space between function name and parenthesis

memset (&data, 0, sizeof(data)); /* <- NO! */
memcpy(&dst, &src, sizeof (src)); /* <- NO! */

Returning from functions

Centralized exiting

A quote from Linux Coding Style as of kernel 4.20:

Albeit deprecated by some people, the equivalent of the goto statement is used frequently by compilers in form of the unconditional jump instruction.

The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done. If there is no cleanup needed then just return directly.

Choose label names which say what the goto does or why the goto exists. An example of a good name could be out_free_buffer: if the goto frees buffer. Avoid using GW-BASIC names like err1: and err2:, as you would have to renumber them if you ever add or remove exit paths, and they make correctness difficult to verify anyway.

The rationale for using gotos is:

• unconditional statements are easier to understand and follow
• nesting is reduced
• errors by not updating individual exit points when making modifications are prevented
• saves the compiler work to optimize redundant code away ;)

Return codes

Please always default to an error return code unless you're positive that everything is totally fine, in which case you explicitly set the success code before returning. Typically, such setting of the success code happens only once right before the first centralized exit label:

int function(void)
{
 uint8_t *buf;
 int rc = -1;
 buf = malloc(SOME_SIZE);
 if (!buf) {
 lprintf(LOG_ERROR, "Memory allocation error");
 goto out_nofree;
 }
 /* Do something here */
 ...
 if (some_failure) {
 goto out;
 }
 ...
 /* We come to this point only when all previous operations succeeded */
 rc = 0;
out:
 free_n(&buf);
out_nofree:
 return rc;
}

Function definition

Function return type shall be put on a separate line. If a function is static, the static keyword must be on a separate line as well and precede everything else.

static
struct ipmi_rs *
get_some_stuff()
{
/* some code here */
}
int
main()
{
 printf("Hello world!\n");
 return 0;
}

Conditionals formatting

General rule

An opening curly immediately follows the conditional operator. Contents of the code block are indented one level deeper than the conditional operator. The else and else if keywords, if required, are put on the same line with the closing curly and are immediately followed by the next opening curly:

if (!ptr) {
 /* do stuff */
} else if (ptr) {
 /* do something else */
} else {
 /* just in case */
}
 
/* This style of if-block is not preferred/discouraged */
if (i == 0)
 printf("Hello world!\n");
printf("Nice to meet you.");

Exceptions

The opening curly may be put on the line right after the closing parenthesis of the condition if the condition is multi-line. That is to improve readability:

while (some_boolean_function(with, some, parameters, that, is, long, enough)
 && some_other_quite_long_a_condtion
 || maybe_some_event_more_conditions)
{
 /* Curly on the above line visually separates the
 * condition from the code
 */
 code_goes_here();
 ...
}

Conditional switch blocks

Do not indent the case lines. Do not enclose case bodies in curlies. One case per line, please. If you need to have some code in a case and then fall through to the next case, use __attribute__ ((fallthrough)); and put an explicit /* fall through */ comment at the end of the falling case to indicate your intent to the reader and to the compiler.

Example:

/* Both styles are accepted, although the former requires more
 * indentation, therefore isn't suitable everywhere.
 */
switch (typecode) {
 case 0:
 /* comment */
 i = 0;
 break;
 case 1:
 i = 1;
 break;
}
/* Preferred */
j = 12;
switch (typecode) {
case 0:
 /* comment */
 i = 0;
 break;
case -1: __attribute__ ((fallthrough));
 j = 37;
 /* fall through */
case 1:
 i = 1;
 break;
}
/* Please, DON'T ... */
switch (foo) {
case 1: case 2: case 3:
 do_something();
}

If you ever find yourself in need of the following, then you're doing something wrong. Simply - don't and put "some code here" into a function.

switch (foo) {
case 1:
 {
 /* some code here */
 }
 break;
case 2:
 {
 /* some code here */
 }
 break;
 /* ... */
}

Comments

Unacceptable ways to comment the code

if (!ptr) { /* don't put comment here */
 return (-1);
}
/*****************
 * Don't draw frames!
 *****************/
/* Unacceptable way of writing
 multi-line comments
 */
// C++ comment - use only C-style comments!

Preferred ways to comment the code

if (!ptr) {
 /* put comment here */
 return (-1);
}
/* multi-line
 * comment
 */
uint8_t *ptr = NULL; /* in-line comments are not encouraged, but mostly ok */

Pointers

The following applies to all situations in the code:

uint8_t * data; /* WRONG: No free-floating asterisks */
uint8_t *data; /* CORRECT: Asterisk sticks to the variable name */

Casts

Don't put a space between a parenthesis and a variable name:

i = (int) j; /* WRONG */
i = (int)j; /* CORRECT */

Assertions

assert() - don't; really, why would you?

Memory allocation

malloc() - return value must be checked, always!

free() - DON'T ever use it. Use free_n(&ptr) instead to at once free the memory and set the pointer to NULL.

Example:

int
function(void)
{
 int rc = FUNCTION_FAILURE;
 void *ptr = NULL;
 ptr = malloc(/* size specifier */);
 if (!ptr) {
 /* Error handling */
 }
 ...
 if (/* error_condition */) {
 goto exit;
 }
 rc = FUNCTION_SUCCESS;
exit:
 free_n(&ptr);
 return rc;
} 

String to number conversion

atoi(), atol() - don't use these, use str2*() from lib/helper.c - no exceptions!

strto*() - use str2*(). If they don't fit your needs (yes, they aren't perfect), then it is obligatory to check errno and, if applicable, end_ptr after the call to strto*().

Architectures

ipmitool may be built for different architectures, so please always consider host endianness and word size.

Endianness and byteswapping

IPMI itself is mostly little-endian, so when dealing with IPMI commands you have to swap bytes for big-endian hosts. This is a common task, and all means for that have long been developed. Please do not reinvent the wheel. Use the existing ipmi*toh() and htoipmi*() functions. They exist for 32, 24 and 16-bit integers and are declared in ipmitool/helper.h.

Wrong:

 lprintf("Value: 0x%06x", (data[VAL_OFFSET] << 16 ) |
 (data[VAL_OFFSET + 1] << 8) |
 data[VAL_OFFSET + 2]);
 msg[len++] = session->out_seq & 0xff;
 msg[len++] = (session->out_seq >> 8) & 0xff;
 msg[len++] = (session->out_seq >> 16) & 0xff;
 msg[len++] = (session->out_seq >> 24) & 0xff;

Right:

 #include <ipmitool/helper.h> #include <inttypes.h> ...
 lprintf("Value: 0x%06" PRIx32, ipmi24toh(&data[VAL_OFFSET])); 
 htoipmi32(session->out_seq, &msg[len]);
 len += sizeof(uint32_t);

Printing out values

Some hosts have 32-bit words, some have 64-bit ones, who knows what other dragons may emerge in future. Integer format specifiers for printf(), such as %d or %l assume different input value width depending on the host architecture. What compiles well on x86_64 will produce a warning for 32-bit systems and vice versa. Thus, alway be safe and use C99's format specifiers when printing fixed-width integer values from ipmitool.

Don't:

 uint32_t val;
 ...
 lprintf("Value: %lu", val);

Do:

 #include <inttypes.h> ...
 uint32_t val;
 ...
 lprintf("Value: %" PRIu32, val);

Print outs, error messages, etc.

/* STDOUT */
printf("Hello world!\n");
/* STDERR - error messages. Note, there is no "\n". */
lprintf(LOG_ERR, "Printer is on fire!");
/* Notice worthy message - warning. */
lprintf(LOG_NOTICE, "Don't forget to check cooker!");
 
void
printf_help()
{
 lprintf(LOG_NOTICE,
"This is OK in a function's top level...");
 lprintf(LOG_NOTICE,
"to get alignment and check length of multi-line print-outs.");
}
int
main()
{
 /* Some code */
 if (!rsp) {
 lprintf(LOG_ERR,
"But it isn't ok on a nested level or for single-line prints.");
 return (-1);
 }
 return 0;
}
## Types

Standardized types from _stdint.h_ should be preferred over generic C types.
This is mandatory for structures!
## Variable declaration

```c
/* Please, avoid multiple variables declaration with a single type specifier! */
int i = 0, validread = 1, thresh_available = 1;

"Pretty" formatting

Despite it looks pretty, it's rather annoying when the code has to be changed. Therefore don't.

/* DO NOT */
int foo = 1;
int foobar = 2;
int i = 3;

The only exception is multi-dimension array or array of structures initialization where the maximum length of all fields except for the last one is known. But better don't even in this case:

struct some_struct {
 uint8_t a;
 uint16_t b;
 unsigned char *s;
};
struct some_struct[SOME_COUNT] = {
 { 1, 65535, "Some string here" },
 /* ... */
 { 255, 325, "Some very long string here" },
}