Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-server/pull/11#discussion_r64854256
--- Diff: src/common-ssh/guac_ssh.c ---
@@ -274,19 +275,36 @@ static void guac_common_ssh_kbd_callback(const char
*name, int name_len,
}
+void ssh_client_error(bool abortOnError, guac_client* client,
guac_protocol_status status,
--- End diff --
The are a few issues here:
1. The other functions defined in this part of guacamole-server all have
the namespace/prefix `guac_common_ssh_`. Should be the same here.
2. If this is not used externally (and not declared in a header file), it
should be `static`.
3. Documentation of the function and its parameters is missing.
4. The variable `abortOnError` does not follow the naming style of the rest
of this file or guacamole-server. Please user lowercase, separating words by
underscores.
As a minor nitpick, it's strange to me to a flag variable as the first
argument in a function. Normally such things are toward the end of the
parameter list. As this is essentially a selective wrapper for
`guac_client_abort()` and `guac_client_log()`, I would recommend following the
same ordering to avoid confusion when people try to develop against this in the
future.
I also notice that the `status` parameter is ignored in the case that the
abort flag is set. I don't immediately see a way to avoid that with this
approach, but perhaps that is a sign that a different approach would be better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
