It is a good idea to split compiler and linker flags, but it is not an optimal solution to split them per component (server, proxy, agent, sender, get).

Best solution would be to split CFLAGS per third-party library and take into account specific LIBRARY_CFLAGS only in those Zabbix compilation targets which use the library directly. For example, DB_CFLAGS are specific for libzbxdb.a and TLS_CFLAGS are specific for libzbxcrypto.a. These static libraries should ideally create an abstraction layer for database access and encryption for other parts of Zabbix code. Such approach will minimize the probability of header name conflicts between different third-party libraries (see ZBX-4069). Including third-party headers as locally as possible will help us to avoid name space pollution (see ZBX-10375).

As to LDFLAGS and LIBS, these are linker flags and therefore can be split per component at best. Splitting LIBS per component is totally fine because we don't want to make any component dependent on shared libraries it will not use in runtime. This also saves a tiny fraction of compilation time.

But per component LDFLAGS and multiple LIBRARY_LDFLAGS can lead to non-obvious results. Using common LDFLAGS for all components and throughout library checking process during ./configure can be a better option.

Consider a situation when two versions of the library are present in the system (see ZBX-4397 and ZBX-8387), for example outdated GnuTLS in /usr/lib (or similar common path) and up-to-date one in /home/user/gnutls. We want Zabbix to use the latter, so:

./configure --enable-agent --enable-server --with-mysql --with-gnutls=/home/user/gnutls

It is very likely that ./configure will find MySQL client library in the same common path where the outdated GnuTLS is installed. It saves -L/usr/lib in DB_LDFLAGS and proceeds with checking GnuTLS library. It finds one in provided path /home/user/gnutls, version is OK, so -L/home/user/gnutls is saved as TLS_LDFLAGS. ./configure succeeds, now make...

Agent binary will be built with specified version of GnuTLS library, because agent does not need DB_LDFLAGS. But linker will fail to link server binary. Linker is provided with DB_LDFLAGS and TLS_LDFLAGS and it will search specified directories for necessary libraries in the order directories were specified. It will find outdated GnuTLS in /usr/lib first.

If the user is advanced enough to know about LD_FLAGS he might try

LD_FLAGS=-L/home/user/gnutls ./configure --enable-agent --enable-server --with-mysql --with-gnutls=/home/user/gnutls

but it will not work either because we use DB_LDFLAGS and TLS_FLAGS as target_LD_FLAGS which have higher priority than ordinary LD_FLAGS. So the server linker gets DB_LDFLAGS then TLS_FLAGS then LD_FLAGS.

The only option left is to export SERVER_LDFLAGS before ./configure but this is really-really-really non-obvious solution and since it is not documented anywhere user will need to investigate configuration process himself.

If we had not split LD_FLAGS per component or per library ./configure would report outdated version of GnuTLS and user could get a hint and use LD_FLAGS to alter directory order for linker.

Fix available in development branch svn://svn.zabbix.com/branches/dev/ZBX-4148 revision 58533.

Library-specific CFLAGS are now propagated to makefiles of those targets which include respective library headers. One exception is libcurl. It is used in a few very distant places, so duplicating {{#include}}s are not convenient. Since we use libcurl or nothing wrapping all used libcurl functionality would produce not necessary overhead.

LDFLAGS were left untouched. Required changes to fix ZBX-4397 and ZBX-8387 proved to be too complicated and out of scope of this issue. However, I have come up with a solution. I will add a comment in ZBX-4397.

(1) We have several naming styles mixed up when using structure name for forward declarations:

src/libs/zbxdbcache/dbconfig.c:typedef struct zbx_dc_trigger_deplist_s
src/libs/zbxdbcache/dbcache.c:typedef struct zbx_hc_data_t
src/libs/zbxdbcache/valuecache.c:typedef struct _zbx_vc_chunk_t
src/libs/zbxcommon/misc.c:typedef struct zbx_scheduler_filter_t
src/libs/zbxcommon/misc.c:typedef struct zbx_scheduler_interval_t
src/zabbix_server/poller/checks_ipmi.c:typedef struct zbx_ipmi_host_s
include/comms.h:typedef struct ZBX_TLS_CTX	*zbx_tls_context_t;

Accordingly to freshly updated guidelines we should keep the following style:

typedef struct zbx_<name>
{
}
zbx_<name>_t;

The zbx_tls_context_t structure definition should be changed to follow the coding guidelines. While at it would be good to update structure definitions in the other places (only where the type is defined as zbx_<name>t and structure name is not defined as zbx<name>).

Glebs Ivanovskis Do we want to fix naming for DB_RESULT too?
RESOLVED for ZBX_TLS_CTX in r58777, r58836, r58837.

Andris Zeila The following structures still does not follow the coding guidelines:

src/libs/zbxdbcache/dbconfig.c:typedef struct zbx_dc_trigger_deplist_s
src/libs/zbxdbcache/dbcache.c:typedef struct zbx_hc_data_t
src/libs/zbxdbcache/valuecache.c:typedef struct _zbx_vc_chunk_t
src/libs/zbxcommon/misc.c:typedef struct zbx_scheduler_filter_t
src/libs/zbxcommon/misc.c:typedef struct zbx_scheduler_interval_t
src/zabbix_server/poller/checks_ipmi.c:typedef struct zbx_ipmi_host_s

REOPENED

Glebs Ivanovskis RESOLVED in r59048.

(2) Suggestion to rename the ZBX_TLS_CTX structure tls_ctx fields to simply ctx or context. s->tls_ctx->ctx or s->tls_ctx->context would have better readability than s->tls_ctx->tls_ctx

Glebs Ivanovskis Went for s->context->tls_ctx. Perhaps, TLS_CTX(s) would me even more readable, but too OOP.

RESOLVED in r58777, r58836, r58837.

Andris Zeila CLOSED

(3) Regarding the following functions:

ssize_t	zbx_tls_write(zbx_socket_t *s, char **error, const char *buf, size_t len);
ssize_t	zbx_tls_read(zbx_socket_t *s, char **error, char *buf, size_t len);

Error ouput variable is usually the last function parameter.

Glebs Ivanovskis Should we take care of these functions as well?

int	zbx_tls_connect(zbx_socket_t *s, char **error, unsigned int tls_connect, char *tls_arg1, char *tls_arg2);
int	zbx_tls_accept(zbx_socket_t *s, char **error, unsigned int tls_accept);

Andris Zeila I'd vote for it. Let's see what Aleksandrs Saveljevs has to say about it.

Aleksandrs Saveljevs No objections.

Glebs Ivanovskis RESOLVED in r58778.

Andris Zeila CLOSED

(4) Compilation errors on Windows (without encryption):

comms.o : error LNK2019: unresolved external symbol _zbx_tls_write referenced in function _zbx_tcp_send_ext
comms.o : error LNK2019: unresolved external symbol _zbx_tls_read referenced in function _zbx_tcp_recv_ext

Glebs Ivanovskis Reason for this is that commonly used zbx_tls_read() and zbx_tls_write() are now defined in src/zbxcrypto/tls.c which is compiled on Windows only if there is need for encryption. We want to keep zbx_tls_read/write() in tls.c for modularity, but there is purely unencrypted part in them. Aleksandrs Saveljevs, Andris Mednis, please help us choose between two options:

1. enable compilation of tls.c on Windows for building without encryption support;
2. take out unencrypted part of zbx_tls_read/write() and put it into comms.c (let's call them zbx_tcp_read/write()).

Glebs Ivanovskis I prefer the second option.
RESOLVED in r58797.

Andris Zeila Compiler warnings without encryption:

tls.c: In function ‘zbx_tls_write’:
tls.c:5248:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
tls.c: In function ‘zbx_tls_read’:
tls.c:5368:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

REOPENED

Glebs Ivanovskis RESOLVED in r58875 and r58926.

(5) Warnings with Net-SNMP library:

discoverer.c: In function ‘discoverer_thread’:
discoverer.c:777:2: warning: implicit declaration of function ‘init_snmp’ [-Wimplicit-function-declaration]
  init_snmp(progname);
  ^
poller.c: In function ‘poller_thread’:
poller.c:832:3: warning: implicit declaration of function ‘init_snmp’ [-Wimplicit-function-declaration]
   init_snmp(progname);
   ^

RESOLVED in r58838.

Andris Zeila CLOSED

(6) Zabbix agent crashes when trying to process encrypted connection. This happens because tls_ctx is freed in zbx_socket_free() which is called at the start of zbx_tcp_recv_ext() function.

Glebs Ivanovskis RESOLVED in r58925 and r59018.

Andris Zeila CLOSED

(7) Possible improvements in polarssl related code:

Now we always allocate zbx_tls_context_t and polarssl context when initializing or accepting tls connection:

s->tls_ctx = zbx_malloc(s->tls_ctx, sizeof(zbx_tls_context_t));
s->tls_ctx->ctx = zbx_malloc(NULL, sizeof(ssl_context));

and free them together.

ssl_free(s->tls_ctx->ctx);
zbx_free(s->tls_ctx->ctx);
zbx_free(s->tls_ctx);

With encryption related data structures wrapped in zbx_tls_context_t we can drop dynamical allocation of polarssl context.

Another thing - in gnutls and openssl code there is a pattern to have separate label for resource deinitialization. In polarssl code the resources are freed 'in place' and the a goto to out label is performed. This results in multiple places where the resources are freed. It might be worth to use similar resource deinitialization pattern as in gnutls and openssl code.

Glebs Ivanovskis I tried to implement s->tls_ctx->ctx as statically allocated ssl_context, it does not look pretty. Seems that all ssl_*() functions want ssl_context *, therefore we will have a lot of &s->tls_ctx->ctx for polarssl instead of s->tls_ctx->ctx for gnutls and openssl. And we will have s->tls_ctx->ctx.smth->smth instead of s->tls_ctx->ctx->smth->smth. For the sake of aesthetics, the first part WON'T FIX

Completely agree with the second part. It will be a step towards library-dependent-code unification. The second part RESOLVED in r59073.

Andris Zeila CLOSED

Successfully tested

Fixed in pre-3.0.2rc1 r59085, pre-3.1.0 (trunk) r59083.

(8) Compilation without unixODBC is broken.

Glebs Ivanovskis RESOLVED in r59120.

Andris Zeila CLOSED

(9) Resource leak found by Coverity (CID 143063). It was introduced before this development but was on DB backend side and therefore was not visible to Coverity. Now we allocate memory in DBselect() in case of MySQL and all DBselect()'s without matching DBfree_result() will be reported.

Glebs Ivanovskis RESOLVED in r59117.

Andris Zeila CLOSED

Succesfully tested

Fixed in pre-3.0.2rc1 r59207, pre-3.1.0 (trunk) r59208.