Fix /proc/mounts parsing & eliminate debug log noise

[overhacked]

1. The code that parses /proc/mounts depended on certain words and spaces that are not present in every *nix distro, so I rewrote using zbx_regexp_sub() to a more generic regex. (n.b. It would have been simpler to use strtok(), but I wasn't sure whether thread-safety was required in this context. I stuck to Zabbix library functions to ensure I didn't introduce any conflicts, and there is no string tokenizer that I could find in the Zabbix library.)
2. I added a stub zbx_module_history_write_cbs(void) to the module, as I was seeing errors in the log at DebugLevel=4. The Zabbix docs for module creators don't say that this function is optional, so I went ahead and implemented it as returning all NULL values in case it becomes a hard requirement in future versions.

[jangaraj]

Hi Ross,

Thank you for your PR 👍 . It needs some testing first.

1.) zbx_regexp_sub()
Is that function available on all 3.0+ agents?

2.) /proc/mounts
Which distro have had a problem with current implementation? Which distros did you use for new regexp testing?

3.) zbx_module_history_write_cbs
Let me ask about recommendation from Zabbix devs? How it works with Zabbix 3.0 agent, where history functions are not expected in the module?

[overhacked]

Hi, thanks for the feedback!

1. zbx_regexp_sub() is present in the currently-available download of Zabbix 2.4.8, so it is at least that old.

2. The problem I'm having is on Gentoo Linux running kernel 4.9.35. I can't find any official kernel.org documentation for /proc/mounts or /proc/self/mounts, but it appears to be generated by the kernel in function show_vfsmnt() of proc_namespace.c. The problem in the existing parsing code is that it seems to assume that the leading devname field before the cgroup path is 'cgroup ', but seems on some distros/kernel versions it actually reads 'cpuset '. E.g.

   cpuset /sys/fs/cgroup/cpuset cgroup rw,nosuid,nodev,noexec,relatime,cpuset 0 0
   

   vs

   cgroup /sys/fs/cgroup/cpuset cgroup rw,nosuid,nodev,noexec,relatime,cpuset 0 0
   

   So the current code leaves the 'cpuset ' in place, extracts the everything before the first space character, then removes the word 'cpuset' from the result, leaving an empty string. My regex is an attempt to abstract away the actual wording of any of the fields and just get at the second field, as long as it begins with a slash. As I said, a string tokenizer would be simpler, but I wanted to be extra-conscious of thread safety.

   I could make a small modification to the regular expression to make the first field optional, in case there are some kernels that do not return a devname, if you think that would be prudent, but the current kernel code appears written to always return something in that position. Alternatively, one could throw caution to the wind and just find the first space-delimited string that begins with '/', which would be future-proof unless somehow the devname for cgroups became /dev/cgroup/whatever.

3. I do see now in section 12.5.2.2 of the Zabbix 3.2 docs that zbx_module_history_write_cbs() is an optional interface, so maybe it is more of a Zabbix bug that it logs its absence, but that section farther down states:

   This function should return callback functions Zabbix server or proxy will use to export history of different data types. Callback functions are provided as fields of ZBX_HISTORY_WRITE_CBS structure, fields can be NULL if module is not interested in the history of certain type.

   So, I figure returning all NULLs is allowed. :-) All these modifications have been tested in Agent 3.2.7, which doesn't seem at all bothered by the presence of the null history callbacks.

[jangaraj]

2.) I'm building my own Gentoo for testing. It's good idea to have some VM with 4.9 kernel. Usually I work with older kernels, so probably that's a reason why I didn't see this problem before. I need to test it also with older kernels, which are used by majority distros.

3.) Yes, it's optional and that message is not error, it's only debug/info. Zabbix dev:

Documentation says zbx_module_history_write_cbs() is optional.
I wouldn't worry about a single informative message on Zabbix startup at LogLevel=4

Could you remove zbx_module_history_write_cbs part from PR please?

[overhacked]

Just removed the zbx_module_history_write_cbs() function, and re-pushed a single commit for the regex-using code. I squashed a small bug fix in that I had redeclared the stat_dir variable local to the zbx_docker_dir_detect() function, breaking things for the rest of the module, so I removed that line.

[jangaraj]

Sorry for a delay. LGTM.

CentOS test:

[root@dockerhost ~]# uname -a
Linux dockerhost 3.10.0-514.26.2.el7.x86_64 #1 SMP Tue Jul 4 15:04:05 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
[root@dockerhost ~]# zabbix_agentd -V
zabbix_agentd (daemon) (Zabbix) 3.4.1
Revision {ZABBIX_REVISION} 28 August 2017, compilation time: Sep 20 2017 20:21:46

Gento test:

localhost ~ # uname -a
Linux localhost 4.12.5-gentoo #1 SMP Fri Aug 18 10:32:47 BST 2017 x86_64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz GenuineIntel GNU/Linux
localhost ~ # zabbix_agentd -V
zabbix_agentd (daemon) (Zabbix) 3.4.1
Revision {ZABBIX_REVISION} 28 August 2017, compilation time: Sep 20 2017 21:03:33

And thank you again.