mike-jumper commented on code in PR #455:
URL: https://github.com/apache/guacamole-server/pull/455#discussion_r1307919654
##########
src/guacd/proc.c:
##########
@@ -36,6 +36,7 @@
#include <pthread.h>
#include <signal.h>
#include <stdlib.h>
+#include <stdio.h>
Review Comment:
And here? No longer needed?
##########
src/common/list.c:
##########
@@ -34,8 +34,32 @@ guac_common_list* guac_common_list_alloc() {
}
-void guac_common_list_free(guac_common_list* list) {
+void guac_common_list_free(
+ guac_common_list* list,
+ guac_common_list_element_free_handler* free_element_handler) {
+
+ pthread_mutex_lock(&(list->_lock));
+
+ /* Free every element of the list */
+ guac_common_list_element* element = list->head;
+ while(element != NULL) {
+
+ guac_common_list_element* next = element->next;
+
+ if (free_element_handler != NULL)
+ free_element_handler(element->data);
+
+ free(element);
+ element = next;
+ }
+
+ /* Destroy the mutex controlling access to the list */
+ pthread_mutex_unlock(&(list->_lock));
Review Comment:
There's no need to lock a mutex that's about to be destroyed. If there is
contention surrounding an instance of `guac_common_list` at the time it is
being freed, that'll be bad news regardless of whether the mutex is held.
The documentation for `_lock` further states:
> ... Possession of the lock is not enforced outside the
`guac_common_list_lock()` function.
So this goes against that documented behavior.
##########
src/guacd/daemon.c:
##########
@@ -457,6 +501,10 @@ int main(int argc, char* argv[]) {
"Child processes may pile up in the process table.");
}
+ /* Clean up and exist if SIGINT or SIGTERM signals are caught */
Review Comment:
exit*, I presume.
##########
src/libguac/guacamole/client.h:
##########
@@ -40,22 +41,32 @@
#include <cairo/cairo.h>
-#include <pthread.h>
+#include <signal.h>
#include <stdarg.h>
+#include <stdatomic.h>
Review Comment:
Are `signal.h` and `stdatomic.h` used here?
##########
src/guacd/daemon.c:
##########
@@ -457,6 +501,10 @@ int main(int argc, char* argv[]) {
"Child processes may pile up in the process table.");
}
+ /* Clean up and exist if SIGINT or SIGTERM signals are caught */
+ signal(SIGINT, signal_stop_handler);
+ signal(SIGTERM, signal_stop_handler);
Review Comment:
The [documentation for
`signal()`](https://man7.org/linux/man-pages/man2/signal.2.html) warns against
using the function at all due to portability issues:
> **WARNING:** the behavior of `signal()` varies across UNIX versions, and
has also varied historically across different versions of Linux. **Avoid its
use**: use
[sigaction(2)](https://man7.org/linux/man-pages/man2/sigaction.2.html) instead.
See _Portability_ below.
Perhaps we should use `sigaction()` instead, as recommended?
##########
src/guacd/proc.c:
##########
@@ -333,6 +354,10 @@ static void guacd_exec_proc(guacd_proc* proc, const char*
protocol) {
/* Enable keep alive on the broadcast socket */
guac_socket_require_keep_alive(client->socket);
+ guacd_proc_self = proc;
+ signal(SIGINT, signal_stop_handler);
+ signal(SIGTERM, signal_stop_handler);
Review Comment:
Same here - perhaps we shouldn't use `signal()`, as strongly recommended by
the docs?
##########
src/terminal/terminal/scrollbar.h:
##########
@@ -260,6 +260,10 @@ void
guac_terminal_scrollbar_flush(guac_terminal_scrollbar* scrollbar);
* completely recreate and redraw the scrollbar rendering over the given
* socket.
*
+ * @deprecated guac_terminal_scrollbar_dup_batch() allows synchronizing
+ * multiple joining users at once using a broadcast socket, and should be
+ * used instead of this function
Review Comment:
Is there any need to maintain and deprecate this variant? Checking
`Makefile.am`, this header looks to be part of the private API.
##########
src/guacd/proc-map.c:
##########
@@ -27,6 +27,26 @@
#include <stdlib.h>
#include <string.h>
+#include <stdio.h>
Review Comment:
No longer needed here, either?
##########
src/libguac/guacamole/client-fntypes.h:
##########
@@ -30,7 +30,9 @@
#include "client-types.h"
#include "object-types.h"
#include "protocol-types.h"
+#include "socket.h"
#include "stream-types.h"
+#include "user-fntypes.h"
Review Comment:
Why are `socket.h` and `user-fntypes.h` added here?
##########
src/protocols/kubernetes/argv.h:
##########
@@ -70,5 +71,23 @@ guac_argv_callback guac_kubernetes_argv_callback;
*/
void* guac_kubernetes_send_current_argv(guac_user* user, void* data);
+/**
+ * Sends the current values of all non-sensitive parameters which may be set
+ * while the connection is running to the all users associated with the
+ * provided socket. Note that the users receiving these values will not
+ * necessarily be able to set new values themselves if their connection is
+ * read-only.
+ *
+ * @param client
+ * The client associated with the users who should receive the values of
+ * all non-sensitive parameters which may be set while the connection is
+ * running.
+ *
+ * @return
+ * Always NULL.
+ */
+void* guac_kubernetes_send_current_argv_batch(
Review Comment:
Is the old `guac_kubernetes_send_current_argv()` still used anywhere? (Same
for other protocols)
##########
src/libguac/client.c:
##########
@@ -277,36 +435,135 @@ void guac_client_abort(guac_client* client,
guac_protocol_status status,
}
+/**
+ * Add the provided user to the list of pending users who have yet to have
+ * their connection state synchronized after joining, for the connection
+ * associated with the given guac client.
+ *
+ * @param client
+ * The client associated with the connection for which the provided user
+ * is pending a connection state synchronization after joining.
+ *
+ * @param user
+ * The user to add to the pending list.
+ */
+static void guac_client_add_pending_user(
+ guac_client* client, guac_user* user) {
+
+ /* Acquire the lock for modifying the list of pending users */
+ guac_acquire_write_lock(&(client->__pending_users_lock));
+
+ user->__prev = NULL;
+ user->__next = client->__pending_users;
+
+ if (client->__pending_users != NULL)
+ client->__pending_users->__prev = user;
+
+ client->__pending_users = user;
+
+ /* Increment the user count */
+ client->connected_users++;
+
+ /* Release the lock */
+ guac_release_lock(&(client->__pending_users_lock));
+
+}
+
+/**
+ * Periodically promote pending users to full users. Returns zero if the timer
+ * is already running, or successfully created, or a non-zero value if the
+ * timer could not be created and started.
+ *
+ * @param client
+ * The guac client for which the new timer should be started, if not
+ * already running.
+ *
+ * @return
+ * Zero if the timer was successfully created and started, or a negative
+ * value otherwise.
+ */
+static int guac_client_start_pending_users_timer(guac_client* client) {
+
+ pthread_mutex_lock(&(client->__pending_users_timer_mutex));
+
+ /* Return success if the timer is already created and running */
+ if (client->__pending_users_timer_state
+ != GUAC_CLIENT_PENDING_TIMER_UNREGISTERED) {
+ pthread_mutex_unlock(&(client->__pending_users_timer_mutex));
+ return 0;
+ }
+
+ /* Configure the timer to synchronize and clear the pending users */
+ struct sigevent signal_config = { 0 };
+ signal_config.sigev_notify = SIGEV_THREAD;
+ signal_config.sigev_notify_function = guac_client_promote_pending_users;
+ signal_config.sigev_value.sival_ptr = client;
+
+ /* Create a timer to synchronize any pending users periodically */
+ if (timer_create(
+ CLOCK_MONOTONIC,
+ &signal_config,
+ &(client->__pending_users_timer))) {
+ pthread_mutex_unlock(&(client->__pending_users_timer_mutex));
+ return 1;
+ }
+
+ /* Configure the pending users timer to run on the defined interval */
+ struct itimerspec time_config = { 0 };
+ time_config.it_interval.tv_nsec =
GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL;
+ time_config.it_value.tv_nsec = GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL;
+
+ /* Start the timer */
+ if (timer_settime(
+ client->__pending_users_timer, 0, &time_config, NULL) < 0) {
+ timer_delete(client->__pending_users_timer);
+ pthread_mutex_unlock(&(client->__pending_users_timer_mutex));
+ return 1;
+ }
+
+ /* Mark the timer as registered but not yet running */
+ client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_REGISTERED;
+
+ pthread_mutex_unlock(&(client->__pending_users_timer_mutex));
+ return 0;
+
+}
+
int guac_client_add_user(guac_client* client, guac_user* user, int argc,
char** argv) {
+ /* Create and start the timer if it hasn't already been initialized */
+ if (guac_client_start_pending_users_timer(client)) {
+
+ /**
+ *
+ * If the timer could not be created, do not add the user - they cannot
+ * be synchronized without the timer.
+ */
+ guac_client_log(client, GUAC_LOG_ERROR,
+ "Could not start pending user timer: %s.", strerror(errno));
+ return 1;
+ }
+
int retval = 0;
/* Call handler, if defined */
if (client->join_handler)
retval = client->join_handler(user, argc, argv);
- pthread_rwlock_wrlock(&(client->__users_lock));
-
- /* Add to list if join was successful */
if (retval == 0) {
- user->__prev = NULL;
- user->__next = client->__users;
-
- if (client->__users != NULL)
- client->__users->__prev = user;
-
- client->__users = user;
- client->connected_users++;
+ /*
+ * Add the user to the list of pending users, to have their connection
+ * state synchronized asynchronously.
+ */
Review Comment:
The `*` characters within this comment appear misaligned.
##########
src/terminal/terminal/display.h:
##########
@@ -340,6 +340,25 @@ void guac_terminal_display_flush(guac_terminal_display*
display);
void guac_terminal_display_dup(guac_terminal_display* display, guac_user* user,
guac_socket* socket);
+/**
+ * Initializes and syncs the current terminal display state for all joining
+ * users associated with the provided socket, sending the necessary
instructions
+ * to completely recreate and redraw the terminal rendering over the given
+ * socket.
+ *
+ * @param display
+ * The terminal display to sync to the users associated with the provided
+ * socket.
+ *
+ * @param client
+ * The client whose users are joining.
+ *
+ * @param socket
+ * The socket over which any necessary instructions should be sent.
+ */
+void guac_terminal_display_dup_batch(
+ guac_terminal_display* display, guac_client* client, guac_socket*
socket);
Review Comment:
Same here - I think there's no need to maintain the old version of this
function, unless it has some utility internally within `guac_terminal`. It's
not part of the public API.
##########
src/libguac/guacamole/client.h:
##########
@@ -40,22 +41,32 @@
#include <cairo/cairo.h>
-#include <pthread.h>
Review Comment:
I think `pthread.h` is still needed - I see at least one `pthread_mutex_t`
structure member below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
