libc/stdout: add an option to redirect standard output stream to syslog #14923

anchao · 2024-11-25T05:24:45Z

Summary

libc/stdout: add an option to redirect standard output stream to syslog

Redirect standard output stream of printf(), vprintf(), puts(), putchar() to syslog,
This option will ensure the printing sequence of syslog/printf to avoid
confusion. Noted that if enable this option, the redirection
capability of standard input(<) and output(>) will not be work as expect in
(nsh)nuttx shell.

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

sim/nsh

enable SYSLOG_STDOUT/SYSLOG_STDOUT_PREFIX
test code in hello

int main(int argc, FAR char *argv[])
{
  printf("Hello, World!!\n");
  syslog(1, "Hello, World!!\n");
  return 0;
}

check result

$ ./nuttx 
NuttShell (NSH) NuttX-10.4.0
nsh> hello
[    1.030000] [ 4] [NOTICE] Hello, World!!
[    1.030000] [ 4] [ ALERT] Hello, World!!
nsh>

Redirect standard output stream of printf(), vprintf(), puts(), putchar() to syslog, This option will ensure the printing sequence of syslog/printf to avoid confusion. Noted that if enable this option, the redirection capability of standard input(<) and output(>) will not be work as expect in (nsh)nuttx shell. Signed-off-by: chao an <[email protected]>

nuttxpr · 2024-11-25T05:34:29Z

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown:

Summary: While it explains the what, it's missing a clear why. Why is redirecting stdout to syslog necessary? What problem does it solve? Is there a specific use case driving this change? The mention of avoiding confusion with syslog/printf sequencing hints at the reason, but it needs to be more explicit. It also lacks issue references if applicable.
Impact: Marking "N/A" is insufficient. The description explicitly mentions that shell redirection (< and >) will no longer work as expected. This is a significant impact on users and needs to be documented as such. There are also potential impacts on:
- Build: New config options (SYSLOG_STDOUT/SYSLOG_STDOUT_PREFIX) imply build system changes.
- Documentation: This new feature requires documentation.
- Compatibility: The change in shell redirection behavior is a backward compatibility break.
Testing: The testing is minimal. It demonstrates a single, simple case. More robust testing is needed, including:
- Tests with different log levels.
- Tests with long output strings.
- Tests demonstrating the broken redirection behavior (and ideally a workaround or explanation if one exists).
- Testing on real hardware, not just the simulator, if feasible. This helps identify potential platform-specific issues.
- "Testing logs before change" is missing entirely. While it might not be applicable in this specific test case, it's a required section. Even a simple "N/A" would be better than omitting it.

In short: The PR needs more detail and more thorough testing to meet the NuttX requirements. It needs to clearly explain the motivation, fully document the impact (especially the broken redirection), and include more comprehensive testing.

xiaoxiang781216 · 2024-11-25T05:40:03Z

drivers/syslog/Kconfig

@@ -407,5 +407,25 @@ config SYSLOG_REGISTER
 	---help---
 		This option will support register the syslog channel dynamically.

+config SYSLOG_STDOUT


but why not enable CONFIG_CONSOLE_SYSLOG? which just do what you want but in an elegant way.

I also need support input from console. It seems CONSOLE_SYSLOG only support output to syslog.

honestly speaking, i don't think it makes much sense to have a kconfig for this specific exotic configuration.

So, do you have a better solution? The current implementation minimizes the impact for framework changes.

well, i don't understand what problem you want to solve.
your description only says "avoid confusion."

if the problem is that your application is using syslog and printf in a way which somehow confuses you,
the most straightforward solution would be to fix your application.

Neither printf nor syslog can guarantee the integrity of printing, they are have different protection mechanisms in the backend implementation(txsem/csection)

what kind of "the integrity of printing" are you talking about?
after all, syslog and stdout are two separate streams.
i suspect you are expecting too much.

The application is an open-source project. It is impossible to change all stdout to syslog().

are you saying that my guess is correct? ie. it's a problem in your application.

This option is a configurable function, and default value is n, which won't impact anyone.

it affects developers by complicating the configuration matrix.

what kind of "the integrity of printing" are you talking about?
after all, syslog and stdout are two separate streams.
i suspect you are expecting too much.

This isn't a problem that I've encountered personally. Many developers in project have been complaining about it. It's a point that needs to be improved.

are you saying that my guess is correct? ie. it's a problem in your application.

Why? Is there any conflict between call which interface with ensuring complete line output? These are two different issues.

it affects developers by complicating the configuration matrix.

Do you still remember that you submitted over 1,000 commits to fix the printing issue? Type mismatch and printf format fixes #2312? Every time I go through the git history, I'll see all this junk. could I say that you've affected me?

depending on the problem you want to solve, (i'm not sure what it is yet)
writing a character device driver for /dev/console can be a less intrusive (and IMO less confusing) solution i guess.
as @xiaoxiang781216 pointed out, it can be similar to what CONFIG_CONSOLE_SYSLOG does.

what kind of "the integrity of printing" are you talking about?
after all, syslog and stdout are two separate streams.
i suspect you are expecting too much.

1. This isn't a problem that I've encountered personally. Many developers in project have been complaining about it. It's a point that needs to be improved.

maybe the problem is obvious for many developers as you say.
but it isn't for me.
can you please give me a reference?

are you saying that my guess is correct? ie. it's a problem in your application.

2. Why?

i suggested to fix your application if it's a problem in your application.
you answered it's unrealistic to fix the application.
so i thought you were implying that the premise (it's a problem in your application) was true.

Is there any conflict between call which interface with ensuring complete line output? These are two different issues.

sorry, i don't understand what you mean here. what are two issues?

it affects developers by complicating the configuration matrix.

3. Do you still remember that you submitted over 1,000 commits to fix the printing issue? [Type mismatch and printf format fixes #2312](https://github.com/apache/nuttx/pull/2312)? Every time I go through the git history, I'll see all this junk. could I say that you've affected me?

maybe. sorry for affecting you.

however, the confusion this PR would introduce is in another level, IMO.
applications usually do not expect fputs(stdout) and puts works so differently.

other possible solutions i can think of are

#define printf(...) something-with-syslog when building your application

use something like syslog_devchannel.c to redirect syslog to your console device

yamt · 2024-11-25T11:15:50Z

drivers/syslog/Kconfig

+	bool "Redirect standard output stream of printf() and vprintf() to syslog"
+	default n
+	---help---
+		Redirect standard output stream of printf(), vprintf(), puts(), putchar() to syslog,


standard output is not a per-function entity.
"standard output stream of printf()" doesn't make sense.

yamt · 2024-11-25T11:17:59Z

drivers/syslog/Kconfig

@@ -407,5 +407,25 @@ config SYSLOG_REGISTER
 	---help---
 		This option will support register the syslog channel dynamically.

+config SYSLOG_STDOUT


honestly speaking, i don't think it makes much sense to have a kconfig for this specific exotic configuration.

github-actions bot added Area: Drivers Drivers issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 25, 2024

anchao force-pushed the 24112501 branch from ed30da1 to 235af97 Compare November 25, 2024 05:26

xiaoxiang781216 reviewed Nov 25, 2024

View reviewed changes

yamt reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libc/stdout: add an option to redirect standard output stream to syslog #14923

libc/stdout: add an option to redirect standard output stream to syslog #14923

anchao commented Nov 25, 2024

nuttxpr commented Nov 25, 2024

xiaoxiang781216 Nov 25, 2024

anchao Nov 25, 2024

yamt Nov 25, 2024

anchao Nov 25, 2024

yamt Nov 26, 2024

yamt Nov 26, 2024

anchao Nov 26, 2024

yamt Nov 26, 2024

yamt Nov 26, 2024

yamt Nov 26, 2024

yamt Nov 25, 2024

yamt Nov 25, 2024

libc/stdout: add an option to redirect standard output stream to syslog #14923

Are you sure you want to change the base?

libc/stdout: add an option to redirect standard output stream to syslog #14923

Conversation

anchao commented Nov 25, 2024

Summary

Impact

Testing

nuttxpr commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment