-
Notifications
You must be signed in to change notification settings - Fork 231
Optional syslog support #1590
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
base: master
Are you sure you want to change the base?
Optional syslog support #1590
Conversation
This patch adds an optional syslog backend alternative to the default stdout/stderr logging. The syslog backend is enabled with `--syslog` on the command line. It logs directly to the local syslog daemon, via the POSIX syslog() API, using a default 'daemon' facility. This facility can be changed if an optional argument is given, e.g., `--syslog=local0`. Useful for basic message filtering to a dedicated log file. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1590 +/- ##
==========================================
- Coverage 84.42% 84.26% -0.16%
==========================================
Files 69 69
Lines 21663 21702 +39
==========================================
Hits 18288 18288
- Misses 3375 3414 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
LGTM, it would indeed be handy to have rather than having to pipe rauc stdout/sterr to logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 2.80, glib has native support for syslog. Currently, we only require glib 2.56. Furthermore, the glib implementation defaults to the "user" facility (if not overridden on the message level). So we can't easily use the native support.
Still, we could take some inspiration from the implementation, such as avoiding the need for SYSLOG_NAMES
by mapping to the integer levels directly.
Which use-case do you see for specifying the facility on the command line? Perhaps hard-coding it to the LOG_DAEMON
value would be enough?
} | ||
} | ||
|
||
openlog(G_LOG_DOMAIN, LOG_PID | LOG_NOWAIT, facility); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syslog(3)
mentions that LOG_NOWAIT
has no effect on Linux, so it could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it depends a bit on the backend. I maintain sysklogd, which today supplies an RFC5424 compliant syslogp()
, as well as replacement syslog()
API, this is one implementation that can replace the stanard C-library APIs for syslog.
There's no overhead to supporting LOG_NOWAIT
, but if you only want to support GLIBC or standard C-library, that is fine and I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the effect of LOG_NOWAIT
with sysklogd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally it will connect immediately to the /dev/log
socket instead of postponing it until the first syslogd()
call.
@@ -10,6 +10,8 @@ | |||
#include <locale.h> | |||
#include <stdio.h> | |||
#include <string.h> | |||
#define SYSLOG_NAMES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? https://sourceware.org/bugzilla/show_bug.cgi?id=16355 sounds like we should avoid this define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The define opens up the use of facilitynames[]
. It has been a long standing practice, but we can replace that with a local array of the same names if you want.
@jluebbe I take it you are still overall in favor of adding native |
To fully replace this native implementation with the glib feature, we'd need to add a way to set the default facility via the glib API, which would result in an even higher minimum version. So I'm still in favor of a native implementation. It would be good to have test coverage for the new code, so that we can avoid regressions. Can we temporarily run a syslog service to catch messages from rauc running with |
glib 2.80 is also quite recent (~10 months ago), so it would be annoying to require it |
OK, cool thanks!
Should definitely be possible. I'll have a look at your regression test system and see what I can do. |
@troglobit Do you need some help for the testing side? If that's blocking you, we could try to find some time to prepare something there. |
Sorry for dragging my feet on this one, I've been reassigned to other projects for a while. Been meaning to get back to this, and possibly in a week or two, now that Sweden is shutting down for the summer, I'll be back on it. If I get stuck, I'll let you know, thanks! |
This pull request adds an optional syslog backend alternative to the default stdout/stderr logging. It is primarily intended for systems using RAUC without systemd, e.g., an embedded system built around BusyBox init.
It is of course possible to use RAUC as-is on non-systemd systems, however with this patch users avoid ANSI escape sequences (color) in their logs and can perform severity based filtering for remote logging of critical events.
We use an earlier version of this patch in the Infix NOS. This updated version adds configurable facility support and also ensures to replace the default logger.