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

Reduce additional console output when muted #1407

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

RedcomOSS
Copy link

@RedcomOSS RedcomOSS commented Sep 3, 2024

While trying to resolve some custom installer issues, we found that using conscontrol(8) or setting kern.always_console_output=0 in sysctl.conf(5) did not always prevent console output. This is in part because some areas of the kernel were outputting to the console device without checking the status of the setting. These changes enforce checking of the value in both locations where console output occurs from kernel and init(8) based callouts.

Details on changes:

  • Moves check for mute to earlier in sequence to silence kernel output even if EARLY_PRINTF is defined.
  • Modifies call prf_putbuf() and prf_putchar() in subr_prf.c to strip TOCONS flag if muting is enabled, to honor the setting at print level.

This is a rather simple change, which increases areas where flags to silence console output are honored. We have been running this change since 10/23 in-house without issue (Patching prior to 14.0 also required making cn_mute non-static.)

Signed-off-by: Craig.Woodward@redcom.com

CW-RCL and others added 2 commits September 3, 2024 14:42
While trying to resolve some custom installer issues, we found that using conscontrol(8) or setting kern.always_console_output=0 in sysctrl.conf(5) did not always prevent console output. This is in part because some areas of the kernel were outputting to the console device without checking the status of the setting. These changes enforce checking of the value in both locations where console output occurs from kernel and init(8) based callouts.

Details on changes:

 - Moves check for mute to earlier in sequence to silence kernel output even if EARLY_PRINTF is defined.
 - Modifies call prf_putbuf() and prf_putchar() in subr_prf.c to strip TOCONS flag if muting is enabled, to honor the setting at print level.

This is a rather simple change, which increases areas where flags to silence console output are honored.
We have been running this change since 10/23 in-house without issue.
(Patching prior to 14.0 also required making cn_mute non-static.)

Signed-off-by: Craig.Woodward@redcom.com
sys/kern/subr_prf.c Outdated Show resolved Hide resolved
sys/kern/subr_prf.c Outdated Show resolved Hide resolved
@jlduran
Copy link
Member

jlduran commented Sep 4, 2024

In the commit message, sysctrl.conf(5) -> sysctl.conf(5).

@RedcomOSS
Copy link
Author

Adjusted

CW-RCL and others added 2 commits September 10, 2024 14:20
While trying to resolve some custom installer issues, we found that using conscontrol(8) or setting kern.always_console_output=0 in sysctl.conf(5) did not always prevent console output. This is in part because some areas of the kernel were outputting to the console device without checking the status of the setting. These changes enforce checking of the value in both locations where console output occurs from kernel and init(8) based callouts.

Details on changes:

  - Moves check for mute to earlier in sequence to silence kernel output even if EARLY_PRINTF is defined.
  - Modifies call prf_putbuf() and prf_putchar() in subr_prf.c to strip TOCONS flag if muting is enabled, to honor the setting at print level.

This is a rather simple change, which increases areas where flags to silence console output are honored.
We have been running this change since 10/23 in-house without issue.
(Patching prior to 14.0 also required making cn_mute non-static.)

Signed-off-by: Craig.Woodward@redcom.com
@RedcomOSS
Copy link
Author

Missed the code changes. I'm a new to the process of pull requests here. Do I need to close this and open a new/clean pull request? The push/merge seems to have gone fine, but it's blocked on compute time limits?

@jlduran
Copy link
Member

jlduran commented Sep 10, 2024

Do I need to close this and open a new/clean pull request?

No worries! It is better to keep it on this same pull request.
It is likely that a final rebase/squash on your commits is requested (or the committer may have an automated script that does it for you).

The push/merge seems to have gone fine, but it's blocked on compute time limits?

Yes, usually the credits run out within the first few days of the month.


Is it easy to add a before/after output? Or a way to easily reproduce the differences?

@RedcomOSS
Copy link
Author

It is likely that a final rebase/squash on your commits is requested (or the committer may have an automated script that does it for you).

Fine with a squash. I repeated the commit in the second update (with requested correction), for easier squashing.

Yes, usually the credits run out within the first few days of the month.

The new delta is literally white-space fixes, so compile should be no different. That would be up to the committer I suppose, but seems unneeded for this delta if they're a scarce resource.

Is it easy to add a before/after output? Or a way to easily reproduce the differences?

If you mean a clean diff from main to this (post fixes) then yes. I can generate it from our base tree vs our last merge point. But doing a checkout/patch/commit would remove the references already in this pull, would it not?

If you mean a test-case for output this patch would block? That's harder.

We found it when an app was trying to run when a library it wanted was not available. We could easily catch the error and setup a workaround. The problem was that the console output would show the library load failure, even when cn_mute was set, messing up the ASCII status screen. We tracked it to this and patched accordingly.

This area of the code has seen little change in the base tree for many years. The only "conflict" in pulling up to 14.0 was cn_mute had changed to non-static, when it was added to the sysctl(8) list. We had it non-static for this patch since 13.X-ish. It complicated the last merge, (potential reverse patch detected) which made the argument to upstream changes easier.

@bsdimp bsdimp added the ready label Oct 1, 2024
@RedcomOSS
Copy link
Author

Since this has been marked "ready", is there anything more I need to do to get this into a main tree? I'm not super familiar with the process here, so just want to make sure I'm not missing a step to get changes merged in.

@bsdimp
Copy link
Member

bsdimp commented Oct 31, 2024

I've been too busy this month to land things... I just need to make sure this boots (I'm 99% sure it will) and I'll commit it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants