RFC: Introduce remote console for CLI interpreter via telnet

RFC: Introduce remote console for CLI interpreter via telnet

[Grigory Tolstolytkin]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

Hi,

Attached is the patch that introduces telnet service which can accept 
and execute CLI commands from the remote user.
It is controlled by two additional MI commands: "-start-telnet-service 
[port]" and "-stop-telnet-service".

After the service is started, user can connect to the gdb via telnet and 
execute CLI commands in parallel with existing MI interface. Thereby it 
is something like a "backdoor" to the gdb.

The patch still needs some additional documentation/changelog entry, but 
the implementation is done in general and ready for the criticism. I'm 
also looking on posibility to add few automated testcases and integrate 
them in.

Current implementation assumes that the inferior is executed in async 
mode and requires "set target-async" to be set to "on".
Two additional options should also be turned on if remote user is going 
to issue interactive commands (like "commands", etc). They are "set 
confirm on" and "set interactive on" and can be set on-the-fly by the 
remote user.

The rest is quite straightforward. When the user connects over telnet, 
it receives gdb prompt and able to issue CLI commands and receive the 
result back.

Regards,
Grigory

[-- Attachment #2: 0001-Add-mi-telnet-support.patch --]
[-- Type: text/x-patch, Size: 15982 bytes --]

---
 gdb/Makefile.in    |   13 +
 gdb/configure      |   24 ++
 gdb/configure.ac   |   19 ++
 gdb/mi/mi-cmds.c   |    4 
 gdb/mi/mi-cmds.h   |    4 
 gdb/mi/mi-telnet.c |  426 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 490 insertions(+)

Index: b/gdb/Makefile.in
===================================================================
--- b.orig/gdb/Makefile.in	2011-10-21 16:45:15.075968998 +0400
+++ b/gdb/Makefile.in	2011-10-28 17:52:21.476015998 +0400
@@ -221,6 +221,15 @@
 SUBDIR_MI_CFLAGS=
 
 #
+# MI telnet directory definitions
+#
+SUBDIR_MITEL_OBS = mi-telnet.o
+SUBDIR_MITEL_SRCS = mi/mi-telnet.c
+SUBDIR_MITEL_DEPS=
+SUBDIR_MITEL_LDFLAGS=
+SUBDIR_MITEL_CFLAGS= -DENABLE_GDBMITEL
+
+#
 # TUI sub directory definitions
 #
 
@@ -1939,6 +1948,10 @@
 	$(COMPILE) $(srcdir)/mi/mi-common.c
 	$(POSTCOMPILE)
 
+mi-telnet.o: $(srcdir)/mi/mi-telnet.c
+	$(COMPILE) $(srcdir)/mi/mi-telnet.c
+	$(POSTCOMPILE)
+
 # gdb/common/ dependencies
 #
 # Need to explicitly specify the compile rule as make will do nothing
Index: b/gdb/configure
===================================================================
--- b.orig/gdb/configure	2011-10-21 16:45:12.585968998 +0400
+++ b/gdb/configure	2011-10-21 16:47:17.245968998 +0400
@@ -954,6 +954,7 @@
 enable_64_bit_bfd
 enable_gdbcli
 enable_gdbmi
+enable_gdbmitel
 enable_tui
 enable_gdbtk
 with_libunwind
@@ -1631,6 +1632,7 @@
   --enable-64-bit-bfd     64-bit support (on hosts with narrower word sizes)
   --disable-gdbcli        disable command-line interface (CLI)
   --disable-gdbmi         disable machine-interface (MI)
+  --enable-gdbmitel       enable remote interface over telnet
   --enable-tui            enable full-screen terminal user interface (TUI)
   --enable-gdbtk          enable gdbtk graphical user interface (GUI)
   --enable-profiling      enable profiling of GDB
@@ -8180,6 +8182,28 @@
   fi
 fi
 
+# Enable MI telnet.
+# Check whether --enable-gdbmitel was given.
+if test "${enable_gdbmitel+set}" = set; then :
+  enableval=$enable_gdbmitel; case $enableval in
+    yes | no)
+      ;;
+    *)
+      as_fn_error "bad value $enableval for --enable-gdbmitel" "$LINENO" 5 ;;
+  esac
+else
+  enable_gdbmitel=no
+fi
+
+if test x"$enable_gdbmitel" = xyes; then
+  if test -d $srcdir/mi; then
+    CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_MITEL_OBS)"
+    CONFIG_DEPS="$CONFIG_DEPS \$(SUBDIR_MITEL_DEPS)"
+    CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_MITEL_SRCS)"
+    ENABLE_CFLAGS="$ENABLE_CFLAGS \$(SUBDIR_MITEL_CFLAGS)"
+  fi
+fi
+
 # Enable TUI.
 # Check whether --enable-tui was given.
 if test "${enable_tui+set}" = set; then :
Index: b/gdb/configure.ac
===================================================================
--- b.orig/gdb/configure.ac	2011-10-21 16:45:09.985968998 +0400
+++ b/gdb/configure.ac	2011-10-21 16:46:03.775968998 +0400
@@ -303,6 +303,25 @@
   fi
 fi
 
+# Enable MI telnet.
+AC_ARG_ENABLE(gdbmitel,
+AS_HELP_STRING([--enable-gdbmitel], [enable remote interface over telnet]),
+  [case $enableval in
+    yes | no)
+      ;;
+    *)
+      AC_MSG_ERROR([bad value $enableval for --enable-gdbmitel]) ;;
+  esac],
+  [enable_gdbmitel=no])
+if test x"$enable_gdbmitel" = xyes; then
+  if test -d $srcdir/mi; then
+    CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_MITEL_OBS)"
+    CONFIG_DEPS="$CONFIG_DEPS \$(SUBDIR_MITEL_DEPS)"
+    CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_MITEL_SRCS)"
+    ENABLE_CFLAGS="$ENABLE_CFLAGS \$(SUBDIR_MITEL_CFLAGS)"
+  fi
+fi
+
 # Enable TUI.
 AC_ARG_ENABLE(tui,
 AS_HELP_STRING([--enable-tui], [enable full-screen terminal user interface (TUI)]),
Index: b/gdb/mi/mi-cmds.c
===================================================================
--- b.orig/gdb/mi/mi-cmds.c	2011-10-21 16:45:22.655968998 +0400
+++ b/gdb/mi/mi-cmds.c	2011-10-21 16:46:41.615968998 +0400
@@ -137,6 +137,10 @@
   { "var-show-attributes", { NULL, 0 }, mi_cmd_var_show_attributes},
   { "var-show-format", { NULL, 0 }, mi_cmd_var_show_format},
   { "var-update", { NULL, 0 }, mi_cmd_var_update},
+#ifdef ENABLE_GDBMITEL
+  { "start-telnet-service", { NULL, 0 }, mi_cmd_start_telnet_service},
+  { "stop-telnet-service", { NULL, 0 }, mi_cmd_stop_telnet_service},
+#endif
   { NULL, }
 };
 
Index: b/gdb/mi/mi-cmds.h
===================================================================
--- b.orig/gdb/mi/mi-cmds.h	2011-10-21 16:45:21.785968998 +0400
+++ b/gdb/mi/mi-cmds.h	2011-10-21 16:46:23.845968998 +0400
@@ -116,6 +116,10 @@
 extern mi_cmd_argv_ftype mi_cmd_var_update;
 extern mi_cmd_argv_ftype mi_cmd_enable_pretty_printing;
 extern mi_cmd_argv_ftype mi_cmd_var_set_update_range;
+#ifdef ENABLE_GDBMITEL
+extern mi_cmd_argv_ftype mi_cmd_start_telnet_service;
+extern mi_cmd_argv_ftype mi_cmd_stop_telnet_service;
+#endif
 
 /* Description of a single command. */
 
Index: b/gdb/mi/mi-telnet.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/gdb/mi/mi-telnet.c	2011-11-11 16:20:48.582148706 +0300
@@ -0,0 +1,426 @@
+/* MI telnet support
+
+   Copyright (C) 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010,
+   2011 Free Software Foundation, Inc.
+
+   Contributed by Mentor Graphics Company <www.mentor.com>
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/*
+  MI telnet support allows remote user to connect to gdb using telnet.
+  GDB should be built with "--enable-gdbmitel=yes" option to enable MI telnet.
+
+  To Start the service use MI command "-start-telnet-service [port]".
+  Default port value is 9950.
+  Service can be stopped by issuing "-stop-telnet-service" command.
+
+  The service requires asynchronous debugging mode. It should be set with
+  "set target-async on" option.
+
+  The following options are required for interactive commands (like "commands")
+  and can be turned on directly via telnet on the fly:
+  "set confirm on"
+  "set interactive on"
+*/
+
+#include <sys/socket.h>
+#include <netinet/in.h>
+
+#include "defs.h"
+#include "top.h"
+#include "mi-cmds.h"
+#include "gdb_string.h"
+#include "event-loop.h"
+#include "interps.h"
+#include "main.h"
+#include "breakpoint.h"
+#include "cli/cli-cmds.h"
+#include "cli/cli-decode.h"
+
+#define DEFAULT_PORT 9950
+#define MAX_CMD_LENGTH 65536
+
+/* socket for accepting telnet connections */
+static int telnet_s = -1;
+
+/* client connection descriptor */
+static int client_fd = -1;
+
+/* buffer for received command and for intermediate interactive questions */
+static char cmd_buf[MAX_CMD_LENGTH];
+static char cmd_buf_intermediate[MAX_CMD_LENGTH];
+
+/* for output redirecting */
+static struct ui_file *str_file = NULL;
+
+/*
+ * Send the reply to remote client
+ */
+static void
+reply_msg (const char *msg)
+{
+  char *result = NULL;
+  long len;
+
+  if (client_fd != -1 && msg != NULL) {
+    /* flush additional output if any */
+    if (str_file) {
+      gdb_flush (str_file);
+      result = ui_file_xstrdup (str_file, &len);
+
+      send (client_fd, result, len, 0);
+
+      xfree (result);
+      ui_file_rewind (str_file);
+    }
+
+    send (client_fd, msg, strlen (msg), 0);
+  }
+}
+
+/*
+ * Hook for readline to receive input over telnet
+ */
+static char *
+telnet_readline_hook (char *prompt)
+{
+  long len;
+
+  reply_msg (prompt);
+
+  len = recv (client_fd, cmd_buf_intermediate, MAX_CMD_LENGTH, 0);
+  if (len <= 0)
+    return NULL;
+
+ cmd_buf_intermediate [len -1] = cmd_buf_intermediate [len -2] = '\0';
+
+  return xstrdup(cmd_buf_intermediate);
+};
+
+/*
+ * Hook for query to ask remote client for additional 'yes or no' question
+ */
+static int
+telnet_query_hook (const char *msg, va_list argp)
+{
+  int answer;
+  int retval = 0;
+  char *question = NULL;
+
+  gdb_flush (gdb_stdout);
+
+  question = xstrvprintf (msg, argp);
+
+  reply_msg (question);
+  reply_msg ("(y or n) ");
+
+  answer = recv (client_fd, cmd_buf_intermediate, MAX_CMD_LENGTH, 0);
+  if (answer <= 0)
+    return 0; /* 'No' by default */
+
+  /* Check the first letter in the answer */
+  switch (cmd_buf_intermediate[0]) {
+    case 'y':
+    case 'Y':
+      retval = 1;
+      break;
+
+    case 'n':
+    case 'N':
+    default:
+      retval = 0;
+      break;
+  }
+
+  xfree(question);
+  return retval;
+}
+
+/*
+ * Add "&" postfix for "run" commands to ensure background execution.
+ * TODO: Are there any other way to detect blocking commands?
+ */
+static const char *const async_commands[] = {
+  "run",
+  "continue",
+  "next",
+  "finish",
+  "until",
+  "go",
+  NULL,
+};
+
+void
+force_async_run_command (char *buffer, int len)
+{
+  int i;
+  struct cmd_list_element *c;
+  char *command;
+
+  /* Is "&" already specified in command? */
+  if (strchr (buffer, '&'))
+    return;
+
+  /* make a copy of the command since lookup_cmd will erase it */
+  command = xstrdup (buffer);
+
+  c = lookup_cmd (&command, cmdlist, "", -1, 1);
+  if (!c) {
+    xfree (command);
+    return;
+  }
+
+  /* find out whether command should be followed by '&' and add it */
+  for (i = 0; async_commands[i] != NULL; i++) {
+    if (!strcmp (async_commands[i], c->name)) {
+      buffer[len - 2] = ' ';
+      buffer[len - 1] = '&';
+      buffer[len] = '\0';
+    }
+  }
+}
+
+/*
+ * Read and execute telnet command
+ */
+static void
+telnet_event_handler(int err, gdb_client_data client_data)
+{
+  long len;
+  struct gdb_exception e;
+  char *result = NULL;
+  struct cleanup *cleanup;
+  struct interp *old_interp = NULL, *interp_to_use = NULL;
+
+  /* receive remote command */
+  len = recv (client_fd, cmd_buf, MAX_CMD_LENGTH, 0);
+  if (len <= 0) {
+    /* client disconnected */
+    delete_file_handler (client_fd);
+    close (client_fd);
+    client_fd = -1;
+    return;
+  }
+
+  /* remove CR/LF sent by telnet */
+  cmd_buf[len - 1] = cmd_buf[len - 2] = '\0';
+
+  /* make sure "run" commands are in async mode */
+  force_async_run_command(cmd_buf, len);
+
+  /* setup cleanups for input/output */
+  cleanup = make_cleanup (null_cleanup, NULL);
+  make_cleanup_restore_ui_file (&gdb_stdin);
+  make_cleanup_restore_ui_file (&gdb_stdout);
+  make_cleanup_restore_ui_file (&gdb_stderr);
+  make_cleanup_restore_ui_file (&gdb_stdlog);
+  make_cleanup_restore_ui_file (&gdb_stdtarg);
+  make_cleanup_restore_ui_file (&gdb_stdtargerr);
+
+  /* execute command in sync, set batch mode to off */
+  make_cleanup_restore_integer (&interpreter_async);
+  make_cleanup_restore_integer (&batch_flag);
+  interpreter_async = 1;
+  batch_flag = 0;
+
+  /* figure out current interpreter */
+  if (current_interp_named_p (INTERP_MI)) {
+    old_interp = interp_lookup (INTERP_MI);
+  } else
+  if (current_interp_named_p (INTERP_MI1)) {
+    old_interp = interp_lookup (INTERP_MI1);
+  } else
+  if (current_interp_named_p (INTERP_MI2)) {
+    old_interp = interp_lookup (INTERP_MI2);
+  } else
+  if (current_interp_named_p (INTERP_MI3)) {
+    old_interp = interp_lookup (INTERP_MI3);
+  };
+
+  /* switch to console interp */
+  interp_to_use = interp_lookup (INTERP_CONSOLE);
+  if (!interp_set (interp_to_use, 0)) {
+    reply_msg ("Error: unexpectedly failed to use CLI the interpreter\n");
+    do_cleanups (cleanup);
+    return;
+  }
+
+  /* first command from remote client, allocate file for output */
+  if (!str_file)
+    str_file = mem_fileopen ();
+
+  /* redirect and catch all input/output into string */
+  if (ui_out_redirect (current_uiout, str_file) < 0)
+    reply_msg ("Error: unexpectedly failed to redirect user interface. No command results will be print\n");
+  else
+    make_cleanup_ui_out_redirect_pop (current_uiout);
+
+  gdb_stdout = str_file;
+  gdb_stderr = str_file;
+  gdb_stdlog = str_file;
+  gdb_stdtarg = str_file;
+  gdb_stdtargerr = str_file;
+
+  /* Hooks for commands that may ask user for questions
+   * TODO: remote user still should explicitly do "set confirm on; set interactive on"
+   * for proper execution of interactive commands (like "commands <breakpoint number>")
+   */
+  deprecated_readline_hook = telnet_readline_hook;
+  deprecated_query_hook = telnet_query_hook;
+
+  /* console interp is properly set up, execute the command */
+  TRY_CATCH (e, RETURN_MASK_ALL)
+    {
+      execute_command (cmd_buf, 1);
+    }
+
+  /* do any breakpoint-related stuff */
+  bpstat_do_actions ();
+
+  deprecated_readline_hook = 0;
+  deprecated_query_hook = 0;
+
+  gdb_flush (str_file);
+
+  if (e.reason < 0) {
+    reply_msg ("Error: ");
+    reply_msg (e.message);
+    reply_msg ("\n");
+  }
+
+  /* send the reply to the client */
+  result = ui_file_xstrdup (str_file, &len);
+  if (len > 0) {
+    send (client_fd, result, len, 0);
+    xfree (result);
+  }
+
+  /* clear output buffer and restore gdb settings */
+  ui_file_rewind (str_file);
+  do_cleanups (cleanup);
+
+  /* show prompt */
+  reply_msg ("\n(gdb) ");
+
+  /* get back to MI interp */
+  interp_set (old_interp, 0);
+}
+
+/*
+ * Handler for the listening socket
+ * Accepts client connection and registers it within event_loop
+ */
+static void
+telnet_accept_handler(int err, gdb_client_data client_data)
+{
+  struct sockaddr_in client;
+  socklen_t len;
+  struct gdb_exception e;
+  int s = -1;
+
+  s = accept (telnet_s, (struct sockaddr *)&client, &len);
+  if (s == -1) {
+    return;
+    close (telnet_s);
+    telnet_s = client_fd = -1;
+    return;
+  }
+
+  if (client_fd != -1) {
+    reply_msg ("Error: another client already connected\n");
+    close (s);
+    return;
+  } else
+    client_fd = s;
+
+  /* add handler for the client socket */
+  add_file_handler (client_fd, telnet_event_handler, 0);
+
+  /* Remote connection is set up, show prompt */
+  reply_msg ("(gdb) ");
+}
+
+/*
+ * Command to start listening for client on specified port
+ */
+void
+mi_cmd_start_telnet_service (char *command, char **argv, int argc)
+{
+  struct sockaddr_in serv, client;
+  int port;
+  socklen_t len;
+
+  if (telnet_s != -1 || client_fd != -1)
+    error (_("Error: telnet service already started"));
+
+  if (argc != 0 && argc != 1)
+    error (_("Usage: -start_telnet_service [port]"));
+
+  if (argc == 1)
+    port = atoi(argv[0]);
+  else
+    port = DEFAULT_PORT;
+
+  if (port <= 0)
+    error (_("Error: wrong port value: %d"), port);
+
+  /* setup server side of telnet service */
+  memset ((char *)&serv, 0, sizeof (serv));
+  serv.sin_family = AF_INET;
+  serv.sin_port = htons (port);
+  serv.sin_addr.s_addr = htonl (INADDR_ANY);
+
+  telnet_s = socket (AF_INET, SOCK_STREAM, IPPROTO_TCP);
+  if (telnet_s == -1)
+    error (_("Error: socket can't be created"));
+
+  if (bind (telnet_s, (const struct sockaddr *)&serv, sizeof (serv)) == -1) {
+    close(telnet_s);
+    telnet_s = -1;
+    error (_("Error: port %d can't be bind"), port);
+  }
+
+  /* one client so far allowed */
+  if (listen (telnet_s, 1) == -1) {
+    close(telnet_s);
+    telnet_s = -1;
+    error (_("Error: %s"), safe_strerror (errno));
+  }
+
+  /* register handler that will accept connections */
+  add_file_handler (telnet_s, telnet_accept_handler, 0);
+};
+
+/*
+ * Command to stop listening for remote clients
+ */
+void
+mi_cmd_stop_telnet_service (char *command, char **argv, int argc)
+{
+  if (client_fd != -1) {
+    delete_file_handler (client_fd);
+    close (client_fd);
+  }
+
+  if (telnet_s != -1) {
+    delete_file_handler (telnet_s);
+    close (telnet_s);
+  }
+
+  telnet_s = client_fd = -1;
+}
+

Re: RFC: Introduce remote console for CLI interpreter via telnet

[Joel Brobecker]

Grigory,

First, thank you for sending the patch.

> Attached is the patch that introduces telnet service which can
> accept and execute CLI commands from the remote user.

No one commented on the desireability of such a feature.
I'm a bit indifferent, to tell you the truth, and it would
be nice to hear from others. For now, I've only skimmed over
the patch...

You'll see that there are a lot of little rules, mostly regarding
the coding style used in GDB, and more generally in GNU projects.
I've mentioned each one of the only once, rather than picking on
them each and every time.

Before you go ahead and fix everything, I think you should wait
to see if others show interest as well. But I thought that the
review would save another more competent maintainer a little
bit of time. So here are my comments....

> It is controlled by two additional MI commands:
> "-start-telnet-service [port]" and "-stop-telnet-service".
[...]
> Current implementation assumes that the inferior is executed in
> async mode and requires "set target-async" to be set to "on".
> Two additional options should also be turned on if remote user is
> going to issue interactive commands (like "commands", etc). They are
> "set confirm on" and "set interactive on" and can be set on-the-fly
> by the remote user.

This is something I wanted to ask when I first heard about the
feature, so these are the operational constraints. OK. I am not
the right maintainer to really look at that, so that'll probably
explain my next question: Are there any potential problems with
concurrency? What if we enter commands from both interfaces at
the same time?

> +# Enable MI telnet.
> +AC_ARG_ENABLE(gdbmitel,
> +AS_HELP_STRING([--enable-gdbmitel], [enable remote interface over telnet]),

Because the telnet interface has to be explicitly enable to be active,
I would imagine that we don't need to provide a configure-time option.

If you do, though, I'd rather it was on by default, rather than
the opposite.  Otherwise, it's going to remain an obscure feature,
and no one is ever going to have it built in when they need it.
And I personally find the name of the option a little obscure.
I'd rather have --enable-gdb-mi-telnet, or --enable-remote-mi-access,
or something like that.

And finally, don't add -DENABLE_SOMETHING to the compilation flags.
Use config.h (generated from config.in by the configure script).

All these issues would go away if we just decide to remove the option
of configuring this feature out.

> Index: b/gdb/mi/mi-telnet.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ b/gdb/mi/mi-telnet.c	2011-11-11 16:20:48.582148706 +0300
> @@ -0,0 +1,426 @@
> +/* MI telnet support
> +
> +   Copyright (C) 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010,
> +   2011 Free Software Foundation, Inc.

I am guessing that the only valid copyright year is 2011.

I am also wondering whether you might be able to use serial.h and
ser-tcp.h instead of using sockets directly.  This would solve
the portability problem, since you don't have BSD sockets on Windows
(we use Winsock instead). I haven't delved in that layer much,
but I think it's worth a look.

> +/*
> +  MI telnet support allows remote user to connect to gdb using telnet.
> +  GDB should be built with "--enable-gdbmitel=yes" option to enable MI telnet.

We typically start the first line of the comment on the same line as
the opening '/*'.

> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include "defs.h"

"defs.h" should always be the first include. Move the fisrt two
includes down.  I'm concerned about including <netinet/in.h>.
For sure you have to conditionalize it with #ifdef HAVE_NETINET_IN_H,
which means the addition of a configure check.

> +#define DEFAULT_PORT 9950
> +#define MAX_CMD_LENGTH 65536
> +
> +/* socket for accepting telnet connections */
> +static int telnet_s = -1;
> +
> +/* client connection descriptor */
> +static int client_fd = -1;
> +
> +/* buffer for received command and for intermediate interactive questions */
> +static char cmd_buf[MAX_CMD_LENGTH];
> +static char cmd_buf_intermediate[MAX_CMD_LENGTH];
> +
> +/* for output redirecting */
> +static struct ui_file *str_file = NULL;

Globals like these are bad form. I understand that it might be necessary
in order to maintain a state of the feature, but at least put them all
in one structure. That way, it looks like we're manipulating *one*
global object instead of a collection of globals. And the day we have
a way to avoid that global, all the date will alreaby be in one structure.

It's also frowned upon to have hard-coded limits like you do
on the cmd_buf & cmd_buf_intermediate. Can't we find a way to
avoid this limitation?

> +/*
> + * Send the reply to remote client
> + */

First of all, THANK YOU for providing comments to the subprograms
you are implementing.  I feel that this is a required part of developing
in GDB, but not everyone follows that rule, or someones everyone follows
it a little differently from the others.

However, function documentation and comments in general are formatted
as follow:

    /* Send the reply to the remote client.
    
       This function does this, and does that, and it is wonderful,
       really.  */

We use full sentences, with an upper-case letter to start, and a period
at the end.  Always two spaces after a period.  And no '*' at the start
of each new line.

> +  if (client_fd != -1 && msg != NULL) {
> +    /* flush additional output if any */

Formatting: The curly brace should be on the next line.  And same
remark about the comment. Thus:

   if (client_fd != -1 && msg != NULL)
     {
       /* Flush additional output if any.  */

> +static const char *const async_commands[] = {
> +  "run",

Same here, curly brace on the next line, please.

> +  /* Is "&" already specified in command? */

Two spaces after the '?'.

-- 
Joel

Re: RFC: Introduce remote console for CLI interpreter via telnet

[Stan Shebs]

On 12/1/11 3:07 PM, Joel Brobecker wrote:
> Grigory,
>
> First, thank you for sending the patch.
>
>> Attached is the patch that introduces telnet service which can
>> accept and execute CLI commands from the remote user.
> No one commented on the desireability of such a feature.
> I'm a bit indifferent, to tell you the truth, and it would
> be nice to hear from others. For now, I've only skimmed over
> the patch...
>

Thanks for the feedback!

This patch originated as a feature request from users of another 
(proprietary) debugger that is used with Eclipse; as I understand it, 
they use it to provide remote access to help someone working with the 
IDE, to do some kinds of scripting-like setup, and to serve as a general 
backdoor fixing up situations where the Eclipse->debugger interface 
messes up.

I suspect that some of these users' needs could probably be handled by 
various Eclipse and/or MI hackery, but as we've nominally had a 
multi-input architecture for some time, it seemed more general, and dare 
I say it, even a little more elegant to simply allow command streams to 
come in from multiple sources.

Stan
stan@codesourcery.com

RE: RFC: Introduce remote console for CLI interpreter via telnet

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Stan Shebs
> Sent: Thursday, December 01, 2011 7:10 PM
> To: gdb-patches@sourceware.org
> Subject: Re: RFC: Introduce remote console for CLI 
> interpreter via telnet
> 
> On 12/1/11 3:07 PM, Joel Brobecker wrote:
> > Grigory,
> >
> > First, thank you for sending the patch.
> >
> >> Attached is the patch that introduces telnet service which can
> >> accept and execute CLI commands from the remote user.
> > No one commented on the desireability of such a feature.
> > I'm a bit indifferent, to tell you the truth, and it would
> > be nice to hear from others. For now, I've only skimmed over
> > the patch...
> >
> 
> Thanks for the feedback!
> 
> This patch originated as a feature request from users of another 
> (proprietary) debugger that is used with Eclipse; as I understand it, 
> they use it to provide remote access to help someone working with the 
> IDE, to do some kinds of scripting-like setup, and to serve 
> as a general 
> backdoor fixing up situations where the Eclipse->debugger interface 
> messes up.

I'm actually hoping to use this feature directly from Eclipse, to
provide a full-fledge GDB-console within Eclipse, with
- prompt
- command completion
- command history
- synchronization with the Eclipse UI

This is something that has been in demand from Eclipse users for
a long time.

I haven't tried the patch just yet, but I'm eager to do so and hope
to have time in the coming weeks.

Thanks!

Marc


> I suspect that some of these users' needs could probably be 
> handled by 
> various Eclipse and/or MI hackery, but as we've nominally had a 
> multi-input architecture for some time, it seemed more 
> general, and dare 
> I say it, even a little more elegant to simply allow command 
> streams to 
> come in from multiple sources.
> 
> Stan
> stan@codesourcery.com
> 
> 

Re: RFC: Introduce remote console for CLI interpreter via telnet

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> I'm actually hoping to use this feature directly from Eclipse, to
Marc> provide a full-fledge GDB-console within Eclipse, with
Marc> - prompt
Marc> - command completion
Marc> - command history

I don't think it will do completion or history.  It doesn't use readline.

It seems trivial to support these from Eclipse though.
You can send plain gdb commands and you can use the complete command to
do completion.

History you can implement in your UI.

I agree about the prompt.

Marc> - synchronization with the Eclipse UI

What does this mean?

Tom

RE: RFC: Introduce remote console for CLI interpreter via telnet

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Friday, December 02, 2011 3:52 PM
> To: Marc Khouzam
> Cc: 'Stan Shebs'; 'gdb-patches@sourceware.org'
> Subject: Re: RFC: Introduce remote console for CLI 
> interpreter via telnet
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> I'm actually hoping to use this feature directly from 
> Eclipse, to
> Marc> provide a full-fledge GDB-console within Eclipse, with
> Marc> - prompt
> Marc> - command completion
> Marc> - command history
> 
> I don't think it will do completion or history.  It doesn't 
> use readline.

That is too bad.  I will have to try it out and figure
out what is missing.

> It seems trivial to support these from Eclipse though.
> You can send plain gdb commands and you can use the complete 
> command to do completion.

Yes, I've been meaning to use the complete command for a while
but never took the time.

> History you can implement in your UI.

Guess so :-(

> I agree about the prompt.

That is good news :-)

> Marc> - synchronization with the Eclipse UI
> 
> What does this mean?

Have Eclipse update properly after commands given
from this new telnet console.  But doing that will
require some GDB events that are not there yet.
But that is the final goal of the Eclispe 
gdb-console.

Marc

Re: RFC: Introduce remote console for CLI interpreter via telnet

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Tom> I don't think it will do completion or history.  It doesn't 
Tom> use readline.

Marc> That is too bad.  I will have to try it out and figure
Marc> out what is missing.

I think it isn't easily possible.  Readline really wants a tty, but this
is just a plain socket connection.

I really think you are better off just implementing a console in your
GUI, and doing the various editing things there.

Tom> History you can implement in your UI.

Marc> Guess so :-(

Actually, I guess interoperating with gdb's history would be nice.  It
would mean that users could "set history save on" and then have their
commands show up in Eclipse as well.

I think this would need some new MI commands.  Specifically, you'd need
an MI request to fetch previous history items.

If you want this, would you mind filing a bug report?

Tom> I agree about the prompt.

Marc> That is good news :-)

Another bug report :)

Marc> - synchronization with the Eclipse UI

Tom> What does this mean?

Marc> Have Eclipse update properly after commands given
Marc> from this new telnet console.  But doing that will
Marc> require some GDB events that are not there yet.
Marc> But that is the final goal of the Eclispe 
Marc> gdb-console.

Ok, I see.  We've added some events in this last release cycle.
Please file bugs for what you need.  These are usually not too hard to
add.

Tom

RE: RFC: Introduce remote console for CLI interpreter via telnet

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Friday, December 02, 2011 4:06 PM
> To: Marc Khouzam
> Cc: 'Stan Shebs'; 'gdb-patches@sourceware.org'
> Subject: Re: RFC: Introduce remote console for CLI 
> interpreter via telnet
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Tom> I don't think it will do completion or history.  It doesn't 
> Tom> use readline.
> 
> Marc> That is too bad.  I will have to try it out and figure
> Marc> out what is missing.
> 
> I think it isn't easily possible.  Readline really wants a 
> tty, but this
> is just a plain socket connection.

Eclipse does provide a pseudo-tty.  In fact, there is also
a full-featured terminal that allows me to access all the
goodies of tcsh for example. 

I guess you are saying that the problem is that this new 
remote console won't allow for the readline features at all.

> I really think you are better off just implementing a console in your
> GUI, and doing the various editing things there.
> 
> Tom> History you can implement in your UI.
> 
> Marc> Guess so :-(
> 
> Actually, I guess interoperating with gdb's history would be nice.  It
> would mean that users could "set history save on" and then have their
> commands show up in Eclipse as well.
> 
> I think this would need some new MI commands.  Specifically, 
> you'd need
> an MI request to fetch previous history items.
> 
> If you want this, would you mind filing a bug report?

I'll try out the patch for the remote console and
write bug reports for suggested improvements.

> Tom> I agree about the prompt.
> 
> Marc> That is good news :-)
> 
> Another bug report :)
> 
> Marc> - synchronization with the Eclipse UI
> 
> Tom> What does this mean?
> 
> Marc> Have Eclipse update properly after commands given
> Marc> from this new telnet console.  But doing that will
> Marc> require some GDB events that are not there yet.
> Marc> But that is the final goal of the Eclispe 
> Marc> gdb-console.
> 
> Ok, I see.  We've added some events in this last release cycle.
> Please file bugs for what you need.  These are usually not too hard to
> add.

Will do.

Thanks a lot!

Marc