Zabbix Code Guidelines

C coding guidelines

Standards

Even system and library functions that conform to POSIX.1-2001 cannot be used without checking (e.g, see ZBX-3937).

Formatting

Lines should not be longer than 120 characters.
Two or more consecutive empty lines should not be used.
Variable declarations should be followed by an empty line to separate them from the rest of the code:

struct utsname	name;
 
if (-1 == uname(&name))
        return SYSINFO_RET_FAIL;

Code that sets up the context for if and while constructs should be separated from if and while by a blank line, unless the body of if and while consists of a single statement:

package = strtok(buf, "\n");
 
while (NULL != package)
{
	...
 
	package = strtok(NULL, "\n");
}

goto labels should be written on a separate line, with no blank lines before and after:

	res = SUCCEED;
clean:
	zabbix_log(LOG_LEVEL_DEBUG, "End of %s():%s", __function_name, zbx_result_string(res));

When type casting, no space should be added after the type:

*lastlogsize = (long)buf.st_size;

Error messages should use “cannot” instead of “can't” or “could not”:

zabbix_log(LOG_LEVEL_WARNING, "cannot remove shared memory for collector [%s]", strerror(errno));

Hexadecimal constants should be written in lowercase:

#define MEM_MAX_SIZE	0x7fffffff

Variables and functions should be named with underscores between words:

int	calculate_item_nextcheck(zbx_uint64_t interfaceid, zbx_uint64_t itemid, int item_type,
		int delay, const char *flex_intervals, time_t now, int *effective_delay);

Use of camel case for naming variables in allowed only in Windows-specific code for consistency with Windows API:

PERFCOUNTER	*counterName = NULL;
DWORD		dwSize;

If a function is referenced in a comment, it should have parentheses after its name:

error = zbx_sock_last_error();	/* zabbix_log() resets the error code */

Debugging information containing a single sentence should not be capitalized and should not end with a period:

zabbix_log(LOG_LEVEL_CRIT, "failed assumption about pointer size (%lu not in {4, 8})", ZBX_PTR_SIZE);

Structure declarations should begin with typedef struct, followed by the structure body and type name zbx_<struct_name>_t:

typedef struct
{
}
zbx_data_t;

If needed for forward declarations the structure name should match the type name without _t suffix - zbx_<struct_name>:

typedef struct zbx_data
{
}
zbx_data_t;

Structure members should be separated by spaces, not tabs when initializing arrays of structures:

static zbx_vmcheck_t	vmchecks[] =
{
	{"cluster.discovery", VMCHECK_FUNC(check_vcenter_cluster_discovery)},
	{"cluster.status", VMCHECK_FUNC(check_vcenter_cluster_status)},
	{"version", VMCHECK_FUNC(check_vcenter_version)},
	{NULL, NULL}
};

There must be an empty line before return statement at the end of the function, unless it is the only line of function body:

static int      proc_get_process_uid(pid_t pid, uid_t *uid)
{
        char            tmp[MAX_STRING_LEN];
        zbx_stat_t      st;
 
        zbx_snprintf(tmp, sizeof(tmp), "/proc/%d", (int)pid);
 
        if (0 != zbx_stat(tmp, &st))
                return FAIL;
 
        *uid = st.st_uid;
 
        return SUCCEED;
}

Short enough label can be treated as empty string:

static int      smtp_debug_function(CURL *easyhandle, curl_infotype type, char *data, size_t size, void *userptr)
{
        const char      labels[3] = {'*', '<', '>'};
 
        if (CURLINFO_TEXT != type && CURLINFO_HEADER_IN != type && CURLINFO_HEADER_OUT != type)
                goto out;
 
        while (0 < size && ('\r' == data[size - 1] || '\n' == data[size - 1]))
                size--;
 
        zabbix_log(LOG_LEVEL_TRACE, "%c %.*s", labels[type], (int)size, data);
out:
        return 0;
}

Comments

Comments containing a single sentence should not be capitalized and should not end with a period:

/* check whether we need to update items_hk index */

Comments containing several sentences should be properly capitalized and end with a period:

/* The agent expects ALWAYS to have lastlogsize and mtime tags. Removing those would cause older agents to fail. */

Comment placement in regular code should indicate what the comment belongs to. For instance, a comment right before a single line of code or right before a block of consecutive lines indicates what this line or a block of lines does:

/* do not reallocate if not much is freed */
if (size > chunk_size / 4)
	return chunk;

If a comment pertains to the whole block between { and }, it should be written on a separate line at the top of the block.

if (0 != next_free)
{
	/* merge with next chunk */
 
	info->used_size -= chunk_size;
	info->used_size += size;
 
	...
}

If there is a comment at the end of a line after a statement, it should be separated from the statement by tabs:

free_perf_collector();	/* cpu_collector must be freed before perf_collector is freed */

In case of a multi-line comment treat every line as a single comment:

if (FAIL == result)
{
	/* since we have successfully sent data earlier, we assume the other */
	/* side is just too busy processing our data if there is no response */
	ret = NETWORK_ERROR;
}

If there is a need for a comment before a function, but all six parts (“Function”, “Purpose”, “Parameters”, “Return value”, “Author”, “Comments”) would be too much, use only the “Comments” part. Use of single-line comments before function definitions should be avoided:

/******************************************************************************
 *                                                                            *
 * Comments: counter is NULL if it is not in the collector,                   *
 *           do not call it for PERF_COUNTER_ACTIVE counters                  *
 *                                                                            *
 ******************************************************************************/
PDH_STATUS	zbx_PdhAddCounter(const char *function, PERF_COUNTERS *counter, PDH_HQUERY query, ...

Conditional statements

If expression is compared to a constant, the constant should be written on the left:

if (SUCCEED == str_in_list("last,prev", function, ','))

if (2 < num_param(param))

If a constant is used in a bitwise expression, it should be written on the right:

if (0x80 == (*text & 0xc0))

If a numeric value or a pointer is tested for being non-zero, write 0 or NULL explicitly:

if (NULL != message_esc && 0 != message_esc_len)

The only exception to the “constant on the left in comparison” rule is when a value is checked for being within certain bounds:

if ('1' <= *(br - 2) && *(br - 2) <= '9')

If there is an aesthetic need for parentheses in a ternary expression, put them around the whole expression, not just the conditional part:

is_numeric |= (SUCCEED == _wis_uint(cpe->szCounterName) ? 0x02 : 0);

In case conditional expression in an if takes several lines the logical operators should be written at the end of those lines and the following statement(s) should always be in braces. Also, second and further lines of the conditional expression should be indented with two tabs, not one:

if (0 == ioctl(s, SIOCGIFFLAGS, ifr) &&
		0 == (ifr->ifr_flags & IFF_LOOPBACK) &&
		0 == ioctl(s, SIOCGIFHWADDR, ifr))
{
	...
}

If a conditional statement features if, else, and maybe even else if, and at least one of the branches contains multiple statements, there should be braces around each branch. The exception is the last else where braces can be omitted:

if (0 == found)
{
	*curr = zbx_strpool_intern(new);
}
else if (0 != strcmp(*curr, new))
{
	zbx_strpool_release(*curr);
	*curr = zbx_strpool_intern(new);
}
else
	return FAIL;

switch statement labels should be written on a separate line, too:

switch (dcheck->type)
{
	case SVC_TELNET:
		service = "telnet";
		break;
	case SVC_ICMPPING:
		break;
	default:
		return FAIL;
}

There should be no empty line before break in a switch statement, even in larger case blocks:

switch (value_type)
{
	case ITEM_VALUE_TYPE_FLOAT:
		numeric = 1;
		break;
	case ITEM_VALUE_TYPE_STR:
		if (NULL == strval)
		{
			strval = zbx_strdup(val);
			zbx_free(val);
 
			if ('\0' != *strval)
			{
				if (SUCCEED != validate(strval))
					ret = FAIL;
			}
			else
				zabbix_log(LOG_LEVEL_DEBUG, "received empty value");
		}
		break;
	default:
		return FAIL;
}

default case is not obligatory in a switch statement:

switch (aggr_func)
{
	case ZBX_AGGR_FUNC_ONE:
	case ZBX_AGGR_FUNC_AVG:
		total += one_total;
		counter += one_counter;
		break;
	case ZBX_AGGR_FUNC_MAX:
		if (0 == process_num || one_counter > counter)
		{
			counter = one_counter;
			total = one_total;
		}
		break;
}

The break should not be used in a switch statement if it has no meaning :

switch (value_type)
{
	case ITEM_VALUE_TYPE_FLOAT:
		return "history";
	case ITEM_VALUE_TYPE_STR:
		return "history_str";
	case ITEM_VALUE_TYPE_LOG:
		return "history_log";
	default:
		assert(0);
}

Logging

In case log level is WARNING or less string quoting must be done using double quotes otherwise single quotes are preferred:

zabbix_log(LOG_LEVEL_WARNING, "cannot remove host \"%s\": host not found", host);
zabbix_log(LOG_LEVEL_DEBUG, "In %s() host:'%s' hostid:%d", __function_name, host, hostid);

Vectors

The vector implementation supports various operations such as sorting, uniqueness, searching.

When a vector is used to store any type of data, it is required to declare a purpose-specific vector type like:

ZBX_PTR_VECTOR_DECL(lld_rule_map, zbx_lld_rule_map_t *)
ZBX_PTR_VECTOR_IMPL(lld_rule_map, zbx_lld_rule_map_t *)

: Where zbx_lld_rule_map_t is the target structure, lld_rule_map is the valuable part of the target structure name that is used to create the vector type name.

For destroying structure-specific type of data and vector element:

zbx_vector_lld_rule_map_clear_ext(&lld_rules, free_lld_rule_map);

: Where free_lld_rule_map is a callback function which receives the pointer to vector element with the target type like:

void	free_lld_rule_map(zbx_lld_rule_map_t *rule);

: Note: for destroying only the vector element it is possible to use the default provided callback function:

void	zbx_ptr_free(void *data)

The use of zbx_vector_ptr_t vector type is acceptable in case of subject field or platform-dependent type of data structures only. When one type of structure has different content, depending on purpose-specific subject field or platform (Linux, Solaris, etc)

Macros

In macro definitions, #define should be followed by a space and the macro name should be followed by a tab:

#define ZBX_PROCESS_SERVER	0x01

In macro conditionals, #ifdef should be followed by a space:

#ifdef HAVE_LDAP

In nested macro conditionals, # character should remain the first character on the line, the rest should be separated by tabs:

#ifdef HAVE_LIBCURL
#	include <curl/curl.h>
#	ifndef HAVE_FUNCTION_CURL_EASY_ESCAPE
#		define curl_easy_escape(handle, string, length) curl_escape(string, length)
#	endif
#endif

However, if there is code inside the conditional the nested macros should not be indented:

#ifndef HAVE_SQLITE
 
static void	inc(int *counter)
{
	*counter++;
}
 
#define INC()	inc(&counter)
#endif

Conditional compilation should not affect indentation of regular code:

#ifndef _WINDOWS
int	get_cpustat(AGENT_RESULT *result, int cpu_num, int state, int mode);
#endif

Comments in macro conditionals (e.g. after #else or #endif) are only encouraged if it is not clear what the conditional refers to otherwise:

#ifdef _WINDOWS
#	include "perfmon.h"
#	include "perfstat.h"
#endif

#ifdef _WINDOWS
 
...long block...
 
#else	/* not _WINDOWS */
 
...long block...
 
#endif	/* _WINDOWS */

Using #ifdef or #ifndef for conditional statements are preferred over #if defined() and #if !defined(). However if there is a need for a more complex conditional blocks (e. g. “else if” conditions) using #if and #elif statements is allowed:

#ifdef _WINDOWS
	if (WSAEAFNOSUPPORT == zbx_sock_last_error())
#else
	if (EAFNOSUPPORT == zbx_sock_last_error())
#endif

#if defined(HAVE_IBM_DB2)
        int             i;
        SQLRETURN       ret = SQL_SUCCESS;
#elif defined(HAVE_ORACLE)
        sword           err = OCI_SUCCESS;
        ub4             counter;
#elif defined(HAVE_POSTGRESQL)
        char            *error = NULL;
#elif defined(HAVE_SQLITE3)
        int             ret = FAIL;
        char            *error = NULL;
#endif

It is highly preferred that a macro never ends with a semicolon so that when using it a semicolon is always appended:

#define zbx_stat(path, buf)	stat(path, buf)
...
zbx_stat(path, buf);</source>
Using macros without semicolon is allowed only if not possible otherwise:
<source lang=c>#define DBPATCH_START()                                 zbx_dbpatch_t   patches[] = {
#define DBPATCH_ADD(version, duplicates, mandatory)     {DBpatch_##version, version, duplicates, mandatory},
#define DBPATCH_END()                                   {NULL}};
...
DBPATCH_START()
 
DBPATCH_ADD(2010001, 0, 1)
DBPATCH_ADD(2010002, 0, 1)
...
 
DBPATCH_END()

In case of a macro with multiple operations each of them should be placed on a separate line. In order to ensure the macro integration in the code use do {…} while (0) loop:

#define zbx_free(ptr)           \
                                \
do                              \
{                               \
        if (ptr)                \
        {                       \
                free(ptr);      \
                ptr = NULL;     \
        }                       \
}                               \
while (0)

Preprocessor directives should not be used in macro arguments. For example, the following must be avoided:

result = DBselect(
		"select distinct h.hostid,h.host,o.type,o.scriptid,o.execute_on,o.port"
			",o.authtype,o.username,o.password,o.publickey,o.privatekey,o.command"
#ifdef HAVE_OPENIPMI
			",h.ipmi_authtype,h.ipmi_privilege,h.ipmi_username,h.ipmi_password"
#endif
		" from opcommand o,opcommand_grp og,hosts_groups hg,hosts h"
		" where o.operationid=og.operationid"
			" and og.groupid=hg.groupid"
			" and hg.hostid=h.hostid"
			" and o.operationid=" ZBX_FS_UI64
			" and h.status=%d"
		" union "
		...

Here, DBselect() is a macro. The behavior of this code is undefined.

For the purpose of type definition typedef existing_type new_type; should be preferred over #define new_type existing_type.

Compiler-specific issues

This section describes why we do not use full features of our programming language and current best practices because of incompatibility with older compilers and software.

Do not use %zu directive for outputting variables of type size_t. Use macro ZBX_FS_SIZE_T as the format specifier and macro zbx_fs_size_t as the type to convert to in calls to printf():

zabbix_log(LOG_LEVEL_DEBUG, "In %s() datalen:" ZBX_FS_SIZE_T, __function_name, (zbx_fs_size_t)j->buffer_size);

Older versions of glibc do not support z modifier:

zabbix_log(LOG_LEVEL_DEBUG, "In %s() datalen:%zu", __function_name, j->buffer_size);

When writing preprocessor directives, leave the # character is the first column.

#ifdef HAVE_LIBCURL
#	include <curl/curl.h>
#	ifndef HAVE_FUNCTION_CURL_EASY_ESCAPE
#		define curl_easy_escape(handle, string, length) curl_escape(string, length)
#	endif
#endif

Some compilers do not recognize the following code as valid:

#ifdef HAVE_LIBCURL
	#include <curl/curl.h>
	#ifndef HAVE_FUNCTION_CURL_EASY_ESCAPE
		#define curl_easy_escape(handle, string, length) curl_escape(string, length)
	#endif
#endif

When writing conditionally compiled code, avoid splitting if conditional into several parts:

#ifdef _WINDOWS
			if (PF_INET6 == current_ai->ai_family &&
				ZBX_TCP_ERROR == setsockopt(s->sockets[s->num_socks], IPPROTO_IPV6, IPV6_V6ONLY, (void *)&on, sizeof(on)))
#else
			if (PF_INET6 == current_ai->ai_family &&
				ZBX_TCP_ERROR == setsockopt(s->sockets[s->num_socks], SOL_IPV6, IPV6_V6ONLY, (void *)&on, sizeof(on)))
#endif

Some compilers do not recognize the following code as valid:

			if (PF_INET6 == current_ai->ai_family &&
#ifdef _WINDOWS
				ZBX_TCP_ERROR == setsockopt(s->sockets[s->num_socks], IPPROTO_IPV6, IPV6_V6ONLY, (void *)&on, sizeof(on)))
#else
				ZBX_TCP_ERROR == setsockopt(s->sockets[s->num_socks], SOL_IPV6, IPV6_V6ONLY, (void *)&on, sizeof(on)))
#endif

Avoid using complicated expressions (more complicated than simple assignment of a single value) in variable declarations. For instance, the following code is good:

char	*p_dst = NULL;

Some compilers have trouble compiling more complicated expressions like the following:

size_t	src_size = strlen(src);
int	feeds = src_size / maxline - (0 != src_size % maxline ? 0 : 1);

C99 features are not allowed in Zabbix agent code. For Zabbix server and proxy, C99 features should be avoided where possible. If the code benefits from better readability then designated initializers and compound literals can be used in Zabbix server and proxy code. Multiline compound literals must use braces indented by one tab:

token = (zbx_token_func_macro_t)
	{
		.macro		= { macro_pos, macro_pos_end },
		.func		= { func_pos, func_pos_end },
		.func_param	= { func_param_pos, func_param_pos_end }
	};

Best practices

It is advisable to write arithmetic expressions with as few floating-point operations as possible in order not to accumulate floating-point errors (e.g., ZBX-5717). For instance, the following

value_dbl = (100 * (host->cnt - host->rcv)) / (double)host->cnt;

should be preferred over

value_dbl = 100 * (1 - (double)hosts[h].rcv / (double)hosts[h].cnt);

Makefile.am

It is advisable to do linking a static module depend on configure parameter keys. For instance, the following

if HAVE_IPMI
zabbix_server_LDADD += ipmi/libipmi.a
endif