Draf: Handle SIGWINCH signal add resize logic #1811
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1811 +/- ##
==========================================
- Coverage 68.64% 68.50% -0.14%
==========================================
Files 121 121
Lines 13325 13378 +53
==========================================
+ Hits 9147 9165 +18
- Misses 4178 4213 +35 |
crates/youki/src/commands/run.rs
Outdated
| .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) }; |
There was a problem hiding this comment.
Have you tried using the nix crate?
https://docs.rs/nix/latest/nix/sys/ioctl/index.html
There was a problem hiding this comment.
I saw a saying that because nix is slow to update, some projects now directly use libc.
There was a problem hiding this comment.
For example, libc::TIOCGWINSZ and libc::TIOCSWINSZ cannot be found in nix.
There was a problem hiding this comment.
I saw a saying that because nix is slow to update, some projects now directly use libc.
Can you post a link here if you can still find it? I would love to see it. I will also read up on ioctl from nix, but from a quick scanning, it looks to be quite messy so for this simple case, it may be worth to just go with the libc version.
Signed-off-by: lengrongfu <[email protected]>
| // We mask all signals here and forward most of the signals to the container | ||
| // init process. | ||
| resize(socket_path.as_ref())?; |
There was a problem hiding this comment.
Move resize() to above the comment.
| 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(); |
| @@ -105,12 +110,50 @@ fn handle_foreground(init_pid: Pid) -> Result<i32> { | |||
| } | |||
| } | |||
|
|
|||
| fn resize(socket_path: Option<&PathBuf>) -> Result<()> { | |||
There was a problem hiding this comment.
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.
crates/youki/src/commands/run.rs
Outdated
| .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) }; |
There was a problem hiding this comment.
I saw a saying that because nix is slow to update, some projects now directly use libc.
Can you post a link here if you can still find it? I would love to see it. I will also read up on ioctl from nix, but from a quick scanning, it looks to be quite messy so for this simple case, it may be worth to just go with the libc version.
| // We mask all signals here and forward most of the signals to the container | ||
| // init process. | ||
| resize(socket_path.as_ref())?; |
There was a problem hiding this comment.
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 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())); |
There was a problem hiding this comment.
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.
|
|
||
| // get socket_file file fd to set socket_file termios winsize | ||
| // let path = socket_path.unwrap(); | ||
| if let Some(path) = socket_path { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| #[test] | ||
| fn test_foreground_sigwatch() -> Result<()> { |
There was a problem hiding this comment.
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?
|
Hi @lengrongfu, it appears that there are conflicts between this PR and the main branch. Could you kindly review and update it? Thank you. |
lengrongfu
changed the title
|
I referred to runc and found that the current console does not support resize logic, because we lack a scene to create a tty: current youki console steup code: So this PR may have to be suspended first, and another console logic needs to be implemented first. |
|
issue: #1672