New plugin: synproxy #2381
New plugin: synproxy #2381
Conversation
There was a problem hiding this comment.
Hi @marekbecka,
thank you very much for your patch! I've left some review comments inline; mostly C style nits. The comments regarding the "type" names are important though.
Best regards,
—octo
src/synproxy.c
Outdated
| } | ||
|
|
||
| /* 1st column (entries) is hardcoded to 0 in kernel code */ | ||
| for (unsigned n = 1; n < 6; n++) { |
There was a problem hiding this comment.
Please don't hard-code 6 here. You can either use numfields or STATIC_ARRAY_SIZE(fields).
There was a problem hiding this comment.
using predefined macro SYNPROXY_FIELDS instead
src/synproxy.c
Outdated
| continue; | ||
| } | ||
|
|
||
| numfields = strsplit(buf, fields, STATIC_ARRAY_SIZE(fields)); |
There was a problem hiding this comment.
int numfields = strsplit();
// ^^^
src/synproxy.c
Outdated
| } | ||
|
|
||
| numfields = strsplit(buf, fields, STATIC_ARRAY_SIZE(fields)); | ||
| if (numfields != 6) { |
There was a problem hiding this comment.
Please don't hard-code 6 here; use STATIC_ARRAY_SIZE() instead, as above.
There was a problem hiding this comment.
using predefined macro SYNPROXY_FIELDS instead
src/synproxy.c
Outdated
| char *endptr = NULL; | ||
| errno = 0; | ||
|
|
||
| results[n].derive += strtoull(fields[n], &endprt, 16); |
There was a problem hiding this comment.
Do these values have the usual 0x prefix? If so, please use parse_value() instead:
if (parse_value(fields[n], &results[n], DS_TYPE_DERIVE) != 0) {
/* error has already been printed */
fclose(fh);
return -1;
}There was a problem hiding this comment.
Values are in hexadecimal without any prefix. Each line represents counters for one CPU core, like in another stats files /proc/net/stat/:
$ cat /proc/net/stat/synproxy
entries syn_received cookie_invalid cookie_valid cookie_retrans conn_reopened
00000000 000eca97 0004a680 000e7175 00000000 00000000
00000000 000ec960 0000cf44 000e6ed2 00000000 00000000
00000000 000ed4f4 0000866b 000e6f02 00000000 00000000
00000000 000ecd47 0000b461 000e6f99 00000000 00000001
00000000 000ecfe9 000611c4 000e736f 00000000 00000001
00000000 000ec4d6 0000c895 000e6d8f 00000000 00000000
00000000 000ec344 000079b3 000e6795 00000000 00000000
00000000 000ed5fb 00006602 000e72b4 00000000 00000002
src/synproxy.c
Outdated
| char *endptr = NULL; | ||
| errno = 0; | ||
|
|
||
| results[n].derive += strtoull(fields[n], &endprt, 16); |
There was a problem hiding this comment.
Why do you add to results[n].derive instead of assigning it?
There was a problem hiding this comment.
Target value is accumulated from separated per CPU counters.
src/synproxy.c
Outdated
| static void synproxy_submit(value_t *results) { | ||
| value_list_t vl = VALUE_LIST_INIT; | ||
|
|
||
| for (unsigned n = 1; n < 6; n++) { |
There was a problem hiding this comment.
Please repeat the below comment about index 0 being unused / empty.
Nit: We usually use size_t for iterating over arrays; some legacy code uses int.
src/synproxy.c
Outdated
| value_t results[6]; | ||
| int is_header = 1, status = 0; | ||
|
|
||
| fh = fopen(synproxy_stat_path, "r"); |
There was a problem hiding this comment.
When possible, please declare variables while defining them, i.e.:
FILE *fh = fopen();
// ^^^^^^
src/synproxy.c
Outdated
| if ((endptr == fields[n]) || errno != 0) { | ||
| ERROR("synproxy plugin: unable to parse value: %s", fields[n]); | ||
| status = -1; | ||
| goto err_close; |
There was a problem hiding this comment.
Please use
fclose(fh);
return -1;instead. Is has the same number of lines as
status = -1;
goto err_close;and avoids a goto.
src/synproxy.c
Outdated
| if (numfields != 6) { | ||
| ERROR("synproxy plugin: unexpected number of columns in %s", | ||
| synproxy_stat_path); | ||
| status = -1; |
There was a problem hiding this comment.
This is one of two places that modify status. If you do
fclose(fh);
return -1;here you can avoid that variable (and tracking that state) entirely.
src/types.db
Outdated
| @@ -232,6 +232,8 @@ spam_score value:GAUGE:U:U | |||
| spl value:GAUGE:U:U | |||
| swap value:GAUGE:0:1099511627776 | |||
| swap_io value:DERIVE:0:U | |||
| synproxy_connections value:DERIVE:0:U | |||
There was a problem hiding this comment.
Plugin-specific types are very discouraged. I recommend to use the existing connections type and rename the other type to simply cookies.
There was a problem hiding this comment.
More generic types connections and cookies are used in a new revision.
|
The CI fails because of an unrelated compilation issue, fixed in 2a86e83. I'm going to rebase the branch on master to resolve this. Once the CI system is happy, I'll merge. |
|
Thanks @marekbecka! |
This plugin provides statistics for Linux SYNPROXY available since 3.12. When kernel modules are loaded, the file with per CPU counters is created under /proc/net/stat/synproxy.