Use the actual pipe size instead of 8,192 #615
Conversation
| mainfd_stdin = fds[1]; | ||
| ret = fcntl(mainfd_stdin, F_GETPIPE_SZ); | ||
| if (ret < 0) | ||
| pexit("main stdin pipe size determination failed"); |
There was a problem hiding this comment.
I admit the fcntl should never fail, since we've just created the pipe and it will just be valid, but could we maybe default to DEF_STDOUT_BUF_SIZE also in this case and also for mainfd_stdout_size below? It seems to be more fail-safe way than simply exiting.
There was a problem hiding this comment.
I agree this one seems to be way brutal, just warning and mainfd_stdin_size = DEF_STDOUT_BUF_SIZE; would do it with grace it seems.
| if (ret < 0) | ||
| pexit("main stderr pipe size determination failed"); | ||
| mainfd_stderr_size = (size_t)ret; | ||
| if ((mainfd_stdout >= 0) && (mainfd_stderr_size != mainfd_stdout_size)) { |
There was a problem hiding this comment.
Out of curiosity, why do we care about the sizes to be the same?
There was a problem hiding this comment.
I've never seen these being different in real life. As a matter of fact I'd remove the whole condition as it never triggers.
|
I will comment on the first commit in the #614. |
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>
The `make clean` target now cleans up the `/tmp` area a bit. It leaves around the cache of the `busybox` image to avoid fetching it too often. Signed-off-by: Peter Portante <peter.portante@redhat.com>
A few of the parameters were named `num_read` "leaking" the use it was named for in a calling function. We use `buflen` with `buf` more consistently, and use `size_t` were possible since we don't need or want to pass a negative value for the size of a buffer. Signed-off-by: Peter Portante <peter.portante@redhat.com>
The default for linux pipes is typically something much larger than 8,192 bytes. We take advantage of this to fact to avoid multiple rounds of reading from a full pipe. The constant, `STDIO_BUF_SIZE` is removed in favor of `fcntl(F_GETPIPE_SZ)` calls for pipes / fifos, an adding `DEF_STDIO_BUF_SIZE` for the case where `stdout` is not a PIPE (perhaps character special file) using a 64K buffer to consume more data per system when possible. Signed-off-by: Peter Portante <peter.portante@redhat.com>
| mainfd_stdin = fds[1]; | ||
| ret = fcntl(mainfd_stdin, F_GETPIPE_SZ); | ||
| if (ret < 0) | ||
| pexit("main stdin pipe size determination failed"); |
There was a problem hiding this comment.
I agree this one seems to be way brutal, just warning and mainfd_stdin_size = DEF_STDOUT_BUF_SIZE; would do it with grace it seems.
| { | ||
| char buf[STDIO_BUF_SIZE]; | ||
| size_t buf_size = ((pipe == STDOUT_PIPE) ? mainfd_stdout_size : mainfd_stderr_size); | ||
| char *buf = alloca(buf_size); |
There was a problem hiding this comment.
I wouldn't alloca() here, please use g_malloc() to avoid possible stack limits?
| if (ret < 0) | ||
| pexit("main stderr pipe size determination failed"); | ||
| mainfd_stderr_size = (size_t)ret; | ||
| if ((mainfd_stdout >= 0) && (mainfd_stderr_size != mainfd_stdout_size)) { |
There was a problem hiding this comment.
I've never seen these being different in real life. As a matter of fact I'd remove the whole condition as it never triggers.
3 participants
The default for linux pipes is typically something much larger than 8,192 bytes.
We take advantage of this to fact to avoid multiple rounds of reading from a
full pipe.
The constant,
STDIO_BUF_SIZEis removed in favor offcntl(F_GETPIPE_SZ)calls for pipes / fifos, an adding
DEF_STDIO_BUF_SIZEfor the case wherestdoutis not a PIPE (perhaps character special file) using a 64K buffer toconsume more data per system when possible.
Signed-off-by: Peter Portante peter.portante@redhat.com
The goal of this work is to make it much easier to implement #264.