Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support configurable syslog facilities #5792

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@thiagoftsm
Copy link
Contributor

commented Apr 3, 2019

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

@thiagoftsm thiagoftsm requested review from cakrit and ktsaou as code owners Apr 3, 2019

@cakrit
Copy link
Contributor

left a 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_id again. 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.

@thiagoftsm thiagoftsm requested a review from mfundul as a code owner Apr 3, 2019

@thiagoftsm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

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
Copy link
Contributor

left a comment

Fast work, good stuff. I haven't tested it yet. Please do the minor fixes I noted as well and I'll test.

@@ -355,6 +355,10 @@ void log_init(void) {
snprintfz(filename, FILENAME_MAX, "%s/access.log", netdata_configured_log_dir);
stdaccess_filename = config_get(CONFIG_SECTION_GLOBAL, "access log", filename);

char deffacility[8];
snprintfz(deffacility,7,"%s","daemon");
facility_log = config_get(CONFIG_SECTION_GLOBAL, "facility log", deffacility);

This comment has been minimized.

Copy link
@cakrit

cakrit Apr 4, 2019

Contributor

Why not make facility_log int and put the call to log_facility_id here as well? Then you only call that function once.

This comment has been minimized.

Copy link
@thiagoftsm

thiagoftsm Apr 4, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@cakrit

cakrit Apr 5, 2019

Contributor

Let me spell it out.

  • Function open_log_file is 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_id from open_log_file at 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.

This comment has been minimized.

Copy link
@thiagoftsm

thiagoftsm Apr 5, 2019

Author Contributor

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.

Show resolved Hide resolved libnetdata/log/log.c Outdated
syslog_init();

syslog_init(log_facility_id(facility_log));
//syslog_init();

This comment has been minimized.

Copy link
@cakrit

cakrit Apr 4, 2019

Contributor

Just remove it.

thiagoftsm added some commits Apr 4, 2019

@cakrit cakrit changed the title Solution for issue 5717 Support configurable syslog facilities Apr 5, 2019

@cakrit

cakrit approved these changes Apr 5, 2019

Copy link
Contributor

left a comment

Tested with facility local3, ran with valgrind, it works.

@cakrit cakrit merged commit e77241c into netdata:master Apr 5, 2019

9 of 12 checks passed

Header rules - netdata No header rules processed
Details
Pages changed - netdata 2 new files uploaded
Details
Redirect rules - netdata No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
Mixed content - netdata No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
netlify/netdata/deploy-preview Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.