mike-jumper commented on code in PR #455:
URL: https://github.com/apache/guacamole-server/pull/455#discussion_r1303410751
##########
src/terminal/terminal/terminal.h:
##########
@@ -632,6 +635,22 @@ int guac_terminal_sendf(guac_terminal* term, const char*
format, ...);
void guac_terminal_dup(guac_terminal* term, guac_user* user,
guac_socket* socket);
+/**
+ * Synchronizes the current display state for all users assocaited with a
Review Comment:
associated*
##########
src/terminal/terminal/terminal.h:
##########
@@ -619,6 +619,9 @@ int guac_terminal_sendf(guac_terminal* term, const char*
format, ...);
* connection. All instructions necessary to replicate state are sent over the
* given socket.
*
+ * @deprecated The guac_terminal_sync_pending_users method should be used for
Review Comment:
The function is defined as `guac_terminal_sync_users()`.
##########
src/libguac/local-lock.c:
##########
@@ -0,0 +1,218 @@
+/*
+ * 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 <pthread.h>
+#include <stdint.h>
+#include "local-lock.h"
+
+/**
+ * The value indicating that the current thread holds neither the read or write
+ * locks.
+ */
+#define GUAC_LOCAL_LOCK_NO_LOCK 0
+
+/**
+ * The value indicating that the current thread holds the read lock.
+ */
+#define GUAC_LOCAL_LOCK_READ_LOCK 1
+
+/**
+ * The value indicating that the current thread holds the write lock.
+ */
+#define GUAC_LOCAL_LOCK_WRITE_LOCK 2
+
+void guac_init_local_lock(guac_local_lock* lock) {
+
+ /* Configure to allow sharing this lock with child processes */
+ pthread_rwlockattr_t lock_attributes;
+ pthread_rwlockattr_init(&lock_attributes);
+ pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED);
+
+ /* Initialize the rwlock */
+ pthread_rwlock_init(&(lock->lock), &lock_attributes);
+
+ /* Initialize the flags to 0, as threads won't have acquired it yet */
+ pthread_key_create(&(lock->key), (void *) 0);
+
+}
+
+void guac_destroy_local_lock(guac_local_lock* lock) {
+
+ /* Destroy the rwlock */
+ pthread_rwlock_destroy(&(lock->lock));
+
+ /* Destroy the thread-local key */
+ pthread_key_delete(lock->key);
+
+}
+
+/**
+ * Clean up and destroy the provided guac local lock.
+ *
+ * @param lock
+ * The guac local lock to be destroyed.
+ */
+void guac_destroy_local_lock(guac_local_lock* lock);
+
+/**
+ * Extract and return the flag indicating which lock is held, if any, from the
+ * provided key value. The flag is always stored in the least-significant
+ * nibble of the value.
+ *
+ * @param value
+ * The key value containing the flag.
+ *
+ * @return
+ * The flag indicating which lock is held, if any.
+ */
+static uintptr_t get_lock_flag(uintptr_t value) {
+ return value & 0xF;
+}
+
+/**
+ * Extract and return the lock count from the provided key. This returned value
+ * is the difference between the number of lock and unlock requests made by the
+ * current thread. This count is always stored in the remaining value after the
+ * least-significant nibble where the flag is stored.
+ *
+ * @param value
+ * The key value containing the count.
+ *
+ * @return
+ * The difference between the number of lock and unlock requests made by
+ * the current thread.
+ */
+static uintptr_t get_lock_count(uintptr_t value) {
+ return value >> 4;
+}
+
+/**
+ * Given a flag indicating if and how the current thread controls a lock, and
+ * a count of the depth of lock requests, return a value containing the flag
+ * in the least-significant nibble, and the count in the rest.
+ *
+ * @param flag
+ * A flag indiciating which lock, if any, is held by the current thread.
+ *
+ * @param count
+ * The depth of the lock attempt by the current thread, i.e. the number of
+ * lock requests minus unlock requests.
+ *
+ * @return
+ * A value containing the flag in the least-significant nibble, and the
+ * count in the rest, cast to a void* for thread-local storage.
+ */
+static void* get_value_from_flag_and_count(
+ uintptr_t flag, uintptr_t count) {
+ return (void*) ((flag & 0xF) | count << 4);
+}
+
+void guac_acquire_write_lock(guac_local_lock* local_lock) {
+
+ uintptr_t key_value = (uintptr_t) pthread_getspecific(local_lock->key);
+ uintptr_t flag = get_lock_flag(key_value);
+ uintptr_t count = get_lock_count(key_value);
+
+ /* If the current thread already holds the write lock, increment the count
*/
+ if (flag == GUAC_LOCAL_LOCK_WRITE_LOCK) {
+ pthread_setspecific(local_lock->key, get_value_from_flag_and_count(
+ flag, count + 1));
+
+ /* This thread already has the lock */
+ return;
+ }
+
+ /*
+ * The read lock must be released before the write lock can be acquired.
+ * This is a little odd because it may mean that a function further down
+ * the stack may have requested a read lock, which will get upgraded to a
+ * write lock by another function without the caller knowing about it. This
+ * shouldn't cause any issues, however.
+ */
+ if (key_value == GUAC_LOCAL_LOCK_READ_LOCK)
+ pthread_rwlock_unlock(&(local_lock->lock));
+
+ /* Acquire the write lock */
+ pthread_rwlock_wrlock(&(local_lock->lock));
+
+ /* Mark that the current thread has the lock, and increment the count */
+ pthread_setspecific(local_lock->key, get_value_from_flag_and_count(
+ GUAC_LOCAL_LOCK_WRITE_LOCK, count + 1));
Review Comment:
In the unlikely event that `count + 1` approaches or overflows the bounds of
a `uintptr_t`, what happens?
##########
src/common/common/cursor.h:
##########
@@ -150,6 +150,10 @@ void guac_common_cursor_free(guac_common_cursor* cursor);
* the current cursor image. The resulting cursor on the remote display will
* be visible.
*
+ * @deprecated guac_common_cursor_dup_batch should be used instead of this
+ * user-specific method. Unlike this function, it supports sending the cursor
+ * to multiple users with a broadcast socket.
Review Comment:
There's no need to do this for functions that are purely internal and not
part of a public API. Same for other parts of guacamole-server's internal
convenience libraries.
##########
src/libguac/local-lock.h:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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_LOCAL_LOCK_H
+#define __GUAC_LOCAL_LOCK_H
+
+#include <pthread.h>
+
+/**
+ * This file implements reentrant read-write locks using thread-local storage
+ * to keep track of how locks are held and released by the current thread,
+ * since the pthread locks do not support reentrant behavior.
+ *
+ * A thread will attempt to acquire the requested lock on the first acquire
+ * function call, and will release it once the number of unlock requests
+ * matches the number of lock requests. Therefore, it is safe to aquire a lock
+ * and then call a function that also acquires the same lock, provided that
+ * the caller and the callee request to unlock the lock when done with it.
+ *
+ * Any lock that's locked using one of the functions defined in this file
+ * must _only_ be unlocked using the unlock function defined here to avoid
+ * unexpected behavior.
+ */
+
+/**
+ * A structure packaging together a pthread rwlock along with a key to a
+ * thread-local property to keep track of the current status of the lock,
+ * allowing the functions defined in this header to provide reentrant behavior.
+ * Note that both the lock and key must be initialized before being provided
+ * to any of these functions.
+ */
+typedef struct guac_local_lock {
Review Comment:
What's the intent behind the naming `guac_local_lock`? It's unclear to me
what "local" is intended to highlight for a user of this lock implementation.
If the naming is meant to highlight the fact that the lock leverages
thread-local storage, I would suggest instead naming things to highlight the
fact that the lock is a reentrant implementation of a read/write lock, leaving
the use of thread-local storage as an internal detail.
##########
src/libguac/guacamole/client.h:
##########
@@ -162,14 +173,65 @@ struct guac_client {
* Lock which is acquired when the users list is being manipulated, or when
* the users list is being iterated.
*/
- pthread_rwlock_t __users_lock;
+ guac_local_lock __users_lock;
/**
* The first user within the list of all connected users, or NULL if no
* users are currently connected.
*/
guac_user* __users;
+ /**
+ * Lock which is acquired when the pending users list is being manipulated,
+ * or when the pending users list is being iterated.
+ */
+ guac_local_lock __pending_users_lock;
+
+ /**
+ * A timer that will periodically synchronize the list of pending users,
+ * emptying the list once synchronization is complete. Only for internal
+ * use within the client. This will be NULL until the first user joins
+ * the connection, as it is lazily instantiated at that time.
+ */
+ timer_t __pending_users_timer;
+
+ /**
+ * Non-zero if the pending users timer is configured and running, or zero
+ * otherwise.
+ */
+ int __pending_users_timer_running;
+
+ /**
+ * A mutex that must be acquired before modifying the pending users timer.
+ */
+ pthread_mutex_t __pending_users_timer_mutex;
+
+ /**
+ * Notification / signaling configuration for the pending users timer.
+ * Only for internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct sigevent __pending_users_timer_signal_config;
+
+ /**
+ * Specification for the timing of the pending users timer. Only for
+ * internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct itimerspec __pending_users_time_spec;
+
+ /**
+ * A flag that indicates whether the pending users timer event thread is
+ * currently running.
+ */
+ volatile atomic_flag __pending_timer_event_active;
Review Comment:
Would it make sense to combine this with `__pending_users_timer_running`
such that there is only one flag+mutex indicating the current state of the
timer (unregistered, registered, triggered)?
##########
src/terminal/terminal/terminal.h:
##########
@@ -632,6 +635,22 @@ int guac_terminal_sendf(guac_terminal* term, const char*
format, ...);
void guac_terminal_dup(guac_terminal* term, guac_user* user,
guac_socket* socket);
+/**
+ * Synchronizes the current display state for all users assocaited with a
+ * provided socket and client.
Review Comment:
IMHO, it's unclear what synchronizing the current display state means in
this context, as well as whether absolutely _all_ users are sync'd / how the
subset of users to be sync'd is determined. I'd recommend a variant of the
wording already used by `guac_terminal_dup()`:
> Replicates the current display state to one or more users that are joining
the connection. All instructions necessary to replicate state are sent over the
given socket. The set of users receiving these instructions is determined
solely by the socket chosen.
##########
src/libguac/guacamole/client-fntypes.h:
##########
@@ -79,5 +91,22 @@ typedef void guac_client_log_handler(guac_client* client,
*/
typedef int guac_client_init_handler(guac_client* client);
+/**
+ * A function that will broadcast arbitrary data to a subset of users for
+ * the provided client, using the provided user callback for any user-specific
+ * operations.
+ *
+ * @param client
+ * The guac_client associated with the users to broadcast to.
+ *
+ * @param callback
+ * A callback that should be invoked with each broadcasted user.
+ *
+ * @param data
+ * Arbitrary data that may be used to broadcast to the subset of users.
+ */
+typedef void guac_client_broadcast_handler(
+ guac_client* client, guac_user_callback* callback, void* data);
Review Comment:
This particular function type ...
* ... is specific to `guac_socket`, and should instead be named
`guac_socket_broadcast_handler` within files specific to `guac_socket`
* ... is internal to the broadcast socket implementation and need not be
exposed publicly. It should instead be declared strictly internally and omitted
from the public API.
##########
src/libguac/guacamole/client.h:
##########
@@ -162,14 +173,65 @@ struct guac_client {
* Lock which is acquired when the users list is being manipulated, or when
* the users list is being iterated.
*/
- pthread_rwlock_t __users_lock;
+ guac_local_lock __users_lock;
/**
* The first user within the list of all connected users, or NULL if no
* users are currently connected.
*/
guac_user* __users;
+ /**
+ * Lock which is acquired when the pending users list is being manipulated,
+ * or when the pending users list is being iterated.
+ */
+ guac_local_lock __pending_users_lock;
+
+ /**
+ * A timer that will periodically synchronize the list of pending users,
+ * emptying the list once synchronization is complete. Only for internal
+ * use within the client. This will be NULL until the first user joins
+ * the connection, as it is lazily instantiated at that time.
+ */
+ timer_t __pending_users_timer;
+
+ /**
+ * Non-zero if the pending users timer is configured and running, or zero
+ * otherwise.
+ */
+ int __pending_users_timer_running;
+
+ /**
+ * A mutex that must be acquired before modifying the pending users timer.
+ */
+ pthread_mutex_t __pending_users_timer_mutex;
+
+ /**
+ * Notification / signaling configuration for the pending users timer.
+ * Only for internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct sigevent __pending_users_timer_signal_config;
+
+ /**
+ * Specification for the timing of the pending users timer. Only for
+ * internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct itimerspec __pending_users_time_spec;
Review Comment:
Do these need to be included within the `guac_client` structure? I see them
used only during initialization.
##########
src/libguac/guacamole/client.h:
##########
@@ -162,14 +173,65 @@ struct guac_client {
* Lock which is acquired when the users list is being manipulated, or when
* the users list is being iterated.
*/
- pthread_rwlock_t __users_lock;
+ guac_local_lock __users_lock;
/**
* The first user within the list of all connected users, or NULL if no
* users are currently connected.
*/
guac_user* __users;
+ /**
+ * Lock which is acquired when the pending users list is being manipulated,
+ * or when the pending users list is being iterated.
+ */
+ guac_local_lock __pending_users_lock;
+
+ /**
+ * A timer that will periodically synchronize the list of pending users,
+ * emptying the list once synchronization is complete. Only for internal
+ * use within the client. This will be NULL until the first user joins
+ * the connection, as it is lazily instantiated at that time.
+ */
+ timer_t __pending_users_timer;
+
+ /**
+ * Non-zero if the pending users timer is configured and running, or zero
+ * otherwise.
+ */
+ int __pending_users_timer_running;
+
+ /**
+ * A mutex that must be acquired before modifying the pending users timer.
+ */
+ pthread_mutex_t __pending_users_timer_mutex;
+
+ /**
+ * Notification / signaling configuration for the pending users timer.
+ * Only for internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct sigevent __pending_users_timer_signal_config;
+
+ /**
+ * Specification for the timing of the pending users timer. Only for
+ * internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct itimerspec __pending_users_time_spec;
+
+ /**
+ * A flag that indicates whether the pending users timer event thread is
+ * currently running.
Review Comment:
* We don't necessarily know that the timer is implemented with threads.
* This indicates whether the timer event callback is currently running, not
whether the timer itself is running, yes? The documentation for
`__pending_users_timer_running` uses the same terminology to refer to whether
the timer has been registered.
##########
src/libguac/guacamole/client-fntypes.h:
##########
@@ -48,6 +50,16 @@
*/
typedef int guac_client_free_handler(guac_client* client);
+/**
+ * Handler that will run before immediately before pending users are promoted
+ * to full users. The pending user socket should be used to communicate to the
+ * pending users.
+ *
+ * @param client
+ * The client whose handler was invoked.
+ */
+typedef void guac_join_pending_handler(guac_client* client);
Review Comment:
Should this function have error-handling semantics, as well, in line with
things like the join handler?
##########
src/libguac/guacamole/client.h:
##########
@@ -162,14 +173,65 @@ struct guac_client {
* Lock which is acquired when the users list is being manipulated, or when
* the users list is being iterated.
*/
- pthread_rwlock_t __users_lock;
+ guac_local_lock __users_lock;
/**
* The first user within the list of all connected users, or NULL if no
* users are currently connected.
*/
guac_user* __users;
+ /**
+ * Lock which is acquired when the pending users list is being manipulated,
+ * or when the pending users list is being iterated.
+ */
+ guac_local_lock __pending_users_lock;
+
+ /**
+ * A timer that will periodically synchronize the list of pending users,
+ * emptying the list once synchronization is complete. Only for internal
+ * use within the client. This will be NULL until the first user joins
+ * the connection, as it is lazily instantiated at that time.
+ */
+ timer_t __pending_users_timer;
+
+ /**
+ * Non-zero if the pending users timer is configured and running, or zero
+ * otherwise.
+ */
+ int __pending_users_timer_running;
+
+ /**
+ * A mutex that must be acquired before modifying the pending users timer.
+ */
+ pthread_mutex_t __pending_users_timer_mutex;
+
+ /**
+ * Notification / signaling configuration for the pending users timer.
+ * Only for internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct sigevent __pending_users_timer_signal_config;
+
+ /**
+ * Specification for the timing of the pending users timer. Only for
+ * internal use within the client. This will be NULL until the
+ * first user joins the connection.
+ */
+ struct itimerspec __pending_users_time_spec;
+
+ /**
+ * A flag that indicates whether the pending users timer event thread is
+ * currently running.
+ */
+ volatile atomic_flag __pending_timer_event_active;
Review Comment:
Which standard defines `atomic_flag`? The guacamole-server build currently
targets C99:
https://github.com/apache/guacamole-server/blob/80598ae857b4ccf696b5adff8c48a9ecbfe4a647/configure.ac#L37
##########
src/libguac/guacamole/client-fntypes.h:
##########
@@ -48,6 +50,16 @@
*/
typedef int guac_client_free_handler(guac_client* client);
+/**
+ * Handler that will run before immediately before pending users are promoted
+ * to full users. The pending user socket should be used to communicate to the
+ * pending users.
+ *
+ * @param client
+ * The client whose handler was invoked.
+ */
+typedef void guac_join_pending_handler(guac_client* client);
Review Comment:
This should be `guac_client_join_pending_handler`.
--
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]
