mike-jumper commented on a change in pull request #228:
URL: https://github.com/apache/guacamole-server/pull/228#discussion_r489702557



##########
File path: src/libguac/client.c
##########
@@ -633,6 +669,44 @@ static void* __webp_support_callback(guac_user* user, 
void* data) {
 }
 #endif
 
+/**
+ * A callback function which is invoked by 
guac_client_owner_supports_required()
+ * to determine if the owner of a client supports the "required" instruction,
+ * updating the flag to indicate support.
+ * 
+ * @param user
+ *     The guac_user that will be checked for "required" instruction support.
+ * 
+ * @param data
+ *     A pointer to an int containing the status for support of the "required"
+ *     instruction.  This will be 0 if the owner does not support this
+ *     instruction, or 1 if the owner does support it.
+ * 
+ * @return
+ *     Always NULL.
+ */
+static void* guac_owner_supports_required_callback(guac_user* user, void* 
data) {
+    
+    int* required_supported = (int *) data;
+    
+    /* Check if owner supports required */
+    if (*required_supported)
+        *required_supported = guac_user_supports_required(user);

Review comment:
       This looks to be written to allow the callback to be used similar to the 
check for WebP support (with each user of a connection being tested for the 
needed support and the test passing only if all connected users have support), 
but I only see `guac_client_owner_supports_required()` and the naming of this 
callback looks like this is intentionally limited in scope.
   
   Why not simply assign to `*required_supported` directly, without the test 
for current value and without the initialization of `required_supported`?

##########
File path: src/libguac/client.c
##########
@@ -478,6 +478,42 @@ int guac_client_load_plugin(guac_client* client, const 
char* protocol) {
 
 }
 
+/**
+ * A callback function which is invoked by guac_client_owner_send_required() to
+ * send the required parameters to the specified user, who is the owner of the
+ * client session.
+ * 
+ * @param user
+ *     The guac_user that will receive the required parameters, who is the 
owner
+ *     of the client.
+ * 
+ * @param data
+ *     A pointer to a NULL-terminated array of required parameters that will be
+ *     passed on to the owner to continue the connection.
+ * 
+ * @return
+ *     A pointer to an integer containing the return status of the send
+ *     operation.

Review comment:
       The line:
   
   ```c
   return (void*) ((intptr_t) guac_protocol_send_required(user->socket, 
required));
   ```
   
   isn't returning a pointer to an integer, but rather an integer value that 
has been cast to `void*` via `intptr_t`.

##########
File path: src/libguac/guacamole/protocol.h
##########
@@ -1007,5 +1023,32 @@ int guac_protocol_send_name(guac_socket* socket, const 
char* name);
  */
 int guac_protocol_decode_base64(char* base64);
 
+/**
+ * Given a string representation of a protocol version, search the mapping
+ * to find the matching enum version, or GUAC_PROTOCOL_VERSION_UNKNOWN if no
+ * match is found.
+ * 
+ * @param version_string
+ *     The string representation of the protocol version.
+ * 
+ * @return 
+ *     The enum value of the protocol version, or GUAC_PROTOCOL_VERSION_UNKNOWN
+ *     if no match is found.
+ */
+guac_protocol_version guac_protocol_string_to_version(char* version_string);

Review comment:
       If `guac_protocol_string_to_version()` does not modify the contents of 
`version_string`, it should be `const char*`.

##########
File path: src/protocols/vnc/argv.c
##########
@@ -0,0 +1,59 @@
+/*
+ * 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 "config.h"
+#include "argv.h"
+#include "vnc.h"
+
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_vnc_argv_callback(guac_user* user, const char* mimetype,
+        const char* name, const char* value, void* data) {
+
+    guac_client* client = user->client;
+    guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
+    guac_vnc_settings* settings = vnc_client->settings;
+
+    /* Update username */
+    if (strcmp(name, GUAC_VNC_ARGV_USERNAME) == 0) {
+        free(settings->username);
+        settings->username = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;
+    }
+    
+    /* Update password */
+    else if (strcmp(name, GUAC_VNC_ARGV_PASSWORD) == 0) {
+        free(settings->password);
+        settings->password = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;

Review comment:
       This should be `GUAC_VNC_COND_FLAG_PASSWORD`, yes?

##########
File path: src/protocols/vnc/auth.c
##########
@@ -19,27 +19,110 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
 #include <guacamole/client.h>
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
 #include <rfb/rfbclient.h>
 #include <rfb/rfbproto.h>
 
+#include <pthread.h>
+#include <string.h>
+
 char* guac_vnc_get_password(rfbClient* client) {
     guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-    return ((guac_vnc_client*) gc->data)->settings->password;
+    guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+    guac_vnc_settings* settings = vnc_client->settings;
+    
+    /* If the client does not support the "required" instruction, just return
+        the configuration data. */
+    if (!guac_client_owner_supports_required(gc))
+        return ((guac_vnc_client*) gc->data)->settings->password;
+    
+    /* If password isn't around, prompt for it. */
+    if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+        /* Lock the thread. */
+        pthread_mutex_lock(&(vnc_client->vnc_credential_lock));
+        
+        /* Send the request for password to the owner. */
+        guac_client_owner_send_required(gc,
+                (const char* []) {GUAC_VNC_ARGV_PASSWORD, NULL});
+        
+        /* Set the conditional flag. */
+        vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_PASSWORD;
+        
+        /* Wait for the condition. */
+        pthread_cond_wait(&(vnc_client->vnc_credential_cond),
+                &(vnc_client->vnc_credential_lock));
+        
+        /* Unlock the thread. */
+        pthread_mutex_unlock(&(vnc_client->vnc_credential_lock));
+    }
+    
+    return settings->password;
+    
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
     guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-    guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+    guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+    guac_vnc_settings* settings = vnc_client->settings;
     
+    /* Handle request for Username/Password credentials */
     if (credentialType == rfbCredentialTypeUser) {
         rfbCredential *creds = malloc(sizeof(rfbCredential));
-        creds->userCredential.username = settings->username;
-        creds->userCredential.password = settings->password;
-        return creds;
+        
+        /* If the client does not support the "required" instruction, just 
return
+           the parameters already configured. */
+        if (!guac_client_owner_supports_required(gc)) {
+            creds->userCredential.username = settings->username;
+            creds->userCredential.password = settings->password;
+            return creds;
+        }
+        
+        char* params[2] = {NULL};
+        int i = 0;
+        
+        /* Check if username is null or empty. */
+        if (settings->username == NULL) {
+            params[i] = GUAC_VNC_ARGV_USERNAME;
+            i++;
+            vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_USERNAME;
+        }
+        
+        /* Check if password is null or empty. */
+        if (settings->password == NULL) {
+            params[i] = GUAC_VNC_ARGV_PASSWORD;
+            i++;
+            vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_PASSWORD;
+        }
+        
+        /* If we have empty parameters, request them. */
+        if (i > 0) {
+            /* Lock the thread. */
+            pthread_mutex_lock(&(vnc_client->vnc_credential_lock));
+            
+            /* Send required parameters to owner. */
+            guac_client_owner_send_required(gc, (const char**) params);
+            
+            /* Wait for the parameters to be returned. */
+            pthread_cond_wait(&(vnc_client->vnc_credential_cond),
+                    &(vnc_client->vnc_credential_lock));

Review comment:
       Why not `guac_argv_await()`?

##########
File path: src/protocols/vnc/argv.c
##########
@@ -0,0 +1,59 @@
+/*
+ * 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 "config.h"
+#include "argv.h"
+#include "vnc.h"
+
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_vnc_argv_callback(guac_user* user, const char* mimetype,
+        const char* name, const char* value, void* data) {
+
+    guac_client* client = user->client;
+    guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
+    guac_vnc_settings* settings = vnc_client->settings;
+
+    /* Update username */
+    if (strcmp(name, GUAC_VNC_ARGV_USERNAME) == 0) {
+        free(settings->username);
+        settings->username = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;

Review comment:
       `vnc_credential_flags` should only be modified/checked while the 
`vnc_credential_lock` is acquired.

##########
File path: src/libguac/guacamole/protocol.h
##########
@@ -1007,5 +1023,32 @@ int guac_protocol_send_name(guac_socket* socket, const 
char* name);
  */
 int guac_protocol_decode_base64(char* base64);
 
+/**
+ * Given a string representation of a protocol version, search the mapping
+ * to find the matching enum version, or GUAC_PROTOCOL_VERSION_UNKNOWN if no
+ * match is found.

Review comment:
       The fact that a mapping is searched is an implementation detail that 
should be opaque to the caller of this function. I suggest documenting what 
this function does from the strict perspective of the caller - that the 
function translates the standard string representation of a protocol version 
into the corresponding `guac_protocol_version` value, returning 
`GUAC_PROTOCOL_VERSION_UNKNOWN` if the version is unknown or the string is 
invalid.

##########
File path: src/protocols/vnc/auth.c
##########
@@ -19,27 +19,110 @@
 
 #include "config.h"
 
+#include "argv.h"
 #include "auth.h"
 #include "vnc.h"
 
 #include <guacamole/client.h>
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
 #include <rfb/rfbclient.h>
 #include <rfb/rfbproto.h>
 
+#include <pthread.h>
+#include <string.h>
+
 char* guac_vnc_get_password(rfbClient* client) {
     guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-    return ((guac_vnc_client*) gc->data)->settings->password;
+    guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+    guac_vnc_settings* settings = vnc_client->settings;
+    
+    /* If the client does not support the "required" instruction, just return
+        the configuration data. */
+    if (!guac_client_owner_supports_required(gc))
+        return ((guac_vnc_client*) gc->data)->settings->password;
+    
+    /* If password isn't around, prompt for it. */
+    if (settings->password == NULL || strcmp(settings->password, "") == 0) {
+        /* Lock the thread. */
+        pthread_mutex_lock(&(vnc_client->vnc_credential_lock));
+        
+        /* Send the request for password to the owner. */
+        guac_client_owner_send_required(gc,
+                (const char* []) {GUAC_VNC_ARGV_PASSWORD, NULL});
+        
+        /* Set the conditional flag. */
+        vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_PASSWORD;
+        
+        /* Wait for the condition. */
+        pthread_cond_wait(&(vnc_client->vnc_credential_cond),
+                &(vnc_client->vnc_credential_lock));
+        
+        /* Unlock the thread. */
+        pthread_mutex_unlock(&(vnc_client->vnc_credential_lock));
+    }
+    
+    return settings->password;
+    
 }
 
 rfbCredential* guac_vnc_get_credentials(rfbClient* client, int credentialType) 
{
     guac_client* gc = rfbClientGetClientData(client, GUAC_VNC_CLIENT_KEY);
-    guac_vnc_settings* settings = ((guac_vnc_client*) gc->data)->settings;
+    guac_vnc_client* vnc_client = ((guac_vnc_client*) gc->data);
+    guac_vnc_settings* settings = vnc_client->settings;
     
+    /* Handle request for Username/Password credentials */
     if (credentialType == rfbCredentialTypeUser) {
         rfbCredential *creds = malloc(sizeof(rfbCredential));
-        creds->userCredential.username = settings->username;
-        creds->userCredential.password = settings->password;
-        return creds;
+        
+        /* If the client does not support the "required" instruction, just 
return
+           the parameters already configured. */
+        if (!guac_client_owner_supports_required(gc)) {
+            creds->userCredential.username = settings->username;
+            creds->userCredential.password = settings->password;
+            return creds;
+        }
+        
+        char* params[2] = {NULL};
+        int i = 0;
+        
+        /* Check if username is null or empty. */
+        if (settings->username == NULL) {
+            params[i] = GUAC_VNC_ARGV_USERNAME;
+            i++;
+            vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_USERNAME;
+        }
+        
+        /* Check if password is null or empty. */
+        if (settings->password == NULL) {
+            params[i] = GUAC_VNC_ARGV_PASSWORD;
+            i++;
+            vnc_client->vnc_credential_flags |= GUAC_VNC_COND_FLAG_PASSWORD;
+        }

Review comment:
       `vnc_credential_flags` should only be modified/checked while the 
`vnc_credential_lock` is acquired.

##########
File path: src/libguac/protocol.c
##########
@@ -1241,3 +1302,34 @@ int guac_protocol_decode_base64(char* base64) {
 
 }
 
+guac_protocol_version guac_protocol_string_to_version(char* version_string) {
+    
+    guac_protocol_version_mapping* current = guac_protocol_version_table;
+    while (current->version != GUAC_PROTOCOL_VERSION_UNKNOWN) {
+        
+        if (strcmp(current->version_string, version_string) == 0)
+            return current->version;
+        
+        current++;
+        
+    }
+    
+    return GUAC_PROTOCOL_VERSION_UNKNOWN;
+    
+}
+
+const char* guac_protocol_version_to_string(guac_protocol_version version) {
+    
+    guac_protocol_version_mapping* current = guac_protocol_version_table;
+    while (current->version) {

Review comment:
       `current` is never incremented within the body of this loop. Unless the 
provided version matches the first entry in the table, this function will never 
return.

##########
File path: src/libguac/tests/protocol/guac_protocol_version.c
##########
@@ -0,0 +1,69 @@
+/*
+ * 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 <CUnit/CUnit.h>
+#include <guacamole/protocol.h>
+#include <guacamole/protocol-types.h>
+
+/**
+ * Test which verifies that conversion of the guac_protocol_version enum to
+ * string values succeeds and produces the expected results.
+ */
+void test_guac_protocol__version_to_string() {
+    
+    guac_protocol_version version_a = GUAC_PROTOCOL_VERSION_1_3_0;
+    guac_protocol_version version_b = GUAC_PROTOCOL_VERSION_1_0_0;
+    guac_protocol_version version_c = GUAC_PROTOCOL_VERSION_UNKNOWN;
+    
+    CU_ASSERT_STRING_EQUAL_FATAL(guac_protocol_version_to_string(version_a), 
"VERSION_1_3_0");

Review comment:
       Any reason why all these assertions are fatal? It's often needed for 
`NULL` checks where future tests would segfault if the initial check fails, but 
that doesn't look to be the case with this test, and there's likely value in 
having the results of as many tests as possible when something is failing.

##########
File path: src/libguac/guacamole/protocol.h
##########
@@ -794,6 +794,22 @@ int guac_protocol_send_push(guac_socket* socket, const 
guac_layer* layer);
 int guac_protocol_send_rect(guac_socket* socket, const guac_layer* layer,
         int x, int y, int width, int height);
 
+/**
+ * Sends a "required" instruction over the given guac_socket connection.  This
+ * instruction indicates to the client that one or more additional parameters
+ * is needed to continue the connection.

Review comment:
       "are needed"

##########
File path: src/libguac/protocol.c
##########
@@ -1241,3 +1302,34 @@ int guac_protocol_decode_base64(char* base64) {
 
 }
 
+guac_protocol_version guac_protocol_string_to_version(char* version_string) {
+    
+    guac_protocol_version_mapping* current = guac_protocol_version_table;
+    while (current->version != GUAC_PROTOCOL_VERSION_UNKNOWN) {
+        
+        if (strcmp(current->version_string, version_string) == 0)
+            return current->version;
+        
+        current++;
+        
+    }
+    
+    return GUAC_PROTOCOL_VERSION_UNKNOWN;
+    
+}
+
+const char* guac_protocol_version_to_string(guac_protocol_version version) {
+    
+    guac_protocol_version_mapping* current = guac_protocol_version_table;
+    while (current->version) {

Review comment:
       `guac_protocol_string_to_version()` explicitly uses the value 
`GUAC_PROTOCOL_VERSION_UNKNOWN` as a sentinel, whereas this function instead 
relies on `GUAC_PROTOCOL_VERSION_UNKNOWN` being zero.
   
   With both functions doing essentially the same thing (iterating through the 
version mapping table), I think both functions should consistently use the same 
test for determining the end of the table. Given a choice between the two, 
rather than assume the integer value of `GUAC_PROTOCOL_VERSION_UNKNOWN`, I 
think the explicit check within `guac_protocol_string_to_version()` should be 
preferred.

##########
File path: src/protocols/vnc/argv.c
##########
@@ -0,0 +1,59 @@
+/*
+ * 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 "config.h"
+#include "argv.h"
+#include "vnc.h"
+
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_vnc_argv_callback(guac_user* user, const char* mimetype,
+        const char* name, const char* value, void* data) {
+
+    guac_client* client = user->client;
+    guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
+    guac_vnc_settings* settings = vnc_client->settings;
+
+    /* Update username */
+    if (strcmp(name, GUAC_VNC_ARGV_USERNAME) == 0) {
+        free(settings->username);
+        settings->username = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;
+    }
+    
+    /* Update password */
+    else if (strcmp(name, GUAC_VNC_ARGV_PASSWORD) == 0) {
+        free(settings->password);
+        settings->password = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;
+    }

Review comment:
       I think new values for username and password should only be accepted if 
their corresponding flags are set (only if they are initially omitted). Doing 
otherwise could allow a non-administrative user to manipulate the values that 
an administrator specified, forcing a login attempt as a different user than 
dictated by the admin.

##########
File path: src/protocols/vnc/argv.c
##########
@@ -0,0 +1,59 @@
+/*
+ * 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 "config.h"
+#include "argv.h"
+#include "vnc.h"
+
+#include <guacamole/protocol.h>
+#include <guacamole/socket.h>
+#include <guacamole/user.h>
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_vnc_argv_callback(guac_user* user, const char* mimetype,
+        const char* name, const char* value, void* data) {
+
+    guac_client* client = user->client;
+    guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
+    guac_vnc_settings* settings = vnc_client->settings;
+
+    /* Update username */
+    if (strcmp(name, GUAC_VNC_ARGV_USERNAME) == 0) {
+        free(settings->username);
+        settings->username = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;
+    }
+    
+    /* Update password */
+    else if (strcmp(name, GUAC_VNC_ARGV_PASSWORD) == 0) {
+        free(settings->password);
+        settings->password = strdup(value);
+        vnc_client->vnc_credential_flags &= GUAC_VNC_COND_FLAG_USERNAME;
+    }
+    
+    /* If no more credentials are required, signal the thread */
+    if (!vnc_client->vnc_credential_flags)
+        pthread_cond_signal(&vnc_client->vnc_credential_cond);

Review comment:
       I'm a bit confused by the usage of `vnc_credential_flags`. When a new 
parameter value is received via an argv stream, this handler updates 
`vnc_credential_flags` with bitwise AND of the flag value. That would clear the 
bits of all flags _except_ that flag.
   
   If the intent of `vnc_credential_flags` is that it should be zero once all 
required parameters are received, then I think the bitwise logic for updating 
the flags is incorrect, and should instead be:
   
   ```c
   vnc_client->vnc_credential_flags &= ~GUAC_VNC_COND_FLAG_WHATEVER;
   ```
   
   to clear _only_ that flag.




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

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to