From e5b3af8ffe9f14ef47225261584ab186d5230849 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Sun, 15 Jan 2017 15:31:33 -0800 Subject: [PATCH] GUACAMOLE-86: Remove terminal emulator's STDOUT pipe. Use pthread_cond_t to signal modification. --- src/protocols/ssh/ssh.c | 2 +- src/protocols/telnet/telnet.c | 4 +- src/terminal/Makefile.am | 2 - src/terminal/common.c | 20 ---- src/terminal/common.h | 21 ---- src/terminal/packet.c | 64 ------------ src/terminal/packet.h | 89 ---------------- src/terminal/terminal.c | 187 +++++++++++++++++----------------- src/terminal/terminal.h | 35 +++---- 9 files changed, 117 insertions(+), 307 deletions(-) delete mode 100644 src/terminal/packet.c delete mode 100644 src/terminal/packet.h diff --git a/src/protocols/ssh/ssh.c b/src/protocols/ssh/ssh.c index db3cea96..4bdf76f7 100644 --- a/src/protocols/ssh/ssh.c +++ b/src/protocols/ssh/ssh.c @@ -339,7 +339,7 @@ void* ssh_client_thread(void* data) { /* Attempt to write data received. Exit on failure. */ if (bytes_read > 0) { - int written = guac_terminal_write_stdout(ssh_client->term, buffer, bytes_read); + int written = guac_terminal_write(ssh_client->term, buffer, bytes_read); if (written < 0) break; diff --git a/src/protocols/telnet/telnet.c b/src/protocols/telnet/telnet.c index 6ca9aba7..531819f2 100644 --- a/src/protocols/telnet/telnet.c +++ b/src/protocols/telnet/telnet.c @@ -150,7 +150,7 @@ static void __guac_telnet_event_handler(telnet_t* telnet, telnet_event_t* event, /* Terminal output received */ case TELNET_EV_DATA: - guac_terminal_write_stdout(telnet_client->term, event->data.buffer, event->data.size); + guac_terminal_write(telnet_client->term, event->data.buffer, event->data.size); /* Continue search for username prompt */ if (settings->username_regex != NULL) { @@ -267,7 +267,7 @@ static void* __guac_telnet_input_thread(void* data) { while ((bytes_read = guac_terminal_read_stdin(telnet_client->term, buffer, sizeof(buffer))) > 0) { telnet_send(telnet_client->telnet, buffer, bytes_read); if (telnet_client->echo_enabled) - guac_terminal_write_stdout(telnet_client->term, buffer, bytes_read); + guac_terminal_write(telnet_client->term, buffer, bytes_read); } return NULL; diff --git a/src/terminal/Makefile.am b/src/terminal/Makefile.am index 19cbf949..59bcd123 100644 --- a/src/terminal/Makefile.am +++ b/src/terminal/Makefile.am @@ -27,7 +27,6 @@ noinst_HEADERS = \ char_mappings.h \ common.h \ display.h \ - packet.h \ scrollbar.h \ terminal.h \ terminal_handlers.h \ @@ -39,7 +38,6 @@ libguac_terminal_la_SOURCES = \ char_mappings.c \ common.c \ display.c \ - packet.c \ scrollbar.c \ terminal.c \ terminal_handlers.c \ diff --git a/src/terminal/common.c b/src/terminal/common.c index cc982767..4018e771 100644 --- a/src/terminal/common.c +++ b/src/terminal/common.c @@ -105,23 +105,3 @@ int guac_terminal_write_all(int fd, const char* buffer, int size) { } -int guac_terminal_fill_buffer(int fd, char* buffer, int size) { - - int remaining = size; - while (remaining > 0) { - - /* Attempt to read data */ - int ret_val = read(fd, buffer, remaining); - if (ret_val <= 0) - return -1; - - /* If successful, continue with what space remains (if any) */ - remaining -= ret_val; - buffer += ret_val; - - } - - return size; - -} - diff --git a/src/terminal/common.h b/src/terminal/common.h index 495bf30b..f7eda619 100644 --- a/src/terminal/common.h +++ b/src/terminal/common.h @@ -49,26 +49,5 @@ bool guac_terminal_has_glyph(int codepoint); */ int guac_terminal_write_all(int fd, const char* buffer, int size); -/** - * Similar to read, but automatically retries the read until an error occurs, - * filling all available space within the buffer. Unless it is known that the - * given amount of space is available on the file descriptor, there is a good - * chance this function will block. - * - * @param fd - * The file descriptor to read data from. - * - * @param buffer - * The buffer to store data within. - * - * @param size - * The number of bytes available within the buffer. - * - * @return - * The number of bytes read if successful, or a negative value if an error - * occurs. - */ -int guac_terminal_fill_buffer(int fd, char* buffer, int size); - #endif diff --git a/src/terminal/packet.c b/src/terminal/packet.c deleted file mode 100644 index ad828ceb..00000000 --- a/src/terminal/packet.c +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#include "common.h" -#include "packet.h" - -#include - -int guac_terminal_packet_write(int fd, const void* data, int length) { - - guac_terminal_packet out; - - /* Do not attempt to write packets beyond maximum size */ - if (length > GUAC_TERMINAL_PACKET_SIZE) - return -1; - - /* Calculate final packet length */ - int packet_length = sizeof(int) + length; - - /* Copy data into packet */ - out.length = length; - memcpy(out.data, data, length); - - /* Write packet */ - return guac_terminal_write_all(fd, (const char*) &out, packet_length); - -} - -int guac_terminal_packet_read(int fd, void* data, int length) { - - int bytes; - - /* Read buffers MUST be at least GUAC_TERMINAL_PACKET_SIZE */ - if (length < GUAC_TERMINAL_PACKET_SIZE) - return -1; - - /* Read length */ - if (guac_terminal_fill_buffer(fd, (char*) &bytes, sizeof(int)) < 0) - return -1; - - /* Read body */ - if (guac_terminal_fill_buffer(fd, (char*) data, bytes) < 0) - return -1; - - return bytes; - -} - diff --git a/src/terminal/packet.h b/src/terminal/packet.h deleted file mode 100644 index 00d30520..00000000 --- a/src/terminal/packet.h +++ /dev/null @@ -1,89 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#ifndef GUAC_TERMINAL_PACKET_H -#define GUAC_TERMINAL_PACKET_H - -/** - * The maximum size of a packet written or read by the - * guac_terminal_packet_write() or guac_terminal_packet_read() functions. - */ -#define GUAC_TERMINAL_PACKET_SIZE 4096 - -/** - * An arbitrary data packet with minimal framing. - */ -typedef struct guac_terminal_packet { - - /** - * The number of bytes in the data portion of this packet. - */ - int length; - - /** - * Arbitrary data. - */ - char data[GUAC_TERMINAL_PACKET_SIZE]; - -} guac_terminal_packet; - -/** - * Writes a single packet of data to the given file descriptor. The provided - * length MUST be no greater than GUAC_TERMINAL_PACKET_SIZE. Zero-length - * writes are legal and do result in a packet being written to the file - * descriptor. - * - * @param fd - * The file descriptor to write to. - * - * @param data - * A buffer containing the data to write. - * - * @param length - * The number of bytes to write to the file descriptor. - * - * @return - * The number of bytes written on success, which may be zero if the data - * length is zero, or a negative value on error. - */ -int guac_terminal_packet_write(int fd, const void* data, int length); - -/** - * Reads a single packet of data from the given file descriptor. The provided - * length MUST be at least GUAC_TERMINAL_PACKET_SIZE to ensure any packet - * read will fit in the buffer. Zero-length reads are possible if a zero-length - * packet was written. - * - * @param fd - * The file descriptor to read from. - * - * @param data - * The buffer to store data within. - * - * @param length - * The number of bytes available within the buffer. - * - * @return - * The number of bytes read on success, which may be zero if the read - * packet had a length of zero, or a negative value on error. - */ -int guac_terminal_packet_read(int fd, void* data, int length); - -#endif - diff --git a/src/terminal/terminal.c b/src/terminal/terminal.c index 598a3852..7488a310 100644 --- a/src/terminal/terminal.c +++ b/src/terminal/terminal.c @@ -24,7 +24,6 @@ #include "display.h" #include "guac_clipboard.h" #include "guac_cursor.h" -#include "packet.h" #include "scrollbar.h" #include "terminal.h" #include "terminal_handlers.h" @@ -32,7 +31,6 @@ #include "typescript.h" #include -#include #include #include #include @@ -318,6 +316,11 @@ guac_terminal* guac_terminal_create(guac_client* client, term->upload_path_handler = NULL; term->file_download_handler = NULL; + /* Init modified flag and conditional */ + term->modified = 0; + pthread_cond_init(&(term->modified_cond), NULL); + pthread_mutex_init(&(term->modified_lock), NULL); + /* Init buffer */ term->buffer = guac_terminal_buffer_alloc(1000, &default_char); @@ -349,14 +352,6 @@ guac_terminal* guac_terminal_create(guac_client* client, term->term_width = available_width / term->display->char_width; term->term_height = height / term->display->char_height; - /* Open STDOUT pipe */ - if (pipe(term->stdout_pipe_fd)) { - guac_error = GUAC_STATUS_SEE_ERRNO; - guac_error_message = "Unable to open pipe for STDOUT"; - free(term); - return NULL; - } - /* Open STDIN pipe */ if (pipe(term->stdin_pipe_fd)) { guac_error = GUAC_STATUS_SEE_ERRNO; @@ -414,10 +409,6 @@ guac_terminal* guac_terminal_create(guac_client* client, void guac_terminal_free(guac_terminal* term) { - /* Close terminal output pipe */ - close(term->stdout_pipe_fd[1]); - close(term->stdout_pipe_fd[0]); - /* Close user input pipe */ close(term->stdin_pipe_fd[1]); close(term->stdin_pipe_fd[0]); @@ -449,84 +440,110 @@ void guac_terminal_free(guac_terminal* term) { } /** - * Waits for data to become available on the given file descriptor. + * Populate the given timespec with the current time, plus the given offset. * - * @param fd - * The file descriptor to wait on. + * @param ts + * The timespec structure to populate. + * + * @param offset_sec + * The offset from the current time to use when populating the given + * timespec, in seconds. + * + * @param offset_usec + * The offset from the current time to use when populating the given + * timespec, in microseconds. + */ +static void guac_terminal_get_absolute_time(struct timespec* ts, + int offset_sec, int offset_usec) { + + /* Get timeval */ + struct timeval tv; + gettimeofday(&tv, NULL); + + /* Update with offset */ + tv.tv_sec += offset_sec; + tv.tv_usec += offset_usec; + + /* Wrap to next second if necessary */ + if (tv.tv_usec >= 1000000) { + tv.tv_sec++; + tv.tv_usec -= 1000000; + } + + /* Convert to timespec */ + ts->tv_sec = tv.tv_sec; + ts->tv_nsec = tv.tv_usec * 1000; + +} + +/** + * Waits for the terminal state to be modified, returning only when the + * specified timeout has elapsed or a frame flush is desired. Note that the + * modified flag of the terminal will only be reset if no data remains to be + * read from STDOUT. + * + * @param terminal + * The terminal to wait on. * * @param msec_timeout * The maximum amount of time to wait, in milliseconds. * * @return - * A positive if data is available, zero if the timeout has elapsed without - * data becoming available, or negative if an error occurred. + * Non-zero if the terminal has been modified, zero if the timeout has + * elapsed without the terminal being modified. */ -static int guac_terminal_wait_for_data(int fd, int msec_timeout) { +static int guac_terminal_wait(guac_terminal* terminal, int msec_timeout) { - /* Build array of file descriptors */ - struct pollfd fds[] = {{ - .fd = fd, - .events = POLLIN, - .revents = 0, - }}; + int retval = 1; - /* Wait for data */ - return poll(fds, 1, msec_timeout); + pthread_mutex_t* mod_lock = &(terminal->modified_lock); + pthread_cond_t* mod_cond = &(terminal->modified_cond); + + /* Split provided milliseconds into microseconds and whole seconds */ + int secs = msec_timeout / 1000; + int usecs = (msec_timeout % 1000) * 1000; + + /* Calculate absolute timestamp from provided relative timeout */ + struct timespec timeout; + guac_terminal_get_absolute_time(&timeout, secs, usecs); + + /* Test for terminal modification */ + pthread_mutex_lock(mod_lock); + if (terminal->modified) + goto wait_complete; + + /* If not yet modified, wait for modification condition to be signaled */ + retval = pthread_cond_timedwait(mod_cond, mod_lock, &timeout) != ETIMEDOUT; + +wait_complete: + + /* Terminal is no longer modified */ + terminal->modified = 0; + pthread_mutex_unlock(mod_lock); + return retval; } int guac_terminal_render_frame(guac_terminal* terminal) { - guac_client* client = terminal->client; - char buffer[GUAC_TERMINAL_PACKET_SIZE]; - int wait_result; - int fd = terminal->stdout_pipe_fd[0]; /* Wait for data to be available */ - wait_result = guac_terminal_wait_for_data(fd, 1000); - if (wait_result > 0) { + wait_result = guac_terminal_wait(terminal, 1000); + if (wait_result) { - guac_terminal_lock(terminal); guac_timestamp frame_start = guac_timestamp_current(); do { - guac_timestamp frame_end; - int frame_remaining; - - int bytes_read; - - /* Read data, write to terminal */ - if ((bytes_read = guac_terminal_packet_read(fd, - buffer, sizeof(buffer))) > 0) { - - if (guac_terminal_write(terminal, buffer, bytes_read)) { - guac_client_abort(client, - GUAC_PROTOCOL_STATUS_SERVER_ERROR, - "Error writing data"); - guac_terminal_unlock(terminal); - return 1; - } - - } - - /* Notify on error */ - if (bytes_read < 0) { - guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, - "Error reading data"); - guac_terminal_unlock(terminal); - return 1; - } - /* Calculate time remaining in frame */ - frame_end = guac_timestamp_current(); - frame_remaining = frame_start + GUAC_TERMINAL_FRAME_DURATION - - frame_end; + guac_timestamp frame_end = guac_timestamp_current(); + int frame_remaining = frame_start + GUAC_TERMINAL_FRAME_DURATION + - frame_end; /* Wait again if frame remaining */ if (frame_remaining > 0) - wait_result = guac_terminal_wait_for_data(fd, + wait_result = guac_terminal_wait(terminal, GUAC_TERMINAL_FRAME_TIMEOUT); else break; @@ -534,18 +551,12 @@ int guac_terminal_render_frame(guac_terminal* terminal) { } while (wait_result > 0); /* Flush terminal */ + guac_terminal_lock(terminal); guac_terminal_flush(terminal); guac_terminal_unlock(terminal); } - /* Notify of any errors */ - if (wait_result < 0) { - guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, - "Error waiting for data"); - return 1; - } - return 0; } @@ -555,28 +566,19 @@ int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size) { return read(stdin_fd, c, size); } -int guac_terminal_write_stdout(guac_terminal* terminal, const char* c, - int size) { +void guac_terminal_notify(guac_terminal* terminal) { - /* Write maximally-sized packets until only one packet remains */ - while (size > GUAC_TERMINAL_PACKET_SIZE) { + pthread_mutex_t* mod_lock = &(terminal->modified_lock); + pthread_cond_t* mod_cond = &(terminal->modified_cond); - /* Write maximally-sized packet */ - if (guac_terminal_packet_write(terminal->stdout_pipe_fd[1], c, - GUAC_TERMINAL_PACKET_SIZE) < 0) - return -1; + pthread_mutex_lock(mod_lock); - /* Advance to next packet */ - c += GUAC_TERMINAL_PACKET_SIZE; - size -= GUAC_TERMINAL_PACKET_SIZE; + /* Signal modification */ + terminal->modified = 1; + pthread_cond_signal(mod_cond); - } + pthread_mutex_unlock(mod_lock); - return guac_terminal_packet_write(terminal->stdout_pipe_fd[1], c, size); -} - -int guac_terminal_notify(guac_terminal* terminal) { - return guac_terminal_packet_write(terminal->stdout_pipe_fd[1], NULL, 0); } int guac_terminal_printf(guac_terminal* terminal, const char* format, ...) { @@ -595,7 +597,7 @@ int guac_terminal_printf(guac_terminal* terminal, const char* format, ...) { return written; /* Write to STDOUT */ - return guac_terminal_write_stdout(terminal, buffer, written); + return guac_terminal_write(terminal, buffer, written); } @@ -711,6 +713,7 @@ void guac_terminal_commit_cursor(guac_terminal* term) { int guac_terminal_write(guac_terminal* term, const char* c, int size) { + guac_terminal_lock(term); while (size > 0) { /* Read and advance to next character */ @@ -725,7 +728,9 @@ int guac_terminal_write(guac_terminal* term, const char* c, int size) { term->char_handler(term, current); } + guac_terminal_unlock(term); + guac_terminal_notify(term); return 0; } diff --git a/src/terminal/terminal.h b/src/terminal/terminal.h index 7598fcef..62312b04 100644 --- a/src/terminal/terminal.h +++ b/src/terminal/terminal.h @@ -159,13 +159,24 @@ struct guac_terminal { pthread_mutex_t lock; /** - * Pipe which should be written to (and read from) to provide output to - * this terminal. Another thread should read from this pipe when writing - * data to the terminal. It would make sense for the terminal to provide - * this thread, but for simplicity, that logic is left to the guac - * message handler (to give the message handler something to block with). + * The mutex associated with the modified condition and flag, locked + * whenever a thread is waiting on the modified condition, the modified + * condition is being signalled, or the modified flag is being changed. */ - int stdout_pipe_fd[2]; + pthread_mutex_t modified_lock; + + /** + * Flag set whenever an operation has affected the terminal in a way that + * will require a frame flush. When this flag is set, the modified_cond + * condition will be signalled. The modified_lock will always be + * acquired before this flag is altered. + */ + int modified; + + /** + * Condition which is signalled when the modified flag has been set + */ + pthread_cond_t modified_cond; /** * Pipe which will be the source of user input. When a terminal code @@ -473,24 +484,14 @@ int guac_terminal_render_frame(guac_terminal* terminal); */ int guac_terminal_read_stdin(guac_terminal* terminal, char* c, int size); -/** - * Writes to this terminal's STDOUT. This function may block until space - * is freed in the output buffer by guac_terminal_render_frame(). - */ -int guac_terminal_write_stdout(guac_terminal* terminal, const char* c, int size); - /** * Notifies the terminal that an event has occurred and the terminal should * flush itself when reasonable. * * @param terminal * The terminal to notify. - * - * @return - * Zero if notification succeeded, non-zero if an error occurred while - * notifying the terminal. */ -int guac_terminal_notify(guac_terminal* terminal); +void guac_terminal_notify(guac_terminal* terminal); /** * Reads a single line from this terminal's STDIN, storing the result in a