mike-jumper commented on code in PR #358:
URL: https://github.com/apache/guacamole-server/pull/358#discussion_r1016118670
##########
src/libguac/guacamole/protocol-types.h:
##########
@@ -306,9 +306,40 @@ typedef enum guac_protocol_version {
* allowing connections in guacd to request information from the client and
* await a response.
*/
- GUAC_PROTOCOL_VERSION_1_3_0 = 0x010300
+ GUAC_PROTOCOL_VERSION_1_3_0 = 0x010300,
+
+ /**
+ * Protocol version 1.5.0, which supports the "msg" instruction, allowing
+ * messages to be sent to the client, and adds support for the "username"
+ * handshake instruction.
+ */
+ GUAC_PROTOCOL_VERSION_1_5_0 = 0x010500
} guac_protocol_version;
+/**
+ * A type that represents codes for human-readable messages sent by the "msg"
+ * instruction to the Client, that will be displayed in the client's browser.
+ * The codes will be interpreted by the client into translatable messages, and
+ * make take arguments, as noted below.
+ */
+typedef enum guac_msg_client {
+
+ /**
+ * A message that notifies the owner of a connection that another user has
+ * joined their connection. There should be a single argument, the username
+ * of the user who has joined.
+ */
+ GUAC_MSG_CLIENT_JOINED = 0x0001,
+
+ /**
+ * A message that notifies the owner of a connection that another user has
+ * left their connection. There should be a single argument provided, the
+ * username of the user who has left.
+ */
+ GUAC_MSG_CLIENT_LEFT = 0x0002
+
+} guac_msg_client;
Review Comment:
Keeping with the naming of other `enum`s defined here, I think this should
be `guac_message_type` rather than `guac_msg_client`.
##########
src/libguac/client.c:
##########
@@ -701,6 +739,108 @@ int guac_client_owner_supports_required(guac_client*
client) {
}
+/**
+ * A callback function that is invokved by guac_client_owner_notify_join() to
+ * notify the owner of a connection that another user has joined the
+ * connection, returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is joining the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_join_callback(guac_user* user, void*
data) {
+
+ const guac_user* joiner = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* owner = "owner";
+ if (user->info.username != NULL)
+ owner = (char *) user->info.username;
+
+ char* joinName = "anonymous";
+ if (joiner->info.username != NULL)
+ joinName = (char *) joiner->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s joining.",
owner, joinName);
+
+ /* Send required parameters to owner. */
+ const char* args[] = { (const char*)joinName, NULL };
+ return (void*) ((intptr_t) guac_protocol_send_msg(user->socket,
GUAC_MSG_CLIENT_JOINED, args));
+
+}
+
+int guac_client_owner_notify_join(guac_client* client, guac_user* joiner) {
+
+ /* Don't send msg instruction if client does not support it. */
+ if (!guac_client_owner_supports_msg(client))
+ return -1;
+
+ return (int) ((intptr_t) guac_client_for_owner(client,
guac_client_owner_notify_join_callback, joiner));
+
+}
+
+/**
+ * A callback function that is invokved by guac_client_owner_notify_leave() to
+ * notify the owner of a connection that another user has left the connection,
+ * returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is leaving the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_leave_callback(guac_user* user, void*
data) {
+
+ const guac_user* quitter = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* ownerName = "owner";
+ if (user->info.username != NULL)
+ ownerName = (char *) user->info.username;
+
+ char* quitterName = "anonymous";
+ if (quitter->info.username != NULL)
+ quitterName = (char *) quitter->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s leaving.",
ownerName, quitterName);
Review Comment:
Same here - `Notifying owner "owner" ...` would be more readable than
`Notifying owner owner ...`.
##########
src/libguac/guacamole/user.h:
##########
@@ -89,6 +89,12 @@ struct guac_user_info {
*/
int optimal_resolution;
+ /**
+ * The human-readable name of the Guacamole user. If the client does not
+ * provide a name then this will be NULL.
+ */
+ const char* username;
Review Comment:
With this being an arbitrary, human-readable value, with no guarantees of
uniqueness nor requirements for uniqueness, I think:
* `name` would be a better choice than `username`, which otherwise implies
uniqueness.
* We should note that no requirement is imposed on the nature of this value,
including whether it is unique, except that it will be the value supplied
during the handshake.
##########
src/libguac/guacamole/protocol-types.h:
##########
@@ -306,9 +306,40 @@ typedef enum guac_protocol_version {
* allowing connections in guacd to request information from the client and
* await a response.
*/
- GUAC_PROTOCOL_VERSION_1_3_0 = 0x010300
+ GUAC_PROTOCOL_VERSION_1_3_0 = 0x010300,
+
+ /**
+ * Protocol version 1.5.0, which supports the "msg" instruction, allowing
+ * messages to be sent to the client, and adds support for the "username"
+ * handshake instruction.
+ */
+ GUAC_PROTOCOL_VERSION_1_5_0 = 0x010500
} guac_protocol_version;
+/**
+ * A type that represents codes for human-readable messages sent by the "msg"
+ * instruction to the Client, that will be displayed in the client's browser.
+ * The codes will be interpreted by the client into translatable messages, and
+ * make take arguments, as noted below.
+ */
+typedef enum guac_msg_client {
+
+ /**
+ * A message that notifies the owner of a connection that another user has
+ * joined their connection. There should be a single argument, the username
+ * of the user who has joined.
+ */
+ GUAC_MSG_CLIENT_JOINED = 0x0001,
+
+ /**
+ * A message that notifies the owner of a connection that another user has
+ * left their connection. There should be a single argument provided, the
+ * username of the user who has left.
+ */
+ GUAC_MSG_CLIENT_LEFT = 0x0002
Review Comment:
Similar to the `enum`, I'd recommend renaming these to
`GUAC_MESSAGE_USER_JOINED` and `GUAC_MESSSAGE_USER_LEFT`.
##########
src/libguac/guacamole/user.h:
##########
@@ -89,6 +89,12 @@ struct guac_user_info {
*/
int optimal_resolution;
+ /**
+ * The human-readable name of the Guacamole user. If the client does not
+ * provide a name then this will be NULL.
+ */
+ const char* username;
Review Comment:
New structure members should be added to the end of the `struct` to preserve
binary compatibility of the offsets of existing structure members. If at the
end, existing code would need only be rebuilt if it allocates `guac_user_info`
itself.
##########
src/libguac/client.c:
##########
@@ -701,6 +739,108 @@ int guac_client_owner_supports_required(guac_client*
client) {
}
+/**
+ * A callback function that is invokved by guac_client_owner_notify_join() to
+ * notify the owner of a connection that another user has joined the
+ * connection, returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is joining the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_join_callback(guac_user* user, void*
data) {
+
+ const guac_user* joiner = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* owner = "owner";
+ if (user->info.username != NULL)
+ owner = (char *) user->info.username;
+
+ char* joinName = "anonymous";
+ if (joiner->info.username != NULL)
+ joinName = (char *) joiner->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s joining.",
owner, joinName);
+
+ /* Send required parameters to owner. */
+ const char* args[] = { (const char*)joinName, NULL };
+ return (void*) ((intptr_t) guac_protocol_send_msg(user->socket,
GUAC_MSG_CLIENT_JOINED, args));
+
+}
+
+int guac_client_owner_notify_join(guac_client* client, guac_user* joiner) {
+
+ /* Don't send msg instruction if client does not support it. */
+ if (!guac_client_owner_supports_msg(client))
+ return -1;
Review Comment:
Just as we're logging when the user has been notified (at the debug level),
I'd recommend also logging when the user _hasn't_ been notified due to lack of
support.
##########
src/libguac/client.c:
##########
@@ -701,6 +739,108 @@ int guac_client_owner_supports_required(guac_client*
client) {
}
+/**
+ * A callback function that is invokved by guac_client_owner_notify_join() to
+ * notify the owner of a connection that another user has joined the
+ * connection, returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is joining the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_join_callback(guac_user* user, void*
data) {
+
+ const guac_user* joiner = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* owner = "owner";
+ if (user->info.username != NULL)
+ owner = (char *) user->info.username;
+
+ char* joinName = "anonymous";
+ if (joiner->info.username != NULL)
+ joinName = (char *) joiner->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s joining.",
owner, joinName);
+
+ /* Send required parameters to owner. */
+ const char* args[] = { (const char*)joinName, NULL };
+ return (void*) ((intptr_t) guac_protocol_send_msg(user->socket,
GUAC_MSG_CLIENT_JOINED, args));
+
+}
+
+int guac_client_owner_notify_join(guac_client* client, guac_user* joiner) {
+
+ /* Don't send msg instruction if client does not support it. */
+ if (!guac_client_owner_supports_msg(client))
+ return -1;
+
+ return (int) ((intptr_t) guac_client_for_owner(client,
guac_client_owner_notify_join_callback, joiner));
+
+}
+
+/**
+ * A callback function that is invokved by guac_client_owner_notify_leave() to
+ * notify the owner of a connection that another user has left the connection,
+ * returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is leaving the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_leave_callback(guac_user* user, void*
data) {
+
+ const guac_user* quitter = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* ownerName = "owner";
+ if (user->info.username != NULL)
+ ownerName = (char *) user->info.username;
+
+ char* quitterName = "anonymous";
+ if (quitter->info.username != NULL)
+ quitterName = (char *) quitter->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s leaving.",
ownerName, quitterName);
+
+ /* Send required parameters to owner. */
+ const char* args[] = { (const char*)quitterName, NULL };
+ return (void*) ((intptr_t) guac_protocol_send_msg(user->socket,
GUAC_MSG_CLIENT_LEFT, args));
+
+}
+
+int guac_client_owner_notify_leave(guac_client* client, guac_user* quitter) {
+
+ /* Don't send msg instruction if client does not support it. */
+ if (!guac_client_owner_supports_msg(client))
+ return -1;
Review Comment:
Same here - recommend additionally logging when we are not able to notify.
##########
src/libguac/client.c:
##########
@@ -701,6 +739,108 @@ int guac_client_owner_supports_required(guac_client*
client) {
}
+/**
+ * A callback function that is invokved by guac_client_owner_notify_join() to
+ * notify the owner of a connection that another user has joined the
+ * connection, returning zero if the message is sent successfully, or non-zero
+ * if an error occurs.
+ *
+ * @param user
+ * The user to send the notification to, which will be the owner of the
+ * connection.
+ *
+ * @param data
+ * The data provided to the callback, which is the user that is joining the
+ * connection.
+ *
+ * @return
+ * Zero if the message is sent successfully to the owner, otherwise
+ * non-zero, cast as a void*.
+ */
+static void* guac_client_owner_notify_join_callback(guac_user* user, void*
data) {
+
+ const guac_user* joiner = (const guac_user *) data;
+
+ if (user == NULL)
+ return (void*) ((intptr_t) -1);
+
+ char* owner = "owner";
+ if (user->info.username != NULL)
+ owner = (char *) user->info.username;
+
+ char* joinName = "anonymous";
+ if (joiner->info.username != NULL)
+ joinName = (char *) joiner->info.username;
+
+ guac_user_log(user, GUAC_LOG_DEBUG, "Notifying owner %s of %s joining.",
owner, joinName);
Review Comment:
If `user->info.username` is `NULL` here, the log message will be "Notifying
owner owner ...". I'd recommend instead enclosing the usernames in quotes.
--
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]
