Log levels implementation #477
Conversation
It compiles but this is not really tested.
| @@ -52,6 +52,14 @@ struct _PolkitBackendAuthority | |||
| GObject parent_instance; | |||
| }; | |||
|
|
|||
| enum | |||
| { | |||
| LOG_LEVEL_ERROR, // log errors only | |||
There was a problem hiding this comment.
Even if other levels are not used, please add support for all syslog levels, so that we can add LogControl1 support later: https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.LogControl1.html (I can wire that up once this is merged)
There was a problem hiding this comment.
Also, the exact same levels as in syslog should be used. This is because those priorities will be propagated to the entries in the journal, and ideally there is a clear mapping between what is visible here in the code and what the journal allows. Doing a 1:1 mapping is the most reasonable.
data/polkit.service.in
Outdated
| ExecStart=@libprivdir@/polkitd --no-debug | ||
| ExecStart=@libprivdir@/polkitd --no-debug --log-level=2 |
There was a problem hiding this comment.
Does --no-debug still make sense if --log-level is specified?
There was a problem hiding this comment.
I asked myself the same question and it seems that it still does make sense, because it reroutes the output from stdout and stderr to /dev/null and that AFAIK affects Glib's library messages too.
| void | ||
| polkit_backend_authority_set_log_level (const gint level) | ||
| { | ||
| if ((level >= 0) && (level <= LOG_LEVEL_VERBOSE)) |
There was a problem hiding this comment.
Boolean operators have lower precedence than comparison operators, so those nested parens are not needed.
There was a problem hiding this comment.
Yes, you're right, but it's clearer to the reader and immediately visible. That's one of the "will do more than I have to just to make it prettier for humans" LifeProTips on my list :D
| @@ -52,6 +52,14 @@ struct _PolkitBackendAuthority | |||
| GObject parent_instance; | |||
| }; | |||
|
|
|||
| enum | |||
| { | |||
| LOG_LEVEL_ERROR, // log errors only | |||
There was a problem hiding this comment.
Also, the exact same levels as in syslog should be used. This is because those priorities will be propagated to the entries in the journal, and ideally there is a clear mapping between what is visible here in the code and what the journal allows. Doing a 1:1 mapping is the most reasonable.
src/polkitbackend/polkitd.c
Outdated
| {"log-level", 'l', 0, G_OPTION_ARG_INT, &opt_log_level, "Set a level of logging. Values (0-3): errors, warnings, notifications, verbose.", | ||
| "[0,1,2,3]"}, |
There was a problem hiding this comment.
Oh yeah, especially numerical values different than syslog would be really confusing. Please don't do this. And names too. Please just reuse the standard values.
This should mitigate the confusion for the user and enable future integration with journal and LogControl.
| @@ -8,7 +8,7 @@ BusName=org.freedesktop.PolicyKit1 | |||
| CapabilityBoundingSet=CAP_SETUID CAP_SETGID | |||
| DeviceAllow=/dev/null rw | |||
| DevicePolicy=strict | |||
| ExecStart=@libprivdir@/polkitd --no-debug | |||
| ExecStart=@libprivdir@/polkitd --no-debug --log-level=5 | |||
There was a problem hiding this comment.
The command line parameter should take string representations, rather than numbers, as it would be really confusing to use otherwise
There was a problem hiding this comment.
Yes, planned after release (that I need to make this week asap).
as it would be really confusing to use otherwise
polkitd binary is not supposed to be run by user and the help info mentions the correct values. I guess admins would figure out.
There was a problem hiding this comment.
It should be trivial to add? I think it would be better to fix this before the release. Admins adding a drop-in to change the default log level are the users, and it's not really nice to use numeric values like this
There was a problem hiding this comment.
It is a glib option that would probably require a callback function. I'm almost sure the result will be a few lines of code, but I've never done that and I'd have to research the docs and stuff and I really need to make that release instead.
There was a problem hiding this comment.
It doesn't need a callback, I'll prep a PR based on this and open it, probably tomorrow, including LogControl support. I'd really like to ahve both in the next release, so they can be in Debian 13
There was a problem hiding this comment.
Here it is: #482
The commits from this PR are squashed in the first one. The difference is using strings instead of numbers for the command line argument, and also passing the log level to the syslog() call so that messages are tagged accordingly to priority in the journal. And then another commit to add LogControl1 on top of that.
There was a problem hiding this comment.
Yeah, I implemented it with a short callback yesterday. I just didn't push it, for I didn't consider this a race.
There was a problem hiding this comment.
Also, I had presumed that implementing it with tools provided by Glib would allow having the options auto-documented in help page and/or auto-translated. Like I said, it required going through the documentation that showed that it probably doesn't cover the case of this PR (maybe I'm wrong about this, a Glib expert could know more).
|
Merged in #482 along with LogControl support. |
Summary
There was a request to being able to log every loaded .rules script. This required a change in the logging function and a new option parsed during the start of the polkit service.
Detailed description and/or reproducer
All the calls of the logging function had to be changed accordingly. The levels chosen for each such call were guessed from the message -> there might be discussions about that.
The default log level is set to "notify" in the input unit file, this equals the original logging behaviour of polkit. The default and backup value in code is set to "errors only". The "default default" logging shall be "quiet", not too "verbose", but this is always subjective.
A log message about every loaded rule file can be activated by editting the unit file and setting the log level to 3.