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

Draf: Handle SIGWINCH signal add resize logic #1811

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 84 additions & 9 deletions crates/youki/src/commands/run.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
use std::path::PathBuf;
use std::{
fs::OpenOptions,
os::{fd::AsRawFd, unix::prelude::OpenOptionsExt},
path::PathBuf,
};

use anyhow::{Context, Result};
use libcontainer::{container::builder::ContainerBuilder, syscall::syscall::create_syscall};
use liboci_cli::Run;
use nix::{
libc,
sys::{
signal::{self, kill},
signalfd::SigSet,
Expand Down Expand Up @@ -43,16 +48,18 @@ pub fn run(args: Run, root_path: PathBuf, systemd_cgroup: bool) -> Result<i32> {
container.pid().is_some(),
"expects a container init pid in the container state"
);
handle_foreground(container.pid().unwrap())
let console_socket: Option<PathBuf> = args.console_socket.as_ref().map(|p| p.into());
handle_foreground(container.pid().unwrap(), console_socket)
}

// handle_foreground will match the `runc` behavior running the foreground mode.
// The youki main process will wait and reap the container init process. The
// youki main process also forwards most of the signals to the container init
// process.
fn handle_foreground(init_pid: Pid) -> Result<i32> {
fn handle_foreground(init_pid: Pid, socket_path: Option<PathBuf>) -> Result<i32> {
// We mask all signals here and forward most of the signals to the container
// init process.
resize(socket_path.as_ref())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move resize() to above the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also want to use with_context to add some error context. Soon, we will try to move away from anyhow is a number of places in favor of thiserror for more rusty error handling, but for the moment, it would be nice to continue with anyhow context, so we make the debugging easier.

let signal_set = SigSet::all();
signal_set
.thread_set_mask()
Expand Down Expand Up @@ -94,9 +101,7 @@ fn handle_foreground(init_pid: Pid) -> Result<i32> {
// In `runc`, SIGURG is used by go runtime and should not be forwarded to
// the container process. Here, we just ignore the signal.
}
signal::SIGWINCH => {
// TODO: resize the terminal
}
signal::SIGWINCH => resize(socket_path.as_ref())?,
signal => {
// There is nothing we can do if we fail to forward the signal.
let _ = kill(init_pid, Some(signal));
Expand All @@ -105,12 +110,50 @@ fn handle_foreground(init_pid: Pid) -> Result<i32> {
}
}

fn resize(socket_path: Option<&PathBuf>) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you write comments explaining the resizing process? Including the reference to the ioctl used? The comment should allow someone with no prior background information to quickly understand what is going on.

// from /dev/tty file get current termios winsize
let tty_file = OpenOptions::new()
.read(true)
.write(true)
.custom_flags(libc::O_RDWR | libc::O_NOCTTY | libc::O_CLOEXEC)
.open(PathBuf::from("/dev/tty"))?;

let mut ws = nix::pty::Winsize {
ws_row: 0,
ws_col: 0,
ws_xpixel: 0,
ws_ypixel: 0,
};
let ret = unsafe { libc::ioctl(tty_file.as_raw_fd(), libc::TIOCGWINSZ, &mut ws) };
if ret != 0 {
return Err(anyhow::format_err!(std::io::Error::last_os_error()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: add a proper error message to identify that the error is for resizing. Otherwise, we get an error number with no idea where the error is coming from. For example, I can't debug a EINVAL with no additional information.

}
log::debug!("winsize is {:?},socket_path is {:?}", ws, socket_path);

// get socket_file file fd to set socket_file termios winsize
// let path = socket_path.unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

deadcode?

if let Some(path) = socket_path {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: In general, if there is one if branch, I am in favor of using if let .... If there are both the if let ... else, then I am in favor a bit more to using match. Functionally, there are the same, and this is a more of a personal taste thing. Up to you.

let socket_file = OpenOptions::new()
.read(true)
.write(true)
.custom_flags(libc::O_RDWR | libc::O_NOCTTY | libc::O_CLOEXEC)
.open(path)?;
let ret = unsafe { libc::ioctl(socket_file.as_raw_fd(), libc::TIOCSWINSZ, &ws) };
if ret != 0 {
return Err(anyhow::format_err!(std::io::Error::last_os_error()));
}
} else {
log::warn!("socket_path is none");
}
Ok(())
}

#[cfg(test)]
mod tests {
use std::time::Duration;

use nix::{
sys::{signal::Signal::SIGKILL, wait},
sys::{signal::Signal::SIGKILL, signal::Signal::SIGWINCH, wait},
unistd,
};

Expand Down Expand Up @@ -152,7 +195,7 @@ mod tests {
match unsafe { unistd::fork()? } {
unistd::ForkResult::Parent { child } => {
// Inside P1.
handle_foreground(child)?;
handle_foreground(child, None)?;
}
unistd::ForkResult::Child => {
// Inside P2. This process block and waits the `sigkill`
Expand Down Expand Up @@ -185,7 +228,39 @@ mod tests {
match unsafe { unistd::fork()? } {
unistd::ForkResult::Parent { child } => {
// Inside P1.
handle_foreground(child)?;
handle_foreground(child, None)?;
}
unistd::ForkResult::Child => {
// Inside P2. The process exits after 1 second.
std::thread::sleep(Duration::from_secs(1));
}
};
}
};

Ok(())
}

#[test]
fn test_foreground_sigwatch() -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you copy pasted the previous test function. Can you clean up the comments because it doesn't make sense anymore. For example, what is the expected behavior after the child process receives SIGWINCH?

// The setup is similar to `handle_foreground`, but instead of
// forwarding signal, the container init process will exit. Again, we
// use `sleep` to simulate the conditions to aovid fine grained
// synchronization for now.
match unsafe { unistd::fork()? } {
unistd::ForkResult::Parent { child } => {
// Inside P0
std::thread::sleep(Duration::from_secs(1));
kill(child, SIGWINCH)?;
wait::waitpid(child, None)?;
}
unistd::ForkResult::Child => {
// Inside P1. Fork P2 as mock container init process and run
// signal handler process inside.
match unsafe { unistd::fork()? } {
unistd::ForkResult::Parent { child } => {
// Inside P1.
handle_foreground(child, None)?;
}
unistd::ForkResult::Child => {
// Inside P2. The process exits after 1 second.
Expand Down