GUACAMOLE-86: Merge removal of terminal emulator's STDOUT pipe.

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;
 

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;

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         \

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;
-
-}
-

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
 

src/terminal/packet.c

@@ -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 <string.h>
-
-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;
-
-}
-

src/terminal/packet.h

@@ -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
-

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 <errno.h>
-#include <poll.h>
 #include <pthread.h>
 #include <stdarg.h>
 #include <stdio.h>
@@ -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
+ * @param ts
- *    The file descriptor to wait on.
+ *     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
+ *    Non-zero if the terminal has been modified, zero if the timeout has
- *    data becoming available, or negative if an error occurred.
+ *    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 */
+    int retval = 1;
-    struct pollfd fds[] = {{
-        .fd      = fd,
-        .events  = POLLIN,
-        .revents = 0,
-    }};
 
-    /* Wait for data */
+    pthread_mutex_t* mod_lock = &(terminal->modified_lock);
-    return poll(fds, 1, msec_timeout);
+    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);
+    wait_result = guac_terminal_wait(terminal, 1000);
-    if (wait_result > 0) {
+    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();
+            guac_timestamp frame_end = guac_timestamp_current();
-            frame_remaining = frame_start + GUAC_TERMINAL_FRAME_DURATION
+            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,
+void guac_terminal_notify(guac_terminal* terminal) {
-        int size) {
 
-    /* Write maximally-sized packets until only one packet remains */
+    pthread_mutex_t* mod_lock = &(terminal->modified_lock);
-    while (size > GUAC_TERMINAL_PACKET_SIZE) {
+    pthread_cond_t* mod_cond = &(terminal->modified_cond);
 
-        /* Write maximally-sized packet */
+    pthread_mutex_lock(mod_lock);
-        if (guac_terminal_packet_write(terminal->stdout_pipe_fd[1], c,
-                GUAC_TERMINAL_PACKET_SIZE) < 0)
-            return -1;
 
-        /* Advance to next packet */
+    /* Signal modification */
-        c    += GUAC_TERMINAL_PACKET_SIZE;
+    terminal->modified = 1;
-        size -= GUAC_TERMINAL_PACKET_SIZE;
+    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;
 
 }

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
+     * The mutex associated with the modified condition and flag, locked
-     * this terminal. Another thread should read from this pipe when writing
+     * whenever a thread is waiting on the modified condition, the modified
-     * data to the terminal. It would make sense for the terminal to provide
+     * condition is being signalled, or the modified flag is being changed.
-     * this thread, but for simplicity, that logic is left to the guac
-     * message handler (to give the message handler something to block with).
      */
-    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