libcontainer: use OwnedFd as console_socket in ContainerBuilder #2966
Conversation
crates/libcontainer/src/tty.rs
Outdated
| @@ -233,7 +221,7 @@ mod tests { | |||
| let lis = UnixListener::bind(&socket_path); | |||
| assert!(lis.is_ok()); | |||
| let fd = setup_console_socket(testdir.path(), &socket_path, CONSOLE_SOCKET); | |||
| let status = setup_console(&fd.unwrap()); | |||
| let status = setup_console(fd.unwrap().as_raw_fd()); | |||
There was a problem hiding this comment.
This test is failing as setup_console is closing the RawFd it receives.
| csocketfd.as_raw_fd(), | ||
| &socket::UnixAddr::new(linked.as_path()).map_err(|err| TTYError::InvalidSocketName { | ||
| source: err, | ||
| socket_name: socket_name.to_string(), | ||
| })?, | ||
| ) { | ||
| Err(Errno::ENOENT) => -1, |
There was a problem hiding this comment.
The logic of this branch (and checked by the test_setup_console_socket_empty test) is lost here.
There was a problem hiding this comment.
Yes, I don't quite get why is this error handled independently, Could you mind explain why do we have to return -1 when ENOENT?
There was a problem hiding this comment.
This avoids failing in the case where the file is not present. I'm not sure if this is part of the spec requirements, or just copying what runc does, or just happens to be an implementation detail of youki.
There was a problem hiding this comment.
Well, How about returning error when the file is not present? I don't think it will succeed if the init process send the tty fd through a socket of fd -1.
It is difficult to modify it if this function return OwnedFd, I think we can't new an OwnedFd from a rawfd of -1, because it may panic if the drop of OwnedFd is called, and we will call close of an fd of -1.
There was a problem hiding this comment.
Can we get the background of why does it return fd of -1 here? It is a little wierd because I don't find any similar logic in runc.
There was a problem hiding this comment.
Hey so I went around in the git blame and tried to find out why this is specifically -1 , but from what I see, the -1 comes directly from the first commit (!) , and is kept as-is for no specific reason. Checking the code there, I think the intention was maybe to conform to the standard error policy (returning -1 on error) , but not sure. What I can see is that in that commit, this will just push error to a later time, when calling sendmsg with socket_fd of -1. Apart from that atleast I cannot find any reason in that or today's code why we specifically set it to -1 rather than return an error, because the code will error when it tries to use this raw fd for sendmsg. I think the correct way to proceed would be another analysis by someone else to check what I'm saying is right or not, and if it is, then replacing the -1 by the general error case.
@utam0k , correct me if I'm wrong here.
There was a problem hiding this comment.
I believe that it came from railcar. -1 doesn't have a special reason. I'm afraid I've confused you... I'll note the person who first committed (that's me) 🫠
https://github.com/oracle/railcar/blob/ef5918e21e7ad9ffd25c1a507df96458ec4e0c24/src/main.rs#L539-L549
There was a problem hiding this comment.
So, Can we say that it is ok to return error rather than -1?
There was a problem hiding this comment.
@abel-von , yes I think we can just merge that into the error case.
|
Hi @YJDoc2, I think they are independent, we found these issues because we are trying to use libcontainer instead of calling runc binary to manage containers. In our senarios, we have one process in a machine to manage all the containers running on it, so when we call libcontainer to manage containers, we have to make sure no resource residual in this process when a container is removed. |
abel-von
force-pushed
the
fix-socket-close
branch
from
bb5ade5
to
a267816
Compare
abel-von
force-pushed
the
fix-socket-close
branch
from
a267816
to
9aa282b
Compare
Signed-off-by: Abel Feng <[email protected]>
abel-von
force-pushed
the
fix-socket-close
branch
from
9aa282b
to
d460f9c
Compare
Fixed the test if we agree that |
| struct CurrentDirGuard { | ||
| path: PathBuf, | ||
| } | ||
| impl Drop for CurrentDirGuard { | ||
| fn drop(&mut self) { | ||
| let _ = env::set_current_dir(&self.path); | ||
| } | ||
| } |
There was a problem hiding this comment.
just comment: I haven't seen this pattern before, seems a good way to manage this kind of thing locally!
There was a problem hiding this comment.
I think there is a crate called scopeguard, which contains a defer! macro, can handle this more elegantly. I just don't want to introduce the dependency.
YJDoc2
added
kind/cleanup
We found we will get a residual fd in the process to call libcontainer, after we called exec of a container and exit.
and after some investigation, I found it is because the opened console_socket is not closed after exec build.