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/22 20:20:00 UTC
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-server/pull/153
GUACAMOLE-269: Add basic support for sending TTY mode encoding over SSH
This is a very basic change that takes care of the problem described in GUACAMOLE-269, specifically in SSH, where the remote system does not correctly register the TTY erase character. It adds the necessary data structure for passing this through, and the changes to the `libssh2_channel_request_pty_ex()` function to pass the structure through.
I tried to do this in a way that allows it to be extended easily in the future. Not sure if this is a sane approach or not - it does take care of the issue raised in -269 for the SSH protocol, but I welcome any comments or suggestions on other approaches.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-server GUACAMOLE-269
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/153.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 #153
----
commit 182ddcfdc13be27975a153f91929fa32a81670ae
Author: Nick Couchman <vn...@...>
Date: 2018-02-22T19:56:40Z
GUACAMOLE-269: Add basic support for sending TTY mode encoding.
----
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170140293
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
--- End diff --
Fixed.
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170140263
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
+
+#define GUAC_SSH_TERM_DEFAULT_BACKSPACE 127
+
+char guac_tty_modes[] = {
--- End diff --
Fixed.
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170126061
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
+
+#define GUAC_SSH_TERM_DEFAULT_BACKSPACE 127
+
+char guac_tty_modes[] = {
--- End diff --
Global variables within pre-defined values should be defined in the header file using `extern`, with the value of that variable being provided by a corresponding `.c` file. Assuming this is meant to be a constant, the naming should also follow the `UPPERCASE_WITH_UNDERSCORES` convention being used for constants.
For example, see the definition of `GUAC_TERMINAL_INITIAL_PALETTE` within `palette.h`:
https://github.com/apache/guacamole-server/blob/bc5b01d4d8ab0c3c89a08007316d33012261f6b3/src/terminal/terminal/palette.h#L187-L192
whose value is defined in `palette.c`:
https://github.com/apache/guacamole-server/blob/bc5b01d4d8ab0c3c89a08007316d33012261f6b3/src/terminal/palette.c#L23-L34
Again, assuming this is meant as a constant, it should also be declared with a proper `const` qualifier.
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170141773
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
+
+#define GUAC_SSH_TERM_DEFAULT_BACKSPACE 127
--- End diff --
Documented.
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman closed the pull request at:
https://github.com/apache/guacamole-server/pull/153
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170126687
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
--- End diff --
Unless there is a technical reason to do otherwise, global macros/variables need to be namespaced (in this case, I assume that would be `GUAC_SSH_`).
---
[GitHub] guacamole-server pull request #153: GUACAMOLE-269: Add basic support for sen...
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/153#discussion_r170126151
--- Diff: src/protocols/ssh/ttymode.h ---
@@ -0,0 +1,36 @@
+/*
+ * 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"
+
+#define TTY_OP_END 0
+#define TTY_OP_VERASE 3
+
+#define GUAC_SSH_TERM_DEFAULT_BACKSPACE 127
--- End diff --
This and all other macros/variables declared here need to be documented.
---