[011] Support for --leave-stdin-open.#15
Conversation
This commit does the following: - Moves the code to run the `runtime` process from `run.rs` to `process.rs` into the `RuntimeProcess` struct. - Moves the common code shared between `create` and `exec` commands to `session.rs` into the `RuntimeSession` struct. - The `RuntimeSession` uses the `RuntimeProcess` in high-level way. - The basic `exec.rs` is implemented using `RuntimeSession`. - The exit code is now passed from the commands to the `main` and returned there. Signed-off-by: Jan Kaluza <[email protected]>
This commit does the following: - Handles the `--exec-attach` option. - Handles the _OCI_ATTACHPIPE and _OCI_STARTPIPE. It is tested by the conmon-v2 tests run as part of `make test`. Signed-off-by: Jan Kaluza <[email protected]>
Summary of ChangesHello @jankaluza, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the --leave-stdin-open flag and includes a significant refactoring of the runtime process management. The new RuntimeSession, RuntimeProcess, and UnixSocket abstractions improve the code's structure and maintainability. My review focuses on potential issues in error handling, file permissions, and logic completeness. Overall, this is a solid improvement, and my comments aim to enhance its robustness.
src/log.rs
Outdated
| let file = OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(path) | ||
| .expect("Failed to open log file"); |
There was a problem hiding this comment.
The use of .expect() here will cause the program to panic if the log file cannot be opened, for instance, due to permission issues. It would be more robust to handle this error by returning a ConmonResult.
| let file = OpenOptions::new() | |
| .create(true) | |
| .append(true) | |
| .open(path) | |
| .expect("Failed to open log file"); | |
| let file = OpenOptions::new() | |
| .create(true) | |
| .append(true) | |
| .open(&path) | |
| .map_err(|e| ConmonError::new(format!("Failed to open log file {path}: {e}"), 1))?; |
| Err(err) => { | ||
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN { | ||
| // nothing more to read right now, jump to next fd | ||
| i += 1; | ||
| continue; | ||
| } | ||
| return Err(ConmonError::new( | ||
| format!( | ||
| "handle_stdio read() failed: {}", | ||
| io::Error::from_raw_os_error(err as i32) | ||
| ), | ||
| 1, | ||
| )); | ||
| } |
There was a problem hiding this comment.
The read call on the stdout/stderr pipes does not handle EINTR. If the read is interrupted by a signal, it will return Err(Errno::EINTR), which will be treated as a fatal error, causing handle_stdio to exit. EINTR should be handled by retrying the read.
| Err(err) => { | |
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN { | |
| // nothing more to read right now, jump to next fd | |
| i += 1; | |
| continue; | |
| } | |
| return Err(ConmonError::new( | |
| format!( | |
| "handle_stdio read() failed: {}", | |
| io::Error::from_raw_os_error(err as i32) | |
| ), | |
| 1, | |
| )); | |
| } | |
| Err(err) => { | |
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN || err == Errno::EINTR { | |
| // nothing more to read right now, jump to next fd | |
| i += 1; | |
| continue; | |
| } | |
| return Err(ConmonError::new( | |
| format!( | |
| "handle_stdio read() failed: {}", | |
| io::Error::from_raw_os_error(err as i32) | |
| ), | |
| 1, | |
| )); | |
| } |
| Err(err) => { | ||
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN { | ||
| i += 1; | ||
| } else { | ||
| // treat as fatal for this remote: remove it | ||
| let remote = remote_sockets.swap_remove(remote_idx); | ||
| fds.swap_remove(i); | ||
| if remote.socket_type == SocketType::Console && !leave_stdin_open { | ||
| // This closes the socket, since moves out of scope. | ||
| workerfd_stdin.take(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the error handling for recvfrom, EINTR is not handled. If the recvfrom call is interrupted by a signal, it will return Err(Errno::EINTR). The current code will treat this as a fatal error for the remote connection and drop it. EINTR should be handled by retrying the operation.
| Err(err) => { | |
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN { | |
| i += 1; | |
| } else { | |
| // treat as fatal for this remote: remove it | |
| let remote = remote_sockets.swap_remove(remote_idx); | |
| fds.swap_remove(i); | |
| if remote.socket_type == SocketType::Console && !leave_stdin_open { | |
| // This closes the socket, since moves out of scope. | |
| workerfd_stdin.take(); | |
| } | |
| } | |
| } | |
| Err(err) => { | |
| if err == Errno::EWOULDBLOCK || err == Errno::EAGAIN || err == Errno::EINTR { | |
| i += 1; | |
| } else { | |
| // treat as fatal for this remote: remove it | |
| let remote = remote_sockets.swap_remove(remote_idx); | |
| fds.swap_remove(i); | |
| if remote.socket_type == SocketType::Console && !leave_stdin_open { | |
| // This closes the socket, since moves out of scope. | |
| workerfd_stdin.take(); | |
| } | |
| } | |
| } |
|
|
||
| // Create the parent-directory of full-path. | ||
| let flags = OFlag::O_CREAT | OFlag::O_CLOEXEC | OFlag::O_PATH; | ||
| let dfd = open(dir, flags, Mode::from_bits_truncate(0o600))?; |
There was a problem hiding this comment.
The parent directory for the socket is created with permissions 0o600. This is incorrect for a directory, as it needs the execute bit (x) to be accessible for lookups. This will likely cause socket creation to fail. The mode should be 0o700 or 0o755.
| let dfd = open(dir, flags, Mode::from_bits_truncate(0o600))?; | |
| let dfd = open(dir, flags, Mode::from_bits_truncate(0o700))?; |
| pub fn exec(&self) -> ConmonResult<ExitCode> { | ||
| let _runtime_args = generate_runtime_args(&self.cfg.common, self); | ||
|
|
||
| Ok(()) | ||
| Ok(ExitCode::SUCCESS) | ||
| } |
There was a problem hiding this comment.
The exec function for restore appears to be a stub. It generates _runtime_args but they are not used, and the function immediately returns Ok(ExitCode::SUCCESS). Is the full implementation of the restore command planned for a future change?
| // TODO: We are reading just once here and if container prints more than | ||
| // a buffer sizeto stderr, we ignore whatever does not fid into the buffer. | ||
| // This might be a problem, but the original conmon-v2 code behaves the same way. |
There was a problem hiding this comment.
As noted in the TODO comment, this function reads from mainfd_stderr only once. If the runtime produces more error output than the buffer size (8192 bytes), the error message sent to the parent will be truncated. It would be better to read from the pipe in a loop until it's empty to capture the full error message, or at least until EWOULDBLOCK is returned.
| Err(Errno::EWOULDBLOCK) => Ok(None), | ||
| Err(e) => { | ||
| eprintln!("warn: Failed to accept client connection on attach socket: {e}"); | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
When accept() fails with an error other than EWOULDBLOCK, the error is printed to stderr and then ignored by returning Ok(None). This could hide persistent issues with the listening socket. It would be more robust to propagate the error up by returning a ConmonResult::Err.
| Err(Errno::EWOULDBLOCK) => Ok(None), | |
| Err(e) => { | |
| eprintln!("warn: Failed to accept client connection on attach socket: {e}"); | |
| Ok(None) | |
| } | |
| Err(Errno::EWOULDBLOCK) => Ok(None), | |
| Err(e) => Err(ConmonError::new( | |
| format!("Failed to accept client connection on attach socket: {e}"), | |
| 1, | |
| )), |
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. - Disables #![allow(clippy::collapsible_if)] lint globally. The Rust version in CI does not support collapsible ifs with `let =` expressions. Signed-off-by: Jan Kaluza <[email protected]>
This commit adds simple logging mechanism based on the `log` and `chrono` crates to write conmon debugging logs in the bundle directory. The directory can also be changed using the env variables. Signed-off-by: Jan Kaluza <[email protected]>
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. Signed-off-by: Jan Kaluza <[email protected]>
This commit adds simple logging mechanism based on the `log` and `chrono` crates to write conmon debugging logs in the bundle directory. The directory can also be changed using the env variables. Signed-off-by: Jan Kaluza <[email protected]>
This commit does few small changes related to buffers used in the code: - The `read_pipe` now accepts buffer reference instead of creating new buffer on each call and copying it back to caller. - The `str::from_utf8` is used when appropriate. - The confusing `bytes` variable is renamed to `n`, indicating it is the number of bytes read. - The `if` is used to detect EOF instead of `match` for a better readibility. Signed-off-by: Jan Kaluza <[email protected]>
jankaluza
changed the title
This commits does the following: - Adds new `UnixSocket` struct to create new Unix socket, bind it to local directory and accept new connections. - Adds new `RemoteSocket` struct representing the UnixSocket clients. - Creates `attach` UnixSocket in the `RuntimeSession`. - Handles the `attach` socket in the the `handle_stdio` - accepting new clients and handling their RemoteSockets. - Handles the `--stdin` option by creating the stdin pipe for a container and sending data to it when available. Signed-off-by: Jan Kaluza <[email protected]>
This commit does few small changes related to buffers used in the code: - The `read_pipe` now accepts buffer reference instead of creating new buffer on each call and copying it back to caller. - The `str::from_utf8` is used when appropriate. - The confusing `bytes` variable is renamed to `n`, indicating it is the number of bytes read. - The `if` is used to detect EOF instead of `match` for a better readibility. Signed-off-by: Jan Kaluza <[email protected]>
This commit handles the --leave-stdin-open flag. When used, the workerfd_stdin is not closed once the remote client disconnects the attach socket. Signed-off-by: Jan Kaluza <[email protected]>
1 participant
This commit handles the --leave-stdin-open flag. When used,
the workerfd_stdin is not closed once the remote client disconnects
the attach socket.
Signed-off-by: Jan Kaluza [email protected]