You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/02/26 19:43:54 UTC
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-server/pull/156
GUACAMOLE-269: Allow backspace key behavior to be configured
Server-side changes that allow the backspace key to be configured, and, for the SSH protocol, also pass through that behavior to the SSH server via the TTY Mode Encoding options. This replaces #153.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-server working/backspace-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/156.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #156
----
commit 182ddcfdc13be27975a153f91929fa32a81670ae
Author: Nick Couchman <vn...@...>
Date: 2018-02-22T19:56:40Z
GUACAMOLE-269: Add basic support for sending TTY mode encoding.
commit 551fa7914a8513195c583b0495f94a43690be9b3
Author: Nick Couchman <vn...@...>
Date: 2018-02-23T00:56:05Z
GUACAMOLE-269: Move constant declaration to ttymode.c
commit 1a7f83235a4d42f44115ccb9ad03067046c13657
Author: Nick Couchman <vn...@...>
Date: 2018-02-23T01:10:16Z
GUACAMOLE-269: Add documentation for the defines and variables.
commit 37be908069d781b5f141c77c399197481af4945a
Author: Nick Couchman <vn...@...>
Date: 2018-02-24T03:14:11Z
GUACAMOLE-269: Allow backspace key to be configured.
commit 8427b9dfea89ba72eb7e422870a8d6893aa0e7f6
Author: Nick Couchman <vn...@...>
Date: 2018-02-25T13:41:14Z
GUACAMOLE-269: Use backspace config to set up tty modes.
----
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178542688
--- Diff: src/protocols/ssh/ssh.c ---
@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
return NULL;
}
+ /* Initialize a ttymode array */
+ const int num_tty_opcodes = 1;
+ char ssh_ttymodes[(GUAC_SSH_TTY_OPCODE_SIZE * num_tty_opcodes) + 1];
--- End diff --
Brilliant. I've used your macro and refactored code and comments with that.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279102
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
+} guac_ssh_ttymode;
+
+/**
+ * Initialize an array with the specified opcode/value
+ * pairs, and return the size, in bytes, of the array.
--- End diff --
Do mean the number of bytes written to the array? Ignoring that the size of the array is already known to the caller, returning the size of the array is not particularly useful, especially since the array may be larger than necessary.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r170947545
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,78 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+guac_ssh_ttymodes init_ttymodes() {
+ // Simple allocation for a placeholder
+ guac_ssh_ttymode* ttymode_array = malloc(1);
+
+ // Set up the initial data structure.
+ guac_ssh_ttymodes empty_modes = {
+ ttymode_array,
+ 0
+ };
+
+ return empty_modes;
--- End diff --
So, I take it this is bad? Would it be preferable to pass by reference or something like that?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178531096
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,58 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ char *current = opcode_array;
+ for (int i = 0; i < num_opcodes; i++) {
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode* ttymode = va_arg(args, guac_ssh_ttymode*);
+
+ /* Place opcode and value in array */
+ *(current++) = ttymode->opcode;
+ *(current++) = (ttymode->value >> 24) & 0xFF;
+ *(current++) = (ttymode->value >> 16) & 0xFF;
+ *(current++) = (ttymode->value >> 8) & 0xFF;
+ *(current++) = ttymode->value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ *(current) = GUAC_SSH_TTY_OP_END;
+
+ return 0;
--- End diff --
Sounds good - is there a more elegant way than just adding an integer byte counter and incrementing along the way?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178482357
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char backspace_str[2];
+ backspace_str[0] = term->backspace;
+ backspace_str[1] = '\0';
--- End diff --
You can clean this up as:
char backspace_str[2] = { term->backspace, '\0' };
or even:
char backspace_str[] = { term->backspace, '\0' };
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178622074
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +299,17 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ int ttymodeBytes = guac_ssh_ttymodes_init(ssh_ttymodes,
+ GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END);
+ if (ttymodeBytes < 1)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding \
--- End diff --
Changed to warning.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279241
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
+ opcode_array[offset + 2] = (ttymode.value >> 16) & 0xFF;
+ opcode_array[offset + 3] = (ttymode.value >> 8) & 0xFF;
+ opcode_array[offset + 4] = ttymode.value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;
--- End diff --
To confirm: the final "END" does not need the full 5-byte opcode and value? Just the opcode?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176918401
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
--- End diff --
Okay, took a stab at this, and I *think* I got it working correctly - at least, the code runs and behaves as expected. Might be worth someone looking it over, though, just to make sure I did it in a way that isn't going to hit some corner case at some point down the road.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917472
--- Diff: src/protocols/telnet/settings.c ---
@@ -328,6 +335,11 @@ guac_telnet_settings* guac_telnet_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_TELNET_CLIENT_ARGS, argv,
IDX_CREATE_RECORDING_PATH, false);
+ /* Parse backspae key code */
--- End diff --
Speling corectid.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917418
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
--- End diff --
Is it possible to do this by pointer/reference rather than value? I'm not familiar with the va_arg macro and how to use it - do I just change the argument to `&guac_ssh_ttymode` and the assignment to `guac_ssh_ttymode* ttymode`?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178614574
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +299,17 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ int ttymodeBytes = guac_ssh_ttymodes_init(ssh_ttymodes,
+ GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END);
+ if (ttymodeBytes < 1)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding \
--- End diff --
This will actually log:
Error storing TTY mode encoding opcodes and values in array.
as the whitespace at the beginning of the following line is part of the string.
To split strings in C, all you need to do is place two strings next to each other. They're automatically concatenated by the compiler:
guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding "
"opcodes and values in array.");
As this message is meant for the user/admin, I suggest rephrasing to describe things from that perspective. For example:
Unable to set TTY modes. Backspace may not work as expected.
at the warning level may make more sense.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178621626
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +299,17 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ int ttymodeBytes = guac_ssh_ttymodes_init(ssh_ttymodes,
+ GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END);
+ if (ttymodeBytes < 1)
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error storing TTY mode encoding \
--- End diff --
Changed.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175278959
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
+} guac_ssh_ttymode;
+
+/**
+ * Initialize an array with the specified opcode/value
--- End diff --
Initializes an array ... why? What's special about the array? Anything you can think of that might make the use of this function clear?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176918408
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
--- End diff --
I think I got this going, too - again, probably worth someone taking a look just to make sure. Seems to work, though.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r170825539
--- Diff: src/protocols/ssh/settings.h ---
@@ -223,6 +223,11 @@ typedef struct guac_ssh_settings {
*/
int server_alive_interval;
+ /**
+ * The decismal ASCII code of the command to send for backspace.
--- End diff --
"decismal" is a typo, but that may be moot as this is technically incorrect. While the string from which this `int` was parsed may have been decimal, at this point it's just an `int`.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175278899
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
--- End diff --
Is there a way you could describe this which provides information/context beyond the members of the structure?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917438
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
--- End diff --
Documented.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279031
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char* backspace_str = malloc(sizeof(char) * 2);
--- End diff --
Keep in mind that dynamic allocation incurs overhead. In some cases, it makes good sense to do this, but it's hard to justify for a 2-byte string. A static buffer would be more appropriate here, particularly since you can initialize things cleanly and clearly inline.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178622107
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,64 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, opcode_array);
+
+ /* Initialize array pointer and byte counter. */
+ char *current = opcode_array;
+ int bytes = 0;
+
+ /* Loop through variable argument list. */
+ while (true) {
+
+ /* Next argument should be an opcode. */
+ char opcode = (char)va_arg(args, int);
+ *(current++) = opcode;
+ bytes += sizeof(char);
+
+ /* If it's the end opcode, we're done. */
+ if (opcode == GUAC_SSH_TTY_OP_END)
+ break;
+
+ /* Next argument should be 4-byte value. */
+ uint32_t value = va_arg(args, uint32_t);
+ *(current++) = (value >> 24) & 0xFF;
+ *(current++) = (value >> 16) & 0xFF;
+ *(current++) = (value >> 8) & 0xFF;
+ *(current++) = value & 0xFF;
+ bytes += sizeof(uint32_t);
+ }
+
+ /* We're done processing arguments. */
+ va_end(args);
+
+ return bytes;
--- End diff --
Yes, of course.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178483146
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +301,15 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+ num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))
--- End diff --
Looking at the source for `guac_ssh_ttymodes_init()`, doesn't this need to be a pointer to a `guac_ssh_ttymode`? ie: `&((guac_ssh_ttymode) { GUAC_SSH_TTY_OP_VERASE, settings->backspace })` ?
Alternatively, there's no need for all these parameters to be of the same type. It would also be possible to do something like:
if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
GUAC_SSH_TTY_OP_VERASE, settings->backspace, GUAC_SSH_TTY_OP_END))
The code is your oyster.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178542720
--- Diff: src/protocols/ssh/ssh.c ---
@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
return NULL;
}
+ /* Initialize a ttymode array */
--- End diff --
Removed.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178541081
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +301,15 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+ num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))
--- End diff --
Refactored the `guac_ssh_ttymodes_init()` function. I may have introduced some other bugs or conditions that need to be handled, so let me know what you think about it.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279053
--- Diff: src/protocols/telnet/settings.c ---
@@ -328,6 +335,11 @@ guac_telnet_settings* guac_telnet_parse_args(guac_user* user,
guac_user_parse_args_boolean(user, GUAC_TELNET_CLIENT_ARGS, argv,
IDX_CREATE_RECORDING_PATH, false);
+ /* Parse backspae key code */
--- End diff --
backspae
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178482474
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +301,15 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+ num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error configuring TTY mode encoding.");
--- End diff --
What do you mean by "configuring TTY mode encoding"? Is there a way to phrase this which would better represent what is failing?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-server/pull/156
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178483536
--- Diff: src/protocols/ssh/ssh.c ---
@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
return NULL;
}
+ /* Initialize a ttymode array */
--- End diff --
Point of clarification - nothing is being initialized here, only declared. With that much being obvious (that the array is being declared), rather than change "Initialize" to "Declare", I'd say just kill this line.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r170826387
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,78 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+guac_ssh_ttymodes init_ttymodes() {
+ // Simple allocation for a placeholder
+ guac_ssh_ttymode* ttymode_array = malloc(1);
+
+ // Set up the initial data structure.
+ guac_ssh_ttymodes empty_modes = {
+ ttymode_array,
+ 0
+ };
+
+ return empty_modes;
--- End diff --
Assigning/returning a struct by value like this actually does an implicit copy in C.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917457
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char* backspace_str = malloc(sizeof(char) * 2);
+ backspace_str[0] = term->backspace;
+ backspace_str[1] = '\0';
+ return guac_terminal_send_string(term, backspace_str);
--- End diff --
Staticized.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r170825324
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1599,7 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ if (keysym == 0xFF08) return guac_terminal_send_string(term, &term->backspace); /* Backspace */
--- End diff --
`guac_terminal_send_string()` requires a null-terminated string. As `&term->backspace` is a pointer to a single character, there is no guarantee of null terminator, and this may read without bound, resulting in connection closure or dumping of memory data to the remote terminal.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178615175
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,81 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Macro for calculating the number of bytes required
+ * to pass a given number of opcodes, which calculates
+ * the size of the number of opcodes plus the single byte
+ * end opcode.
+ */
+#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)
--- End diff --
Please document `num_opcodes` with `@param`, just as you would for a function.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279035
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char* backspace_str = malloc(sizeof(char) * 2);
+ backspace_str[0] = term->backspace;
+ backspace_str[1] = '\0';
+ return guac_terminal_send_string(term, backspace_str);
--- End diff --
Returning here without freeing the memory allocated above will leak memory each time backspace is pressed. Probably shouldn't be dynamically allocating at all, though (see above).
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178483453
--- Diff: src/protocols/ssh/ssh.c ---
@@ -191,6 +192,10 @@ void* ssh_client_thread(void* data) {
return NULL;
}
+ /* Initialize a ttymode array */
+ const int num_tty_opcodes = 1;
+ char ssh_ttymodes[(GUAC_SSH_TTY_OPCODE_SIZE * num_tty_opcodes) + 1];
--- End diff --
Not really a dealbreaker, but if you want to ensure this is a constant expression while also abstracting away the calculations required, this could be handled via a macro defined in `ttymode.h`. For example:
#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)
You could then rely upon:
char ssh_ttymodes[GUAC_SSH_TTYMODES_SIZE(1)];
... and, of course, documenting within `guac_ssh_ttymodes_init()` how the minimum size for the provided buffer should be calculated.
Magic.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917426
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
+ opcode_array[offset + 2] = (ttymode.value >> 16) & 0xFF;
+ opcode_array[offset + 3] = (ttymode.value >> 8) & 0xFF;
+ opcode_array[offset + 4] = ttymode.value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;
+
+ return 0;
--- End diff --
Documentation has been fixed.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178540964
--- Diff: src/protocols/ssh/ssh.c ---
@@ -296,9 +301,15 @@ void* ssh_client_thread(void* data) {
}
+ /* Set up the ttymode array prior to requesting the PTY */
+ if (guac_ssh_ttymodes_init(ssh_ttymodes, sizeof(ssh_ttymodes),
+ num_tty_opcodes, (guac_ssh_ttymode){ GUAC_SSH_TTY_OP_VERASE, settings->backspace}))
+ guac_client_abort(client, GUAC_PROTOCOL_STATUS_SERVER_ERROR, "Error configuring TTY mode encoding.");
--- End diff --
Tried to clean up this error message bit.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917475
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
+} guac_ssh_ttymode;
+
+/**
+ * Initialize an array with the specified opcode/value
+ * pairs, and return the size, in bytes, of the array.
--- End diff --
Documentation updated.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279087
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
+ opcode_array[offset + 2] = (ttymode.value >> 16) & 0xFF;
+ opcode_array[offset + 3] = (ttymode.value >> 8) & 0xFF;
+ opcode_array[offset + 4] = ttymode.value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;
+
+ return 0;
--- End diff --
The documentation states that this function returns the size of the array (or rather, I assume, the number of bytes written to the array), but this isn't happening here.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279204
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
--- End diff --
The structure in this case is rather small, so this does not make much of a difference in this case, but beware that passing structures by value will result in a full copy of that structure, with this assignment resulting in another full copy.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178622206
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,81 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Macro for calculating the number of bytes required
+ * to pass a given number of opcodes, which calculates
+ * the size of the number of opcodes plus the single byte
+ * end opcode.
+ */
+#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)
--- End diff --
Documented that, and also documented the return value with `@returns` - not sure if that should be done, too?
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178482718
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,58 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
--- End diff --
There must be a matching call to `va_end()` before the end of this function.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917434
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
--- End diff --
Took a stab at some more thorough documentation.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178540922
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,58 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ char *current = opcode_array;
+ for (int i = 0; i < num_opcodes; i++) {
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode* ttymode = va_arg(args, guac_ssh_ttymode*);
+
+ /* Place opcode and value in array */
+ *(current++) = ttymode->opcode;
+ *(current++) = (ttymode->value >> 24) & 0xFF;
+ *(current++) = (ttymode->value >> 16) & 0xFF;
+ *(current++) = (ttymode->value >> 8) & 0xFF;
+ *(current++) = ttymode->value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ *(current) = GUAC_SSH_TTY_OP_END;
+
+ return 0;
--- End diff --
I've gone the byte counter route - if you have a suggestion for another way to do it, let me know.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178533605
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,58 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
--- End diff --
Added.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175279222
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
--- End diff --
You don't need to manually calculate the offset for each byte written. If you have a pointer to the next byte to be written within the array, this could be as simple as:
*(current++) = whatever_you_want_to_write;
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175313519
--- Diff: src/protocols/ssh/ttymode.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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ for (int i = 0; i < num_opcodes; i++) {
+ /* Calculate offset in array */
+ int offset = i * GUAC_SSH_TTY_OPCODE_SIZE;
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode ttymode = va_arg(args, guac_ssh_ttymode);
+
+ /* Place opcode and value in array */
+ opcode_array[offset] = ttymode.opcode;
+ opcode_array[offset + 1] = (ttymode.value >> 24) & 0xFF;
+ opcode_array[offset + 2] = (ttymode.value >> 16) & 0xFF;
+ opcode_array[offset + 3] = (ttymode.value >> 8) & 0xFF;
+ opcode_array[offset + 4] = ttymode.value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ opcode_array[num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE] = GUAC_SSH_TTY_OP_END;
--- End diff --
That's correct - as soon as it encounters the 0 opcode in one of the expected positions (5 byte offsets, basically) it stops.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178530634
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char backspace_str[2];
+ backspace_str[0] = term->backspace;
+ backspace_str[1] = '\0';
--- End diff --
Of course. Done & thanks.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r170825936
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,78 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+guac_ssh_ttymodes init_ttymodes() {
+ // Simple allocation for a placeholder
--- End diff --
Comments for C source within Guacamole should use `/* ... */` syntax, even if only a single line.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917443
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
+} guac_ssh_ttymode;
+
+/**
+ * Initialize an array with the specified opcode/value
--- End diff --
Beefed up this documentation.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r175278922
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,80 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Simple structure that defines a opcode and
+ * value pair.
+ */
+typedef struct guac_ssh_ttymode {
+ char opcode;
+ uint32_t value;
--- End diff --
Structure members need to be documented.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178482037
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,58 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], const int array_size,
+ const int num_opcodes, ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, num_opcodes);
+
+ /* Check to make sure the array has enough space.
+ We need one extra byte at the end for the ending opcode. */
+ if ((num_opcodes * GUAC_SSH_TTY_OPCODE_SIZE) >= (array_size))
+ return 1;
+
+ char *current = opcode_array;
+ for (int i = 0; i < num_opcodes; i++) {
+
+ /* Get the next argument to this function */
+ guac_ssh_ttymode* ttymode = va_arg(args, guac_ssh_ttymode*);
+
+ /* Place opcode and value in array */
+ *(current++) = ttymode->opcode;
+ *(current++) = (ttymode->value >> 24) & 0xFF;
+ *(current++) = (ttymode->value >> 16) & 0xFF;
+ *(current++) = (ttymode->value >> 8) & 0xFF;
+ *(current++) = ttymode->value & 0xFF;
+ }
+
+ /* Put the end opcode in the last opcode space */
+ *(current) = GUAC_SSH_TTY_OP_END;
+
+ return 0;
--- End diff --
I recommend instead returning the size of the data written to the array, with zero indicating failure. This would remove the need for the caller to know ahead of time the size of the data this function will write (which is an odd requirement for what is otherwise an abstraction).
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178614942
--- Diff: src/protocols/ssh/ttymode.c ---
@@ -0,0 +1,64 @@
+/*
+ * 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 "ttymode.h"
+
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+
+int guac_ssh_ttymodes_init(char opcode_array[], ...) {
+
+ /* Initialize the variable argument list. */
+ va_list args;
+ va_start(args, opcode_array);
+
+ /* Initialize array pointer and byte counter. */
+ char *current = opcode_array;
+ int bytes = 0;
+
+ /* Loop through variable argument list. */
+ while (true) {
+
+ /* Next argument should be an opcode. */
+ char opcode = (char)va_arg(args, int);
+ *(current++) = opcode;
+ bytes += sizeof(char);
+
+ /* If it's the end opcode, we're done. */
+ if (opcode == GUAC_SSH_TTY_OP_END)
+ break;
+
+ /* Next argument should be 4-byte value. */
+ uint32_t value = va_arg(args, uint32_t);
+ *(current++) = (value >> 24) & 0xFF;
+ *(current++) = (value >> 16) & 0xFF;
+ *(current++) = (value >> 8) & 0xFF;
+ *(current++) = value & 0xFF;
+ bytes += sizeof(uint32_t);
+ }
+
+ /* We're done processing arguments. */
+ va_end(args);
+
+ return bytes;
--- End diff --
FYI - you can actually do:
current - opcode_array
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r178624284
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,81 @@
+/*
+ * 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_SSH_TTYMODE_H
+#define GUAC_SSH_TTYMODE_H
+
+#include "config.h"
+
+#include <stdint.h>
+
+/**
+ * The size of a TTY mode encoding opcode and
+ * value pair. As defined in the SSH RFC, this
+ * is 5 bytes - a single byte for the opcode, and
+ * 4 bytes for the value.
+ */
+#define GUAC_SSH_TTY_OPCODE_SIZE 5
+
+/**
+ * The SSH TTY mode encoding opcode that terminates
+ * the list of TTY modes.
+ */
+#define GUAC_SSH_TTY_OP_END 0
+
+/**
+ * The SSH TTY mode encoding opcode that configures
+ * the TTY erase code to configure the server
+ * backspace key.
+ */
+#define GUAC_SSH_TTY_OP_VERASE 3
+
+/**
+ * Macro for calculating the number of bytes required
+ * to pass a given number of opcodes, which calculates
+ * the size of the number of opcodes plus the single byte
+ * end opcode.
+ */
+#define GUAC_SSH_TTYMODES_SIZE(num_opcodes) ((GUAC_SSH_TTY_OPCODE_SIZE * num_opcodes) + 1)
--- End diff --
Yup, definitely. Good catch.
---
[GitHub] guacamole-server pull request #156: GUACAMOLE-269: Allow backspace key behav...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/156#discussion_r176917451
--- Diff: src/terminal/terminal.c ---
@@ -1594,7 +1598,13 @@ static int __guac_terminal_send_key(guac_terminal* term, int keysym, int pressed
/* Non-printable keys */
else {
- if (keysym == 0xFF08) return guac_terminal_send_string(term, "\x7F"); /* Backspace */
+ /* Backspace can vary based on configuration of terminal by client. */
+ if (keysym == 0xFF08) {
+ char* backspace_str = malloc(sizeof(char) * 2);
--- End diff --
Switched to static.
---