Support configurable syslog facilities#5792
Conversation
cakrit
left a comment
There was a problem hiding this comment.
Nice and clean.
Two issues I found and one hint:
- Issue 1: It doesn't compile with cmake (but compiles fine with make):
/usr/bin/ld: CMakeFiles/libnetdata.dir/libnetdata/log/log.c.o: in function `open_log_file':
/home/christopher/CLionProjects/netdata/libnetdata/log/log.c:365: undefined reference to `netdata_config'
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/cgroup-network.dir/build.make:122: cgroup-network] Error 1
make[1]: *** [CMakeFiles/Makefile2:184: CMakeFiles/cgroup-network.dir/all] Error 2
- Since we only have one config setting for the facility, there's no reason to go through the initialization and call
log_facility_idagain. Just use a static int, initialized to -1 and go through the steps once.
Hint: You could use a hash function to avoid all those strcmps and instead compare integers first. It's not a big deal in this case because we only open a few log files. When you use a static int, you'll only go through the comparisons once, so hashing is not needed here. But search for hash in health/health_config.c and you'll see what I mean.
|
Initially I compiled with the traditional steps(./configure && make && make install ), so I could not identify the problem that happens when we use script. After to use the script, I had the same problem and I could fix using the following steps: I changed the libnet/log/log.c adding the variable facility_log and I put an extern reference to it inside the file libnet/log/log.h . After this I moved few lines of code that were inside the function open_log_file to the daemon/main.c, so I am reading the option "facility log" there now. I accepted all the suggestions given and I wrote the body of the function log_facility_id using hashes. I agree 100% that we could consider this "not a big deal", but when we remove the strcmp we avoid some call instructions and no less important, I could learn more about the source code. I decided to use simple_hash instead simple_uhash, because I arrived in the conclusion it is the correct case here, please can you confirm this? Finally, to save some byte codes, I commented the function(log_facility_name) that is not been used anywhere. |
cakrit
left a comment
There was a problem hiding this comment.
Fast work, good stuff. I haven't tested it yet. Please do the minor fixes I noted as well and I'll test.
|
|
||
| char deffacility[8]; | ||
| snprintfz(deffacility,7,"%s","daemon"); | ||
| facility_log = config_get(CONFIG_SECTION_GLOBAL, "facility log", deffacility); |
There was a problem hiding this comment.
Why not make facility_log int and put the call to log_facility_id here as well? Then you only call that function once.
There was a problem hiding this comment.
I do not see any problem to use an integer, but it will be necessary to export log_facility_id. I did not use integer initially, because yesterday when we talked about the libnetdata/log/log.c you said me "Since we only have one config setting for the facility, there's no reason to go through the initialization and call log_facility_id again. Just use a static int,", after read this I supposed that we had to keep this function static, so I decided to use a pointer to get the configuration and carry it to the log.
There was a problem hiding this comment.
Let me spell it out.
- Function
open_log_fileis called at least three times, for access.log, error.log, debug.log and it's also called we reopen the log files (I don't remember exactly where, you can check the usages of this function). - Since we unconditionally initialize syslog each time filename="syslog", we are also making three calls to
log_facility_id. - However, there's a single input and a single output to that function, as we only have a single config parameter to control the facility. So there's no point in calling it again, with the exact same input.
There are a few ways to prevent this:
- Put a static int ihavealreadydonethis either in open_log_file or in log_facility_id. The first time we enter it's set e.g. to -1, the second to something else. We check its value and based on that, we don't go through the checks again. There are many variations of this solution.
- Don't call
log_facility_idfromopen_log_fileat all. Do instead:
facility_log = log_facility_id(config_get(CONFIG_SECTION_GLOBAL, "facility log", deffacility));
and do syslog_init(facility_log) in open_log_file. Of course then facility_log needs to be an int.
The older comments were a bit vague, so I get why you didn't understand them.
There was a problem hiding this comment.
Now I could understand the previous request. I found another solution and I decided use it. Inside the function syslog_init we already have an "static int i", so the function openlog is called only one time, so I returned the syslog_init for the previous format and I put the call of the function log_facility_id inside the openlog.
| syslog_init(); | ||
|
|
||
| syslog_init(log_facility_id(facility_log)); | ||
| //syslog_init(); |
cakrit
left a comment
There was a problem hiding this comment.
Tested with facility local3, ran with valgrind, it works.
* Changes in libnetdata/log/log.c to append facility * Fixing compilatioN * Fixing compilation problem! * Remove comments and adjust indentation inside log * Update log.c * Elimination of unnecessary calls to function
Summary
Fixes #5717
In the link #5717 there was a request for the system admin to select the facility that he wanna have in syslog. In this commit I am creating two functions and I am also changing two to solve the described issue.
The function log_facility_id is the unique that is really necessary to fix the problem. When I created it I used all the facilities that are listed in the operate system headers, including the facility that is not available(mark) and it was commented due this or it is considered deprecated(security). This situation happens, because I am considering possible mistakes in the configure file. When someone chooses a facility that is not available, I am starting the syslog with the default that was present in the log.c previously.
The function log_facility_name was created to keep the same pattern that I found inside the file web/server/web_server.c . This function is not necessary to fix the problem, but it helps with possible future features.
The function syslog_init now has an argument, the argument is an integer that represents the facility that will be set.
Finally I had to append few lines of code inside the function open_log_file. I am considering that the config file will have a configuration variable "facility log" for the user to set the facility that is wished. I called three functions in the same line, instead to create variables to store the results, because the return of the called function is direct the input for the new function, so I am considering that there is not any necessity to extract results from the registers to put in memory addresses.
Component Name
I am changing the file libnetdata/log/log.c
Additional Information