Remove need for extra bytes for remote consoles #614
Conversation
|
Would it be possible to make it atomic? |
portante
force-pushed
the
remove-extra-bytes
branch
from
57da497 to
d40339b
Compare
Not sure this is what you are thinking, but I believe this solves the request to make it atomic. If the direction seems like a good one, I'll squash the commits into one. |
portante
force-pushed
the
remove-extra-bytes
branch
from
9812fa4 to
357bd1c
Compare
jnovy
left a comment
jnovy
left a comment
There was a problem hiding this comment.
Also please squash both commits.
portante
force-pushed
the
remove-extra-bytes
branch
from
357bd1c to
8943c47
Compare
|
LGTM |
src/conn_sock.c
Outdated
| return; | ||
| struct iovec iov[2] = {{&pipe, 1}, {buf, buflen}}; | ||
| writev_iov_t wviov = {2, 2, iov}; | ||
| g_ptr_array_foreach(local_mainfd_stdin.readers, write_remote_sock, &wviov); |
There was a problem hiding this comment.
The wviov is created few lines above, passed as pointer down to write_remote_sock for each local_mainfd_stdin.reader. I think the issue is that the first writev_all call changes the wviov and the second call of writev_all for the second reader from local_mainfd_stdin.readers will get a wviov with buf->iovcnt = 0;, effectively writing nothing.
I think we will have to rebuild the writev_iov_t in the write_remote_sock or change the writev_all to operate on a local copy and do not mutate it.
I think it would be also great to add a bats test for two readers, so this use-case is covered.
There was a problem hiding this comment.
I think I have a way to address this.
Would like to be pointed in the right direction for the two readers use case.
src/conn_sock.c
Outdated
| return; | ||
| struct iovec iov[2] = {{&pipe, 1}, {buf, buflen}}; | ||
| writev_iov_t wviov = {2, 2, iov}; | ||
| g_ptr_array_foreach(local_mainfd_stdin.readers, write_remote_sock, &wviov); |
There was a problem hiding this comment.
I think another issue here is that change of order of array iteration.
Previously, the local_mainfd_stdin.readers was iterated in the reverse order. I believe the reason was that the remote_sock_shutdown can remove the reader from the array (It calls g_ptr_array_remove).
The proposed code uses g_ptr_array_foreach. The documentation for this says func must not add elements to or remove elements from the array., but this is what can actually happen in the remote_sock_shutdown.
I think the way forward is to keep the reverse iteration.
There was a problem hiding this comment.
The reverse iteration breaks encapsulation and adds to the fragility of the code.
Let's me propose an alternative before we maintain this broken encapsulation.
|
I'm not sure what local crash you see. It would be great to generate the coredump and check the backtrace to see what's going on there. |
I'm traveling this week, so most likely I'll recreate this next week. Thanks! |
portante
force-pushed
the
remove-extra-bytes
branch
from
8943c47 to
5a907a9
Compare
I am unable to reproduce the crash on |
We use `writev()` for remote consoles to remove the need for extra bytes surrounding the data to be written. We really don't want to "leak" the protocol requirement for remote consoles to the rest of the code base. To hide that, and avoid two `write_all()` method calls in a row, we promote `writev_buffer_flush()` to a `utils` method called `writev_all()` to leverage I/O vectors in one call (memory copies will need to occur for remote sockets, so using `writev()` avoids that). Further, we use the `g_ptr_array_foreach()` method instead of doing it ourselves to avoid breaking the `GPtrArray` encapsulation. Signed-off-by: Peter Portante <peter.portante@redhat.com>
portante
force-pushed
the
remove-extra-bytes
branch
from
5a907a9 to
54e0322
Compare
4 participants
We use
writev()for remote consoles to remove the need for extra bytes surrounding the data to be written.We really don't want to "leak" the protocol requirement for remote consoles to the rest of the code base. To hide that, and avoid two
write_all()method calls in a row, we promotewritev_buffer_flush()to autilsmethod calledwritev_all()to leverage I/O vectors in one call (memory copies will need to occur for remote sockets, so usingwritev()avoids that).Further, we use the
g_ptr_array_foreach()method instead of doing it ourselves to avoid breaking theGPtrArrayencapsulation.Signed-off-by: Peter Portante peter.portante@redhat.com
FWIW, this PR is a pre-requisite to #615, which in turn will make it easier to finally implement #264.