Merge pull request #82 from haircommander/try_other_pipe

 Handle window resize events in a different fifo

cmd/conmon-config/conmon-config.go

@@ -16,10 +16,20 @@ func main() {
 #define STDIO_BUF_SIZE %d
 #define CONN_SOCK_BUF_SIZE %d
 #define DEFAULT_SOCKET_PATH "%s"
+#define WIN_RESIZE_EVENT %d
+#define REOPEN_LOGS_EVENT %d
 
 #endif // CONFIG_H
 `
-	if err := ioutil.WriteFile("config.h", []byte(fmt.Sprintf(output, config.BufSize, config.BufSize, config.ConnSockBufSize, config.ContainerAttachSocketDir)), 0644); err != nil {
-		fmt.Errorf(err.Error())
+	if err := ioutil.WriteFile("config.h", []byte(fmt.Sprintf(
+			output,
+			config.BufSize,
+			config.BufSize,
+			config.ConnSockBufSize,
+			config.ContainerAttachSocketDir,
+			config.WinResizeEvent,
+			config.ReopenLogsEvent)),
+		0644); err != nil {
+			fmt.Errorf(err.Error())
 	}
 }

runner/config/config.go

@@ -6,4 +6,11 @@ const (
 	// ConnSockBufSize is the size of the socket used for
 	// to attach to the container
 	ConnSockBufSize = 32768
+	// WinResizeEvent is the event code the caller program will
+	// send along the ctrl fd to signal conmon to resize
+	// the pty window
+	WinResizeEvent = 1
+	// ReopenLogsEvent is the event code the caller program will
+	// send along the ctrl fd to signal conmon to reopen the log files
+	ReopenLogsEvent = 2
 )

src/config.h

@@ -6,5 +6,7 @@
 #define STDIO_BUF_SIZE 8192
 #define CONN_SOCK_BUF_SIZE 32768
 #define DEFAULT_SOCKET_PATH "/var/run/crio"
+#define WIN_RESIZE_EVENT 1
+#define REOPEN_LOGS_EVENT 2
 
 #endif // CONFIG_H

src/conmon.c

@@ -145,7 +145,6 @@ static ssize_t write_all(int fd, const void *buf, size_t count)
 	return count;
 }
 
-
 /*
  * Returns the path for specified controller name for a pid.
  * Returns NULL on error.
@@ -292,6 +291,8 @@ static int attach_socket_fd = -1;
 static int console_socket_fd = -1;
 static int terminal_ctrl_fd = -1;
 static int inotify_fd = -1;
+static int winsz_fd_w = -1;
+static int winsz_fd_r = -1;
 
 static gboolean timed_out = FALSE;
 
@@ -683,48 +684,35 @@ static void resize_winsz(int height, int width)
 }
 
 #define CTLBUFSZ 200
-static gboolean ctrl_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC_UNUSED gpointer user_data)
+/*
+ * read_from_ctrl_buffer reads a line (of no more than CTLBUFSZ) from an fd,
+ * and calls line_process_func. It is a generic way to handle input on an fd
+ * line_process_func should return TRUE if it succeeds, and FALSE if it fails
+ * to process the line.
+ */
+static gboolean read_from_ctrl_buffer(int fd, gboolean(*line_process_func)(char*))
 {
 	static char ctlbuf[CTLBUFSZ];
 	static int readsz = CTLBUFSZ - 1;
 	static char *readptr = ctlbuf;
 	ssize_t num_read = 0;
-	int ctl_msg_type = -1;
-	int height = -1;
-	int width = -1;
-	int ret;
 
 	num_read = read(fd, readptr, readsz);
 	if (num_read <= 0) {
-		nwarn("Failed to read from control fd");
+		nwarnf("Failed to read from fd %d", fd);
 		return G_SOURCE_CONTINUE;
 	}
 
 	readptr[num_read] = '\0';
-	ninfof("Got ctl message: %s", ctlbuf);
+	ninfof("Got ctl message: %s on fd %d", ctlbuf, fd);
 
 	char *beg = ctlbuf;
 	char *newline = strchrnul(beg, '\n');
 	/* Process each message which ends with a line */
 	while (*newline != '\0') {
-		ret = sscanf(ctlbuf, "%d %d %d\n", &ctl_msg_type, &height, &width);
-		if (ret != 3) {
-			nwarn("Failed to sscanf message");
+		if (!line_process_func(ctlbuf)) {
 			return G_SOURCE_CONTINUE;
 		}
-		ninfof("Message type: %d, Height: %d, Width: %d", ctl_msg_type, height, width);
-		switch (ctl_msg_type) {
-		// This matches what we write from container_attach.go
-		case 1:
-			resize_winsz(height, width);
-			break;
-		case 2:
-			reopen_log_files();
-			break;
-		default:
-			ninfof("Unknown message type: %d", ctl_msg_type);
-			break;
-		}
 		beg = newline + 1;
 		newline = strchrnul(beg, '\n');
 	}
@@ -755,6 +743,78 @@ static gboolean ctrl_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC_UNU
 	return G_SOURCE_CONTINUE;
 }
 
+/*
+ * process_terminal_ctrl_line takes a line from the
+ * caller program (received through the terminal ctrl fd)
+ * and either writes to the winsz fd (to handle terminal resize events)
+ * or reopens log files.
+ */
+static gboolean process_terminal_ctrl_line(char* line)
+{
+	int ctl_msg_type, height, width, ret = -1;
+	_cleanup_free_ char *hw_str = NULL;
+
+	// while the height and width won't be used in this function,
+	// we want to remove them from the buffer anyway
+	ret = sscanf(line, "%d %d %d\n", &ctl_msg_type, &height, &width);
+	if (ret != 3) {
+		nwarn("Failed to sscanf message");
+		return FALSE;
+	}
+
+	ninfof("Message type: %d", ctl_msg_type);
+	switch (ctl_msg_type) {
+	case WIN_RESIZE_EVENT:
+		hw_str = g_strdup_printf("%d %d\n", height, width);
+		if (write(winsz_fd_w, hw_str, strlen(hw_str)) < 0) {
+			nwarn("Failed to write to window resizing fd. A resize event may have been dropped");
+			return FALSE;
+		}
+		break;
+	case REOPEN_LOGS_EVENT:
+		reopen_log_files();
+		break;
+	default:
+		ninfof("Unknown message type: %d", ctl_msg_type);
+		break;
+	}
+	return TRUE;
+}
+
+/*
+ * ctrl_cb is a callback for handling events directly from the caller
+ */
+static gboolean ctrl_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC_UNUSED gpointer user_data)
+{
+	return read_from_ctrl_buffer(fd, process_terminal_ctrl_line);
+}
+
+/*
+ * process_winsz_ctrl_line processes a line passed to the winsz fd
+ * after the terminal_ctrl fd receives a winsz event.
+ * It reads a height and length, and resizes the pty with it.
+ */
+static gboolean process_winsz_ctrl_line(char * line)
+{
+	int height, width, ret = -1;
+	ret = sscanf(line, "%d %d\n", &height, &width);
+	ninfof("Height: %d, Width: %d", height, width);
+	if (ret != 2) {
+		nwarn("Failed to sscanf message");
+		return FALSE;
+	}
+	resize_winsz(height, width);
+	return TRUE;
+}
+
+/*
+ * ctrl_winsz_cb is a callback after a window resize event is sent along the winsz fd.
+ */
+static gboolean ctrl_winsz_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC_UNUSED gpointer user_data)
+{
+	return read_from_ctrl_buffer(fd, process_winsz_ctrl_line);
+}
+
 static gboolean terminal_accept_cb(int fd, G_GNUC_UNUSED GIOCondition condition, G_GNUC_UNUSED gpointer user_data)
 {
 	const char *csname = user_data;
@@ -797,10 +857,10 @@ static gboolean terminal_accept_cb(int fd, G_GNUC_UNUSED GIOCondition condition,
 	masterfd_stdin = console.fd;
 	masterfd_stdout = console.fd;
 
-	/* now that we've set masterfd_stdout, we can register the ctrl_cb
+	/* now that we've set masterfd_stdout, we can register the ctrl_winsz_cb
 	 * if we didn't set it here, we'd risk attempting to run ioctl on
 	 * a negative fd, and fail to resize the window */
-	g_unix_fd_add(terminal_ctrl_fd, G_IO_IN, ctrl_cb, NULL);
+	g_unix_fd_add(winsz_fd_r, G_IO_IN, ctrl_winsz_cb, NULL);
 
 	/* Clean up everything */
 	close(connfd);
@@ -974,29 +1034,38 @@ static char *setup_attach_socket(void)
 	return attach_symlink_dir_path;
 }
 
-static int setup_terminal_control_fifo()
-{
-	_cleanup_free_ char *ctl_fifo_path = g_build_filename(opt_bundle_path, "ctl", NULL);
-	ninfof("ctl fifo path: %s", ctl_fifo_path);
+static void setup_fifo(int *fifo_r, int *fifo_w, char * filename, char* error_var_name) {
+	_cleanup_free_ char *fifo_path = g_build_filename(opt_bundle_path, filename, NULL);
+
+	if (!fifo_r || !fifo_w)
+		pexitf("setup fifo was passed a NULL pointer");
 
-	/* Setup fifo for reading in terminal resize and other stdio control messages */
+	if (mkfifo(fifo_path, 0666) == -1)
+		pexitf("Failed to mkfifo at %s", fifo_path);
 
-	if (mkfifo(ctl_fifo_path, 0666) == -1)
-		pexitf("Failed to mkfifo at %s", ctl_fifo_path);
+	if ((*fifo_r = open(fifo_path, O_RDONLY | O_NONBLOCK | O_CLOEXEC)) == -1)
+		pexitf("Failed to open %s read half", error_var_name);
 
-	terminal_ctrl_fd = open(ctl_fifo_path, O_RDONLY | O_NONBLOCK | O_CLOEXEC);
-	if (terminal_ctrl_fd == -1)
-		pexit("Failed to open control fifo");
+	if ((*fifo_w = open(fifo_path, O_WRONLY | O_CLOEXEC)) == -1)
+		pexitf("Failed to open %s write half", error_var_name);
+}
+
+static void setup_console_fifo() {
+	setup_fifo(&winsz_fd_r, &winsz_fd_w, "winsz", "window resize control fifo");
+	ninfof("winsz read side: %d, winsz write side: %d", winsz_fd_r, winsz_fd_r);
+}
 
+static int setup_terminal_control_fifo()
+{
 	/*
 	 * Open a dummy writer to prevent getting flood of POLLHUPs when
 	 * last writer closes.
 	 */
-	int dummyfd = open(ctl_fifo_path, O_WRONLY | O_CLOEXEC);
-	if (dummyfd == -1)
-		pexit("Failed to open dummy writer for fifo");
-
+	int dummyfd = -1;
+	setup_fifo(&terminal_ctrl_fd, &dummyfd, "ctl", "terminal control fifo");
 	ninfof("terminal_ctrl_fd: %d", terminal_ctrl_fd);
+	g_unix_fd_add(terminal_ctrl_fd, G_IO_IN, ctrl_cb, NULL);
+
 	return dummyfd;
 }
 
@@ -1326,10 +1395,10 @@ int main(int argc, char *argv[])
 		masterfd_stdout = fds[0];
 		slavefd_stdout = fds[1];
 
-		/* now that we've set masterfd_stdout, we can register the ctrl_cb
+		/* now that we've set masterfd_stdout, we can register the ctrl_winsz_cb
 		 * if we didn't set it here, we'd risk attempting to run ioctl on
 		 * a negative fd, and fail to resize the window */
-		g_unix_fd_add(terminal_ctrl_fd, G_IO_IN, ctrl_cb, NULL);
+		g_unix_fd_add(winsz_fd_r, G_IO_IN, ctrl_winsz_cb, NULL);
 	}
 
 	/* We always create a stderr pipe, because that way we can capture
@@ -1524,6 +1593,7 @@ int main(int argc, char *argv[])
 	if (opt_bundle_path != NULL) {
 		attach_symlink_dir_path = setup_attach_socket();
 		dummyfd = setup_terminal_control_fifo();
+		setup_console_fifo();
 
 		if (opt_attach) {
 			ndebug("sending attach message to parent");