openvpn: reworked status file parser #2352
Conversation
1a79b57 to
e30ec78
Compare
|
I have checked this on my setups. Status files of 'SINGLE' , 'MULTI' version 1, version 2, version 3 formats from OpenVPN-2.2; 'MULTI' version 2 status file from OpenVPN-2.4 was checked. @octo, @rubenk - I offer this already polished PR to your attention. Let's review and merge it? |
make all-am
make[1]: Entering directory '/home/ruben/src/collectd'
CC src/snmp_agent_la-snmp_agent.lo
In file included from ./src/daemon/common.h:33:0,
from src/snmp_agent.c:31:
src/snmp_agent.c: In function ‘snmp_agent_dump_data’:
src/snmp_agent.c:42:21: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
#define PLUGIN_NAME "snmp_agent"
^
./src/daemon/plugin.h:400:42: note: in definition of macro ‘DEBUG’
#define DEBUG(...) plugin_log(LOG_DEBUG, __VA_ARGS__)
^~~~~~~~~~~
src/snmp_agent.c:192:13: note: in expansion of macro ‘PLUGIN_NAME’
DEBUG(PLUGIN_NAME ": OID[%d]: %s", i, oid_str);
^~~~~~~~~~~
src/snmp_agent.c:192:35: note: format string is defined here
DEBUG(PLUGIN_NAME ": OID[%d]: %s", i, oid_str);
~^
%ld
CCLD snmp_agent.la
af9df00 to
7350e97
Compare
There was a problem hiding this comment.
Hi @rpv-tomsk,
thank you very much for your patch! It looks great overall! One issue is that we removed ssnprintf() in the mean time and since I had the review started, I've left a couple more style nits … sorry ;-)
Best regards,
—octo
src/openvpn.c
Outdated
|
|
||
| int status; | ||
|
|
||
| ssnprintf(callback_name, sizeof(callback_name), "openvpn/%s", status_name); |
There was a problem hiding this comment.
ssnprintf() has been removed (be12604), use snprintf() instead.
src/openvpn.c
Outdated
| temp = malloc(sizeof(*temp)); | ||
| if (temp == NULL) { | ||
| /* create a new vpn element */ | ||
| instance = malloc(sizeof(*instance)); |
src/openvpn.c
Outdated
| value); | ||
| return 1; | ||
| } | ||
| vpn_status_t *instance; | ||
|
|
||
| status_file = sstrdup(value); | ||
| if (status_file == NULL) { |
There was a problem hiding this comment.
That doesn't make sense, since the difference between strdup() and sstrdup() is that the latter exits. I'd prefer to switch to strdup() here (and leave the error handling). Unfortunately I haven't found the energy to remove the uses of sstrdup() globally yet.
src/openvpn.c
Outdated
| * better play it save. */ | ||
| if ((filename == NULL) || (*filename == 0)) | ||
| return 0; | ||
| vpn_status_t *st; |
There was a problem hiding this comment.
Declare and initialize in one line, if possible:
vpn_status_t *st = user_data->data;
src/openvpn.c
Outdated
| long long sum_users = 0; | ||
|
|
||
| int found_header = 0; |
* Added detection of status fields index by parsing 'HEADER' lines. This allows to support status files from latest OpenVPN-2.4 and possible from future versions; * Fixed a bug with OpenVPN status file version 3 where delimiter is 'tab' (not space or comma); * Status file version detection was moved from configuration callback to read callback; * Now 'plugin_register_complex_read()' is used for registering read callback. Closes: collectd#2128
7350e97 to
7744290
Compare
|
Hi, Florian! Glad to get your feedback! The patch is rebased to current master, 'nits' are fixed, code passed through clang-format. |
7744290 to
5dcc047
Compare
This allows to support status files from latest OpenVPN-2.4 and possible from future versions;
Closes: #2128