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

Do not start getty on tty0 to avoid screen corruption #4517

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rucoder
Copy link
Contributor

@rucoder rucoder commented Jan 10, 2025

Replace tty0 with tty1 when starting getty

getty can be started in two different ways:

  1. when 'getty' parameter is specified on kernel coommane line and there is at least one console= parameter
  2. if getty is NOT specified on kernel command line but debug.enable.console=true is set AND at least one console= is specified

most people specify console=tty0 on kernel command line if VGA console is desired. this is technically works becasue during kernel start tty0 points to the first available console which is tty1. However when getty is started after sytems is booted tty0 may not be equal to tty1 anymore e.g. if openvt was called like we do to run monitor app. in this case getty will take over CURRENT virtual console which is tty2 in case of monitor application.

  1. To reproduce the issue (tty0 points to current virtual console e.g. tty2) following conditions must be met
  • there is NO 'getty' specified on kernel command line
  • there is console=tty0 specified
  • debug.enable.console=true is set

if all 3 are true then getty is started by pillar AFTER monitor app is started on tty2 and since tty0 points to tty2 in this case getty revokes console from monitor app and the output gets corrupted

  1. we cannot use /dev/console from rungetty.sh anymore because it corrupts tty2 when running from pillar. Instead we use /dev/tty so the logs will be redirected to proper stdout and it should work in both cases: when getty is started by 'getty' parameter and when it is started from context of pillar. In later case logs will appear from pillar context which is correct

@rene
Copy link
Contributor

rene commented Jan 10, 2025

@rucoder , I think it would be better to fix the original rungetty.sh script (from linuxkit: https://github.com/linuxkit/linuxkit/blob/master/pkg/getty/usr/bin/rungetty.sh) instead of introduce a new one....

@@ -3,3 +3,4 @@
.*\.dts
.*\.md
^.codespellignorelines
pkg/dom0-ztools/rootfs/usr/bin/rungetty.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you want to ignore blank tabs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, because it's not our code originally )

@rene
Copy link
Contributor

rene commented Jan 10, 2025

@rucoder , I think it would be better to fix the original rungetty.sh script (from linuxkit: https://github.com/linuxkit/linuxkit/blob/master/pkg/getty/usr/bin/rungetty.sh) instead of introduce a new one....

☝️ @deitch

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we add this new script, but where do we run it? I don't see any invocation...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to point to the source of the file in the commit message if it's not ours.
https://github.com/linuxkit/linuxkit/blob/master/pkg/getty/usr/bin/rungetty.sh, I guess?

@@ -3,3 +3,4 @@
.*\.dts
.*\.md
^.codespellignorelines
pkg/dom0-ztools/rootfs/usr/bin/rungetty.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, because it's not our code originally )

@rucoder
Copy link
Contributor Author

rucoder commented Jan 10, 2025

@rucoder , I think it would be better to fix the original rungetty.sh script (from linuxkit: https://github.com/linuxkit/linuxkit/blob/master/pkg/getty/usr/bin/rungetty.sh) instead of introduce a new one....

☝️ @deitch

@deitch @rene technically this fix is applicable to eve only. no one in their sane mind would run getty manually from another process. I would not accept such fix if I were a maintainer for getty in linux kit

@rucoder
Copy link
Contributor Author

rucoder commented Jan 10, 2025

@rene @OhmSpectator the script is ignored in SPDX and tabs check as well as yetus checks because it is not our code and I just copied original file to minimize changes and make them visible.

@rucoder
Copy link
Contributor Author

rucoder commented Jan 10, 2025

I see we add this new script, but where do we run it? I don't see any invocation...

makes sense, fixed

- Add unmodified file to patch it following commit
The script is taken from getty container from linux kit
https://github.com/linuxkit/linuxkit/blob/master/pkg/getty/usr/bin/rungetty.sh

Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
1) tty0 points to current virtual console. it is not a problem when getty
is started very early before pillar/monitor are started however under
following conditions it causes problems:
1. there is NO 'getty' specified on kernel command line
2. there is console=tty0 specified
3. debug.enable.console=true is set

if all 3 are true then getty is started by pillar AFTER monitor app is
started on tty2 and since tty0 points to tty2 in this case getty revokes
console from monitor app and the output gets corrupted

2) we cannot use /dev/console from rungetty.sh anymore because it
corrupts tty2 when running from pillar. Instead we use /dev/tty so
the logs will be redirected to proper stdout and it should work in both
cases: when getty is started by 'getty' parameter and when it is started
from context of pillar. In later case logs will appear from pillar
context which is correct

Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
We do not maintain this file so do not check for errors (too many of them
really)

Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
@rucoder rucoder force-pushed the rucoder/getty-tty0 branch from 80f6356 to a993891 Compare January 10, 2025 21:44
@deitch
Copy link
Contributor

deitch commented Jan 12, 2025

technically this fix is applicable to eve only. no one in their sane mind would run getty manually from another process. I would not accept such fix if I were a maintainer for getty in linux kit

Exactly. Are we "in our sane mind" if we take a kernel cmdline that included console=tty0 and change it under the covers to tty1? I would think that would upset the user once they figured out what happened ("user" being us when we eventually have a problem and figure out what we did after spending two days trying to understand where getty is when we know we wrote "console=tty0").

becasue during kernel start tty0 points to the first available console which is tty1. However when getty is started after sytems is booted tty0 may not be equal to tty1 anymore e.g. if openvt was called like we do to run monitor app. in this case getty will take over CURRENT virtual console which is tty2 in case of monitor application

This completely confused me. Someone writes tty0 on cmdline, but it actually is tty1, and sometimes tty2?

Going more to the heart of the issue, based on what you described above, it sounds like you are saying, in chronological order:

  1. System starts
  2. TUI starts on tty0
  3. getty starts, tries also to grab tty0, not knowing that something already has it, and they corrupt each other.

Would it make sense then, rather than some hard-coding, to have a detection, some form of, "if ttyX already is in use, skip it"? I don't know how that would work, but it would be a logical thing to do.

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

Successfully merging this pull request may close these issues.

4 participants