conmon: drop usage of splice(2) #131
Conversation
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
splice(2) doesn't seem to handle correctly sockets having a type of SOCK_SEQPACKET. When using a SOCK_SEQPACKET, each read is limited to the current packet. The data that doesn't fit in a single read(2) call will be discarded. This can be a problem when the fd_out doesn't accept all the data, so on a short write we lose the data that could not be written by the splice(2) call. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
good find! |
|
fixed the comments and pushed a new version |
| if (w < 0) { | ||
| nwarn("Failed to write to container stdin"); | ||
| } else { |
There was a problem hiding this comment.
stylistically, this is inconsistent with line 727 WRT the braces on a one line if statement. I don't mind which, but we should probably choose one for the project 😄
|
|
||
| /* There is still data in the buffer. */ | ||
| if (sock->remaining) { | ||
| sock->data_ready = true; |
There was a problem hiding this comment.
same here. Above has_data is capital, but here is lower case. I vote we choose one and stick to it
|
sorry, a couple more nits, then LGTM |
|
uh @cevich PTAL |
|
Are we thinking this is the cause of our exec bug? If so, it's actually amazing that it only happens with exec - this definitely looks like a case that run would hit as well. Regardless, great catch @giuseppe |
|
@giuseppe is this still a draft? |
I could easily reproduce the issue you've reported locally and this patch solves it for me. Do we use a different buffer size in Podman for |
marked ready for review |
|
LGTM (test failures are not the fault of this PR) |
|
jk they're not fixed yet |
now that we dropped splice(2), we need to make sure the read/write loop doesn't hang indefinitely conmon. Attempting to write to a fd that is not ready for write causes conmon to block on the write(2) call, at the same time the container process might be blocked attempting to write to stderr but since conmon is not processing any data from the container, both processes are dead locked. Keep in a buffer what we read from the pipe and write to the masterfd_stdin only when it is ready for write. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
Other than @haircommander's nits and unhappy tests, LGTM |
|
FYI: Cirrus-CI testing in GCP should un-break shortly (hours). |
|
tests are green |
|
Need to cut a release? |
splice(2) doesn't seem to handle correctly sockets having a type of SOCK_SEQPACKET.
When using a SOCK_SEQPACKET, each read is limited to the current packet. The data that doesn't fit in a single read(2) call will be discarded. This can be a problem when the fd_out doesn't accept all the data, so on a short write we lose the data that could not be written by the splice(2) call.
Replace splice(2) with splitting the read/write loop.
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com