Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#define CONFIG_H

#define BUF_SIZE 8192
#define STDIO_BUF_SIZE 8192
#define DEF_STDOUT_BUF_SIZE 65536
#define CONN_SOCK_BUF_SIZE 32768
#define DEFAULT_SOCKET_PATH "/var/run/crio"
#define WIN_RESIZE_EVENT 1
Expand Down
15 changes: 15 additions & 0 deletions src/conmon.c
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,10 @@ int main(int argc, char *argv[])
pexit("Failed to create !terminal stdin pipe");

mainfd_stdin = fds[1];
ret = fcntl(mainfd_stdin, F_GETPIPE_SZ);
if (ret < 0)
pexit("main stdin pipe size determination failed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mainfd_stdin_size = (size_t)ret;
workerfd_stdin = fds[0];

if (g_unix_set_fd_nonblocking(mainfd_stdin, TRUE, NULL) == FALSE)
Expand All @@ -175,6 +179,10 @@ int main(int argc, char *argv[])
pexit("Failed to create !terminal stdout pipe");

mainfd_stdout = fds[0];
ret = fcntl(mainfd_stdout, F_GETPIPE_SZ);
if (ret < 0)
pexit("main stdout pipe size determination failed");
mainfd_stdout_size = (size_t)ret;
workerfd_stdout = fds[1];
}

Expand All @@ -194,6 +202,13 @@ int main(int argc, char *argv[])
pexit("Failed to create stderr pipe");

mainfd_stderr = fds[0];
ret = fcntl(mainfd_stderr, F_GETPIPE_SZ);
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why do we care about the sizes to be the same?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nwarn("main stderr and stdout pipe sizes don't match");
}
workerfd_stderr = fds[1];

GPtrArray *runtime_argv = configure_runtime_args(csname);
Expand Down
15 changes: 12 additions & 3 deletions src/ctr_logging.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#define _GNU_SOURCE
#include "ctr_logging.h"
#include "globals.h"
#include "cli.h"
#include "config.h"
#include <ctype.h>
Expand Down Expand Up @@ -355,10 +356,18 @@ static int parse_priority_prefix(const char *buf, size_t buflen, int *priority,
*/
static int write_journald(int pipe, char *buf, size_t buflen)
{
static char stdout_partial_buf[STDIO_BUF_SIZE];
static char *stdout_partial_buf = NULL;
static size_t stdout_partial_buf_len = 0;
static char stderr_partial_buf[STDIO_BUF_SIZE];
static char *stderr_partial_buf = NULL;
static size_t stderr_partial_buf_len = 0;
size_t buf_size = (pipe == STDOUT_PIPE ? mainfd_stdout_size : mainfd_stderr_size);

if (stdout_partial_buf == NULL) {
stdout_partial_buf = g_malloc(mainfd_stdout_size);
}
if (stderr_partial_buf == NULL) {
stderr_partial_buf = g_malloc(mainfd_stderr_size);
}

char *partial_buf;
size_t *partial_buf_len;
Expand Down Expand Up @@ -388,7 +397,7 @@ static int write_journald(int pipe, char *buf, size_t buflen)
/* If this is a partial line, and we have capacity to buffer it, buffer it and return.
* The capacity of the partial_buf is one less than its size so that we can always add
* a null terminating char later */
if (buflen && partial && ((unsigned long)line_len < (STDIO_BUF_SIZE - *partial_buf_len))) {
if (buflen && partial && ((unsigned long)line_len < (buf_size - *partial_buf_len))) {
memcpy(partial_buf + *partial_buf_len, buf, line_len);
*partial_buf_len += line_len;
return 0;
Expand Down
5 changes: 3 additions & 2 deletions src/ctr_stdio.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,14 @@ static void drain_log_buffers(stdpipe_t pipe)

static bool read_stdio(int fd, stdpipe_t pipe, gboolean *eof)
{
char buf[STDIO_BUF_SIZE];
size_t buf_size = ((pipe == STDOUT_PIPE) ? mainfd_stdout_size : mainfd_stderr_size);
char *buf = alloca(buf_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't alloca() here, please use g_malloc() to avoid possible stack limits?

ssize_t num_read = 0;

if (eof)
*eof = false;

num_read = read(fd, buf, STDIO_BUF_SIZE);
num_read = read(fd, buf, buf_size);
if (num_read == 0) {
if (eof)
*eof = true;
Expand Down
12 changes: 12 additions & 0 deletions src/ctrl.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,18 @@ gboolean terminal_accept_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC
mainfd_stdout = dup(console.fd);
if (mainfd_stdout < 0)
pexit("Failed to dup console file descriptor");
struct stat stat_s = {0};
int ret = fstat(mainfd_stdout, &stat_s);
if (ret < 0)
pexit("main stdout pipe fstat() failed");
if (S_ISFIFO(stat_s.st_mode)) {
ret = fcntl(mainfd_stdout, F_GETPIPE_SZ);
if (ret < 0)
pexit("main stdout pipe size determination failed");
mainfd_stdout_size = (size_t)ret;
} else {
mainfd_stdout_size = DEF_STDOUT_BUF_SIZE;
}

/* Now that we have a fd to the tty, make sure we handle any pending data
* that was already buffered. */
Expand Down
3 changes: 3 additions & 0 deletions src/globals.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ int runtime_status = -1;
int container_status = -1;

int mainfd_stdin = -1;
size_t mainfd_stdin_size = 0;
int mainfd_stdout = -1;
size_t mainfd_stdout_size = 0;
int mainfd_stderr = -1;
size_t mainfd_stderr_size = 0;

int attach_socket_fd = -1;
int console_socket_fd = -1;
Expand Down
3 changes: 3 additions & 0 deletions src/globals.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@ extern int runtime_status;
extern int container_status;

extern int mainfd_stdin;
extern size_t mainfd_stdin_size;
extern int mainfd_stdout;
extern size_t mainfd_stdout_size;
extern int mainfd_stderr;
extern size_t mainfd_stderr_size;

extern int attach_socket_fd;
extern int console_socket_fd;
Expand Down
Loading