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

Implement stdin/stdout/stderr redirection support using multiport virtio-console #157

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

mtjhrc
Copy link
Collaborator

@mtjhrc mtjhrc commented Jan 31, 2024

No description provided.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
This makes the device act as a multiport console.
This doesn't introduce new functionality, the number of ports remains
fixed at 1 port (for the terminal with stdin/stdout).

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Signed-off-by: Matej Hrica <mhrica@redhat.com>
This is done by creating a port for each of stdin/stdout/stderr,
Each of these ports is multidirectional but we only use one direction.
Port 0 must always be a console - this is implied by the specification.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
@mtjhrc mtjhrc force-pushed the multiport-console-v3 branch 2 times, most recently from 1cbe806 to c14f41f Compare January 31, 2024 19:46
When we redirect the stdout of the guest it opens the question,
where should init/kernel messages (things written to the console) go.
It seems to make sense to write it as errors to the log,
because for now the only messages that go there are errors.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
This works by writing ascii 'ETX' into the guest console on SIGINT.
This curently only works when the guest input is redirected (otherwise
pressing ^C should work, but SIGINT to the host libkrun process
won't work.

Furthermore init process needs to fork itself, because pid 1
cannot receive SIGINT signal, unless it instals a handler, but we cannot
expect all programs to do that.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
We should wait until the output currently in the queue of the guest
is written and only then exit the libkrun process.

This fixes an issue where you would sometimes not get the full output
of a program inside the vm (we would call exit sooner, than we
finished all the writes).

Signed-off-by: Matej Hrica <mhrica@redhat.com>
@mtjhrc mtjhrc marked this pull request as ready for review February 1, 2024 09:03
Copy link
Contributor

@slp slp left a comment

Choose a reason for hiding this comment

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

Overall looks good, just suggested some minor changes for macOS compatbility.

src/devices/src/virtio/console/port_io.rs Outdated Show resolved Hide resolved
src/devices/src/virtio/console/port_io.rs Outdated Show resolved Hide resolved
@@ -28,6 +29,7 @@ use kbs_types::Tee;
use crate::device_manager;
#[cfg(feature = "tee")]
use crate::resources::TeeConfig;
use crate::signal_handler::register_sigint_handler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the register_sigint_handler import and its uses behind a #[cfg(target_os = "linux")] build-time conditional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but why? Does it use something non portable?

Copy link
Collaborator Author

@mtjhrc mtjhrc Feb 6, 2024

Choose a reason for hiding this comment

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

Oh the whole signal_handler file is not compiled on macOS.

Btw we can add cargo clippy --target aarch64-apple-darwin to the pipeline, which checks against simple issues like this (non existing imports), but this currently generates some warnings, so maybe it can be in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's get this one merged first. I'll prepare a new PR setting up the CI for aarch64-apple-darwin.

Signed-off-by: Matej Hrica <mhrica@redhat.com>
Copy link
Contributor

@slp slp left a comment

Choose a reason for hiding this comment

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

Builds now fine on macOS/aarch64. Let's get it merged. Thanks!

@slp slp merged commit c16ffcf into containers:main Feb 6, 2024
4 checks passed
@slp slp mentioned this pull request Aug 5, 2024
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.

2 participants