Skip to content

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

troglobit
Copy link
Contributor

@troglobit troglobit commented Jan 8, 2025

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.

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>
@ejoerns ejoerns added the enhancement Adds new functionality or enhanced handling to RAUC label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes missing coverage. Please review.

Project coverage is 84.26%. Comparing base (7b60851) to head (ccd2af8).

Files with missing lines Patch % Lines
src/main.c 0.00% 39 Missing ⚠️
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     
Flag Coverage Δ
service=false 80.98% <0.00%> (-0.15%) ⬇️
service=true 84.19% <0.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jacmet
Copy link
Contributor

jacmet commented Jan 8, 2025

LGTM, it would indeed be handy to have rather than having to pipe rauc stdout/sterr to logger

@jluebbe jluebbe self-requested a review January 8, 2025 13:39
Copy link
Member

@jluebbe jluebbe left a 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);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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.

@troglobit
Copy link
Contributor Author

@jluebbe I take it you are still overall in favor of adding native syslog() support in this form? Or would you prefer closing this in favor of the native (later) glib support? I'm fine either way, we can carry the patch locally until we get the bump in glib version.

@jluebbe
Copy link
Member

jluebbe commented Jan 8, 2025

@jluebbe I take it you are still overall in favor of adding native syslog() support in this form? Or would you prefer closing this in favor of the native (later) glib support? I'm fine either way, we can carry the patch locally until we get the bump in glib version.

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 --syslog?

@jacmet
Copy link
Contributor

jacmet commented Jan 8, 2025

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.

glib 2.80 is also quite recent (~10 months ago), so it would be annoying to require it

@troglobit
Copy link
Contributor Author

troglobit commented Jan 9, 2025

@jluebbe I take it you are still overall in favor of adding native syslog() support in this form? Or would you prefer closing this in favor of the native (later) glib support? I'm fine either way, we can carry the patch locally until we get the bump in glib version.

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.

OK, cool thanks!

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 --syslog?

Should definitely be possible. I'll have a look at your regression test system and see what I can do.

@troglobit troglobit marked this pull request as draft January 9, 2025 13:12
@troglobit troglobit changed the title RFC: Optional syslog support Optional syslog support Jan 9, 2025
@jluebbe
Copy link
Member

jluebbe commented Jun 25, 2025

@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.

@troglobit
Copy link
Contributor Author

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds new functionality or enhanced handling to RAUC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants