Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell.

[Abhijit Halder <abhijit.k.halder@gmail.com>]

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

On Wed, Aug 3, 2011 at 11:16 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> I have made corrections suggested by Sergio Durigan Junior in his
>> code-review. Only the documentation section I am leaving for the time
>> being.
>
> Almost there :-).
>
>> +/* List of characters that can be used as delimiter to separate out
>> +   gdb-command and shell command.  */
>> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>-"
>> +
>> +typedef char *iostream_mode_t;
>> +
>> +#define RD_TEXT "r"
>> +#define WR_TEXT "w"
>
> Based on the discussion with Jan, I thought you would get rid of the `r'
> argument, right?  I'm not sure, so please correct me if I'm wrong.
>
I thought I will keep the -r option available for future use. Just to
resemble with gdb_stdin I created read terminal of pipe. If in future
some command started using gdb_stdin, there will be no coding effort
to make use of that command with pipe.  Please suggest me whether I
should remove these (-r as well as -w) options.

>> +/* Prototype of local function.  */
>
> `functions.'
>
Sorry for this typo! I will correct this.

>> +static struct pipe_t *
>> +construct_pipe (char *p)
>> +{
>> +  struct pipe_t *pipe = NULL;
>> +  int found_mode = 0, pipe_opt_done = 0;
>> +  struct cleanup *old_chain;
>> +
>> +  if (p != NULL && *p != '\0')
>> +    {
>> +      pipe = xmalloc (sizeof (struct pipe_t));
>> +      old_chain = make_cleanup (xfree, pipe);
>> +
>> +      /* Default mode of pipe.  */
>> +      pipe->mode = WR_TEXT;
>> +
>> +      while (!pipe_opt_done)
>> +        {
>> +          p = skip_spaces (p);
>> +
>> +          /* If we don't get an argument started with '-' and which is not
>> +             even a value associated with some option, we consider it as a
>> +             potential delimiter and stop parsing for further option
>> +             arguments.  */
>
> Thanks for fixing the indentation problems.  But I still see one nit:
> according to the GNU Coding Standards, whenever you have 8 spaces, you
> must convert them to a TAB character.  I know it is kind of a pain, but
> we try to follow the convention.  If you use Vim, ping me offlist and I
> can provide you a code that does that.
>
I am still not fully familiar with GNU style. I am sending my .vimrc
to you. It will be a great help if you correct that.

> Also, I see you have an extra space in the end of some lines.  Please
> remove them.
>
>> +            case 'r':
>> +              if (found_mode)
>> +                {
>> +                  error (_("Invalid option"));
>> +                  do_cleanups (old_chain);
>> +                  return NULL;
>> +                }
>
> Some things I'd like to comment about this.
>
> First, I think the error message could be better, explaining what is the
> invalid option.
>
> Second, the `error' routine already calls `do_cleanups', so you don't
> need to call it directly here.
>
> Third, you don't need to return because the `error' routine takes care
> of returning the prompt to the user.
>
>> +      if (pipe->dlim == '\0'
>> +          || strchr (PIPE_DELIMITER, pipe->dlim) == NULL)
>> +        {
>> +          error (_("Invalid delimiter '%c'"), pipe->dlim);
>> +          do_cleanups (old_chain);
>> +          return NULL;
>> +        }
>
> Same comment about `error' and `do_cleanups', here and in the other
> places as well.
>
>> +      if (!pipe->handle)
>> +        {
>> +          error (_("Failed to create pipe.\n%s"), strerror (errno));
>> +          do_cleanups (old_chain);
>> +          return NULL;
>> +        }
>> +    }
>> +
>> +  return pipe;
>> +}
>
> Before returning, you must call `discard_cleanups' if everything went OK.
>
>> +void
>> +_initialize_pipe (void)
>> +{
>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe between gdb and shell for I/O based communication.\n\
>> +Arguments are option(s) to the command, then a delimiter character, \
>> +then the gdb-command and finally the shell-command.\n\
>> +If no option is given, the default behavior of pipe will be to \
>> +pass the gdb-command output to the shell."),
>> +           &cmdlist);
>> +}
>
> Is the user able to use the `-w' argument?  If so, I think you should
> mention is here as well.
>
> I think that is it.  Thank you for your work on this patch.  I would
> appreciate if some maintainer could review it too, in order to catch
> things that I didn't see.
>
> Regards,
>
> Sergio.
>
Please find the corrected patch.

Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command.patch --]
[-- Type: text/x-patch, Size: 7913 bytes --]

diff -rup src/gdb/Makefile.in dst/gdb/Makefile.in
--- src/gdb/Makefile.in	2011-07-27 23:55:26.000000000 +0530
+++ dst/gdb/Makefile.in	2011-07-29 16:12:32.578048797 +0530
@@ -713,7 +713,7 @@ SFILES = ada-exp.y ada-lang.c ada-typepr
 	objc-exp.y objc-lang.c \
 	objfiles.c osabi.c observer.c osdata.c \
 	opencl-lang.c \
-	p-exp.y p-lang.c p-typeprint.c p-valprint.c parse.c printcmd.c \
+	p-exp.y p-lang.c p-typeprint.c p-valprint.c parse.c pipe.c printcmd.c \
 	proc-service.list progspace.c \
 	prologue-value.c psymtab.c \
 	regcache.c reggroups.c remote.c remote-fileio.c reverse.c \
@@ -870,7 +870,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $
 	mi-common.o \
 	event-loop.o event-top.o inf-loop.o completer.o \
 	gdbarch.o arch-utils.o gdbtypes.o osabi.o copying.o \
-	memattr.o mem-break.o target.o parse.o language.o buildsym.o \
+	memattr.o mem-break.o target.o parse.o pipe.o language.o buildsym.o \
 	findcmd.o \
 	std-regs.o \
 	signals.o \
diff -rup src/gdb/pipe.c dst/gdb/pipe.c
--- src/gdb/pipe.c	2011-07-29 15:15:26.078048517 +0530
+++ dst/gdb/pipe.c	2011-08-04 12:51:01.583117045 +0530
@@ -0,0 +1,207 @@
+/* Everything about pipe, for GDB.
+
+   Copyright (C) 2011 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#include "defs.h"
+#include "gdbcmd.h"
+#include <ctype.h>
+#include "gdb_string.h"
+#include "ui-file.h"
+#include "cli/cli-utils.h"
+
+/* List of characters that can be used as delimiter to separate out
+   gdb-command and shell command.  */
+#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>-"
+
+typedef char *iostream_mode_t;
+
+#define RD_TEXT "r"
+#define WR_TEXT "w"
+
+struct pipe_t
+{
+  /* The shell-command.  */
+  char *shell_cmd;
+
+  /* The gdb-command.  */
+  char *gdb_cmd;
+
+  /* The delimiter to separate out gdb-command and shell-command.  */
+  char dlim;
+
+  /* The mode of gdb side pipe (read or write).  */
+  iostream_mode_t mode;
+
+  /* The stream pointer of pipe.  */
+  FILE *handle;
+};
+
+/* Prototype of local functions.  */
+
+static struct pipe_t *construct_pipe (char *);
+
+static void destruct_pipe (struct pipe_t *);
+
+static struct pipe_t *execute_command_to_pipe (struct pipe_t *, int);
+
+static void pipe_command (char *, int);
+
+static struct pipe_t *
+construct_pipe (char *p)
+{
+  struct pipe_t *pipe = NULL;
+  int found_mode = 0, pipe_opt_done = 0;
+  struct cleanup *old_chain;
+
+  if (p != NULL && *p != '\0')
+    {
+      pipe = xmalloc (sizeof (struct pipe_t));
+      old_chain = make_cleanup (xfree, pipe);
+
+      /* Default mode of pipe.  */
+      pipe->mode = WR_TEXT;
+
+      while (!pipe_opt_done)
+	{
+	  p = skip_spaces (p);
+
+	  /* If we don't get an argument started with '-' and which is not
+	     even a value associated with some option, we consider it as a
+	     potential delimiter and stop parsing for further option
+	     arguments.  */
+	  if (*p != '-')
+	    break;
+
+	  switch (*++p)
+	    {
+	    case 'r':
+	      if (found_mode)
+		error (_("Invalid option"));
+	      pipe->mode = RD_TEXT;
+	      found_mode = 1;
+	      ++p;
+	      break;
+
+	    case 'w':
+	      if (found_mode)
+		error (_("Invalid option"));
+	      pipe->mode = WR_TEXT;
+	      found_mode = 1;
+	      ++p;
+	      break;
+
+	    case ' ':
+	      pipe_opt_done = 1;
+	      ++p;
+	      break;
+
+	    default:
+	      error (_("Invalid option"));
+	    }
+	}
+
+      p = skip_spaces (p);
+      pipe->dlim = *p++;
+      p = skip_spaces (p);
+      pipe->gdb_cmd = p;
+
+      /* Validate the delimiter from a pre-defined whitelist characters.
+	 This will enforce not to use special (e.g., alpha-numeric) list
+	 of characters.  */
+      /* NOTE: If DLIM become null, P points to a bad string, hence
+	 before doing further processing of P we should check DLIM.  */
+      if (pipe->dlim == '\0'
+	  || strchr (PIPE_DELIMITER, pipe->dlim) == NULL)
+	error (_("Invalid delimiter '%c'"), pipe->dlim);
+
+      if ((p = strchr (p, pipe->dlim)) == NULL)
+	error (_("Found no shell command"));
+
+      *p++ = '\0';
+      pipe->shell_cmd = p;
+
+      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
+
+      if (!pipe->handle)
+	error (_("Failed to create pipe.\n%s"), strerror (errno));
+
+      discard_cleanups (old_chain);
+    }
+
+  return pipe;
+}
+
+static void
+destruct_pipe (struct pipe_t *pipe)
+{
+  pclose (pipe->handle);
+  xfree (pipe);
+}
+
+static struct pipe_t *
+execute_command_to_pipe (struct pipe_t *pipe, int from_tty)
+{
+  FILE *fstream, *pstream;
+  struct ui_file *gdb_stdio;
+
+  if (!pipe->mode)
+    internal_error (__FILE__, __LINE__,
+		    _("execute_command_to_pipe: un-initialized pipe"));
+  else if (!strcmp (pipe->mode, RD_TEXT))
+    gdb_stdio = gdb_stdin;
+  else if (!strcmp (pipe->mode, WR_TEXT))
+    gdb_stdio = gdb_stdout;
+  else
+    internal_error (__FILE__, __LINE__,
+		    _("execute_command_to_pipe: bad pipe mode"));
+  pstream = pipe->handle;
+  fstream = gdb_modify_io (gdb_stdio, pstream);
+  execute_command (pipe->gdb_cmd, from_tty);
+  pstream = gdb_modify_io (gdb_stdio, fstream);
+  pipe->handle = pstream;
+  return pipe;
+}
+
+static void
+pipe_command (char *arg, int from_tty)
+{
+  struct pipe_t *pipe;
+
+  pipe = construct_pipe (arg);
+  if (pipe != NULL) 
+    {
+      pipe = execute_command_to_pipe (pipe, from_tty);
+      destruct_pipe (pipe);
+    }
+}
+
+void
+_initialize_pipe (void)
+{
+  add_cmd ("pipe", no_class, pipe_command, _("\
+Create pipe between gdb and shell for I/O based communication.\n\
+Arguments are option(s) to the command, then a delimiter character, \
+then the gdb-command and finally the shell-command.\n\
+List of options available:\n\
+  -r    gdb reads output of shell-command from pipe.\n\
+  -w    gdb passes output of a command to shell.\n\
+  -     end of gdb option list.\n\
+If no option is given, the default behavior of pipe will be to \
+pass the gdb-command output to the shell."),
+	   &cmdlist);
+}
diff -rup src/gdb/ui-file.c dst/gdb/ui-file.c
--- src/gdb/ui-file.c	2011-05-14 11:14:36.000000000 +0530
+++ dst/gdb/ui-file.c	2011-08-04 12:17:54.519115933 +0530
@@ -619,6 +619,20 @@ stdio_fileopen (FILE *file)
   return stdio_file_new (file, 0);
 }
 
+FILE *
+gdb_modify_io (struct ui_file *file, FILE *iostream_new)
+{
+  FILE *iostream_old;
+  struct stdio_file *stdio = ui_file_data (file);
+
+  if (stdio->magic != &stdio_file_magic)
+    internal_error (__FILE__, __LINE__,
+		    _("gdb_modify_io: bad magic number"));
+  iostream_old = stdio->file;
+  stdio->file = iostream_new;
+  return iostream_old;
+}
+
 struct ui_file *
 gdb_fopen (char *name, char *mode)
 {
diff -rup src/gdb/ui-file.h dst/gdb/ui-file.h
--- src/gdb/ui-file.h	2011-05-13 22:58:20.000000000 +0530
+++ dst/gdb/ui-file.h	2011-08-02 17:17:54.130442994 +0530
@@ -126,6 +126,9 @@ extern struct ui_file *stdio_fileopen (F
 /* Open NAME returning an STDIO based UI_FILE.  */
 extern struct ui_file *gdb_fopen (char *name, char *mode);
 
+/* Modify the file I/O stream pointer of an STDIO based UI_FILE.  */
+extern FILE *gdb_modify_io (struct ui_file *file, FILE *iostream_new);
+
 /* Create a file which writes to both ONE and TWO.  CLOSE_ONE
    and CLOSE_TWO indicate whether the original files should be
    closed when the new file is closed.  */