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

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

[Abhijit Halder]

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

This is the implementation of a new gdb command, named 'pipe', to make
ease of I/O communication between gdb and shell.
The syntax of this command is shown as follows:
(gdb) pipe [option] <dlim> gdb-cmd <dlim> shell-cmd
List of options go with pipe command:
  -r   gdb reads output of shell-command from pipe
  -w  gdb passes output of a command to shell to process.
  -    end of gdb option list
dlim (delimiter) is a single ASCII character from the set below:
{|/\'"`#@!$%^} (We actually can remove this restriction).
The default behaviour of pipe will be to pass the gdb command output to shell.


 Makefile.in |    4 -
 pipe.c      |  194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pipe.h      |   34 ++++++++++
 ui-file.c   |   14 ++++
 ui-file.h   |    3
 5 files changed, 247 insertions(+), 2 deletions(-)


Thanks,
Abhijit Halder

[-- Attachment #2: ChangeLog --]
[-- Type: application/octet-stream, Size: 336 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@symantec.com>

	Implementation of pipe to make I/O communication 
	between gdb and shell.

	* ui-file.c (gdb_modify_io): New function.
	* ui-file.h (gdb_modify_io): Function prototype.
	* pipe.c: New file.
	* pipe.h: New file.
	* Makefile.in (SFILES): Add pipe.c.
	(COMMON_OBS): Add pipe.o. 

[-- Attachment #3: gdb-pipe-command.patch --]
[-- Type: text/x-patch, Size: 8904 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-07-29 18:16:07.502049125 +0530
@@ -0,0 +1,194 @@
+/* Everything about pipe, for GDB.
+
+   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1999, 2000, 2001, 2002,
+   2003, 2004, 2007, 2008, 2009, 2010, 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 <stdio.h>
+#include <ctype.h>
+#include <string.h>
+#include "cli/cli-utils.h"
+#include "ui-file.h"
+#include "pipe.h"
+
+/* Prototypes for 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;
+
+  if (p != NULL && *p != '\0')
+    {
+      pipe = xmalloc (sizeof(struct pipe_t));
+      pipe->mode = WR_TEXT;
+
+      while (!pipe_opt_done)
+    {
+      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)
+          {
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+          }
+            pipe->mode = RD_TEXT;
+            found_mode = 1;
+            ++p;
+            break;
+
+          case 'w':
+            if (found_mode)
+          {
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+          }
+            pipe->mode = WR_TEXT;
+            found_mode = 1;
+            ++p;
+            break;
+
+          case ' ':
+            pipe_opt_done = 1;
+            ++p;
+            break;
+ 
+          default:
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+        }
+    }
+
+      skip_spaces (p);
+      pipe->dlim = *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->dlim) == NULL)
+    {
+      printf_filtered (_("Invalid delimiter '%c'\n"), pipe->dlim);
+      xfree (pipe);
+      return NULL;
+    }
+
+      if ((p = strchr (p, pipe->dlim)) == NULL)
+    {
+      printf_filtered (_("Found no shell command\n"));
+      xfree (pipe);
+      return NULL;
+    }
+
+      *p++ = '\0';
+      pipe->shell_cmd = p;
+
+      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
+
+      if (!pipe->handle)
+    {
+      internal_error (__FILE__, __LINE__,
+		      _("construct_pipe: failed to create pipe.\n%s"), 
+              strerror (errno));
+
+      xfree (pipe);
+      return NULL;
+    }
+    }
+
+  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."), 
+    &cmdlist);
+}
diff -rup src/gdb/pipe.h dst/gdb/pipe.h
--- src/gdb/pipe.h	2011-07-29 15:15:32.466049126 +0530
+++ dst/gdb/pipe.h	2011-07-29 14:34:02.330049110 +0530
@@ -0,0 +1,34 @@
+/* Data structures associated with pipe in GDB.
+   Copyright (C) 1986, 1989, 1990, 1991, 1992, 1993, 1994, 1998, 1999, 2000,
+   2002, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#if !defined (PIPE_H)
+#define PIPE_H 1
+
+typedef char *iostream_mode_t;
+
+#define RD_TEXT "r"
+#define WR_TEXT "w"
+
+struct pipe_t {
+  char *shell_cmd;
+  char *gdb_cmd;
+  char dlim;
+  iostream_mode_t mode;
+  FILE *handle;
+};
+
+#endif /* !defined (PIPE_H) */
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-07-29 14:32:07.838049413 +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-07-29 14:31:38.074047122 +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.  */
+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.  */

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

[Abhijit Halder]

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

On Fri, Jul 29, 2011 at 6:45 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> This is the implementation of a new gdb command, named 'pipe', to make
> ease of I/O communication between gdb and shell.
> The syntax of this command is shown as follows:
> (gdb) pipe [option] <dlim> gdb-cmd <dlim> shell-cmd
> List of options go with pipe command:
>  -r   gdb reads output of shell-command from pipe
>  -w  gdb passes output of a command to shell to process.
>  -    end of gdb option list
> dlim (delimiter) is a single ASCII character from the set below:
> {|/\'"`#@!$%^} (We actually can remove this restriction).
> The default behaviour of pipe will be to pass the gdb command output to shell.
>
>
>  Makefile.in |    4 -
>  pipe.c      |  194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  pipe.h      |   34 ++++++++++
>  ui-file.c   |   14 ++++
>  ui-file.h   |    3
>  5 files changed, 247 insertions(+), 2 deletions(-)
>
>
> Thanks,
> Abhijit Halder
>

A last minute regression happened because of wrong use skip_spaces.
Re-submitting the corrected patch.

 Makefile.in |    4 -
 pipe.c      |  194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pipe.h      |   34 ++++++++++
 ui-file.c   |   14 ++++
 ui-file.h   |    3
 5 files changed, 247 insertions(+), 2 deletions(-)


Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command-corrected.patch --]
[-- Type: text/x-patch, Size: 8890 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-07-29 18:16:07.502049125 +0530
@@ -0,0 +1,194 @@
+/* Everything about pipe, for GDB.
+
+   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1999, 2000, 2001, 2002,
+   2003, 2004, 2007, 2008, 2009, 2010, 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 <stdio.h>
+#include <ctype.h>
+#include <string.h>
+#include "ui-file.h"
+#include "pipe.h"
+
+/* Prototypes for 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;
+
+  if (p != NULL && *p != '\0')
+    {
+      pipe = xmalloc (sizeof(struct pipe_t));
+      pipe->mode = WR_TEXT;
+
+      while (!pipe_opt_done)
+    {
+      while (isspace (*p))
+        ++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)
+          {
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+          }
+            pipe->mode = RD_TEXT;
+            found_mode = 1;
+            ++p;
+            break;
+
+          case 'w':
+            if (found_mode)
+          {
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+          }
+            pipe->mode = WR_TEXT;
+            found_mode = 1;
+            ++p;
+            break;
+
+          case ' ':
+            pipe_opt_done = 1;
+            ++p;
+            break;
+ 
+          default:
+            printf_filtered (_("Invalid option\n"));
+            xfree (pipe);
+            return NULL;
+        }
+    }
+
+      while (isspace (*p))
+        ++p;
+
+      pipe->dlim = *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->dlim) == NULL)
+    {
+      printf_filtered (_("Invalid delimiter '%c'\n"), pipe->dlim);
+      xfree (pipe);
+      return NULL;
+    }
+
+      if ((p = strchr (p, pipe->dlim)) == NULL)
+    {
+      printf_filtered (_("Found no shell command\n"));
+      xfree (pipe);
+      return NULL;
+    }
+
+      *p++ = '\0';
+      pipe->shell_cmd = p;
+
+      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
+
+      if (!pipe->handle)
+    {
+      internal_error (__FILE__, __LINE__,
+		      _("construct_pipe: failed to create pipe.\n%s"), 
+              strerror (errno));
+
+      xfree (pipe);
+      return NULL;
+    }
+    }
+
+  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."), 
+    &cmdlist);
+}
diff -rup src/gdb/pipe.h dst/gdb/pipe.h
--- src/gdb/pipe.h	2011-07-29 15:15:32.466049126 +0530
+++ dst/gdb/pipe.h	2011-07-29 14:34:02.330049110 +0530
@@ -0,0 +1,34 @@
+/* Data structures associated with pipe in GDB.
+   Copyright (C) 1986, 1989, 1990, 1991, 1992, 1993, 1994, 1998, 1999, 2000,
+   2002, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
+
+   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/>.  */
+
+#if !defined (PIPE_H)
+#define PIPE_H 1
+
+typedef char *iostream_mode_t;
+
+#define RD_TEXT "r"
+#define WR_TEXT "w"
+
+struct pipe_t {
+  char *shell_cmd;
+  char *gdb_cmd;
+  char dlim;
+  iostream_mode_t mode;
+  FILE *handle;
+};
+
+#endif /* !defined (PIPE_H) */
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-07-29 14:32:07.838049413 +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-07-29 14:31:38.074047122 +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.  */
+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.  */

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 336 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@symantec.com>

	Implementation of pipe to make I/O communication 
	between gdb and shell.

	* ui-file.c (gdb_modify_io): New function.
	* ui-file.h (gdb_modify_io): Function prototype.
	* pipe.c: New file.
	* pipe.h: New file.
	* Makefile.in (SFILES): Add pipe.c.
	(COMMON_OBS): Add pipe.o. 

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

[Sergio Durigan Junior]

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> On Fri, Jul 29, 2011 at 6:45 PM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
>> This is the implementation of a new gdb command, named 'pipe', to make
>> ease of I/O communication between gdb and shell.
>> The syntax of this command is shown as follows:
>> (gdb) pipe [option] <dlim> gdb-cmd <dlim> shell-cmd
>> List of options go with pipe command:
>>  -r   gdb reads output of shell-command from pipe
>>  -w  gdb passes output of a command to shell to process.
>>  -    end of gdb option list
>> dlim (delimiter) is a single ASCII character from the set below:
>> {|/\'"`#@!$%^} (We actually can remove this restriction).
>> The default behaviour of pipe will be to pass the gdb command output to shell.

Thanks.  Your patch has a bunch of issues, specially bad indentation,
lack of comments, and other things related to the coding standards.  I
strongly suggest you take a look at the GNU Coding Standards manual
(link below).

Also, you will need to write documentation for it, add an entry to the
NEWS file, and possible add a testcase.  I added other comments below.

> A last minute regression happened because of wrong use skip_spaces.

This is because `skip_spaces' returns the pointer to the string with
spaces skipped.  You should use it like this:

  p = skip_spaces (p);

> 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-07-29 18:16:07.502049125 +0530
> @@ -0,0 +1,194 @@
> +/* Everything about pipe, for GDB.
> +
> +   Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997, 1999, 2000, 2001, 2002,
> +   2003, 2004, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.

Since this is a new file, the copyright notice should mention only this
year (2011).

> +#include "defs.h"
> +#include "gdbcmd.h"
> +#include <stdio.h>

<stdio.h> is already included by "defs.h".

> +#include <ctype.h>
> +#include <string.h>

Others can correct me, but I think you should include "gdb_string.h"
instead of <string.h>.

> +#include "ui-file.h"
> +#include "pipe.h"
> +
> +/* Prototypes for local functions */

Period in the end of the sentence.

> +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);

Add a comment for each function, and spaces between them, like this:

    /* Comment for foo.  */
    static int foo (int i);

    /* Comment for bar.  */
    static int bar (int j);

> +static struct pipe_t *
> +construct_pipe (char *p)
> +{
> +  struct pipe_t *pipe = NULL;
> +  int found_mode = 0, pipe_opt_done = 0;
> +
> +  if (p != NULL && *p != '\0')
> +    {
> +      pipe = xmalloc (sizeof(struct pipe_t));

Spaces between `sizeof' and parenthesis.

> +      while (!pipe_opt_done)
> +    {

Braces misplaced here (and in a lot of other places).

Take a look at the GNU Coding Standards:

     http://www.gnu.org/prep/standards/

If you are using Emacs, you get that for free.  If you are using Vim,
there are some settings you can do in order to proper edit your files.

> +      while (isspace (*p))
> +        ++p;

You can use `skip_spaces' here (and in the other places as well).

> +
> +      /* 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.  */

These lines are too small.  The convention is to break the line when it
reaches 80 chars (some people use 72 chars as a soft limit).

> +      switch (*++p)
> +        {
> +          case 'r':
> +            if (found_mode)
> +          {

Misplaced braces (not only here).

> +            printf_filtered (_("Invalid option\n"));
> +            xfree (pipe);
> +            return NULL;

I think the convention in such cases is to register a cleanup to xfree
`pipe', and to call `error' instead of use `printf_filtered'.

> +      while (isspace (*p))
> +        ++p;

`skip_spaces'.

> +      pipe->dlim = *p++;
> +      pipe->gdb_cmd = p;
> +
> +      /* Validate the delimiter from a pre-defined
> +         whitelist characters. This will enforce 

Sorry about the nit-picking.  There must be a space between
`characters.' and `This'.

> +         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->dlim) == NULL)

The `||' must be in the second line.  Maybe you could create a #define
for this delimiter sequence?

#define PIPE_DELIMITERS "..."

The `if' must be:

    if (*pipe->dlim
        || strchr (PIPE_DELIMITERS, pipe->dlim) == NULL)

> +    {
> +      printf_filtered (_("Invalid delimiter '%c'\n"), pipe->dlim);
> +      xfree (pipe);
> +      return NULL;
> +    }

Same comment regarding calling `error' and making a cleanup of `pipe'.

> +      *p++ = '\0';
> +      pipe->shell_cmd = p;
> +
> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
> +
> +      if (!pipe->handle)
> +    {
> +      internal_error (__FILE__, __LINE__,
> +		      _("construct_pipe: failed to create pipe.\n%s"), 
> +              strerror (errno));

The indentation is wrong here.  Also, I don't think this can be treated
as an internal error.  IIUC, if the pipe creation has failed, GDB has
done nothing wrong.  It is probably a system error or something.  I
think you should just call `error' and be done with it.

> +void
> +_initialize_pipe (void)
> +{
> +  add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe between gdb and shell for I/O based communication."), 
> +    &cmdlist);

There is no way for the user to know what are the arguments taken by the
command.  I suggest you mention them here.

> diff -rup src/gdb/pipe.h dst/gdb/pipe.h
> --- src/gdb/pipe.h	2011-07-29 15:15:32.466049126 +0530
> +++ dst/gdb/pipe.h	2011-07-29 14:34:02.330049110 +0530
> @@ -0,0 +1,34 @@
> +/* Data structures associated with pipe in GDB.
> +   Copyright (C) 1986, 1989, 1990, 1991, 1992, 1993, 1994, 1998, 1999, 2000,
> +   2002, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
> +
> +   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/>.  */
> +
> +#if !defined (PIPE_H)
> +#define PIPE_H 1
> +
> +typedef char *iostream_mode_t;
> +
> +#define RD_TEXT "r"
> +#define WR_TEXT "w"
> +
> +struct pipe_t {
> +  char *shell_cmd;
> +  char *gdb_cmd;
> +  char dlim;
> +  iostream_mode_t mode;
> +  FILE *handle;
> +};

The convention is to add a comment for each field in the struct, and add
blank lines between them.  Something like:

  struct foo
    {
      /* Comment for bar.  */
      int bar;

      /* Comment for baz.  */
      char baz;
    };

> +#endif /* !defined (PIPE_H) */

As far as I have seen, you are only using `struct pipe_t' inside
pipe.c.  Thus, I don't think you need to create pipe.h because you are
not exporting anything externally.  I'd rather keep the definition of
`struct pipe' (along with `RD_TEXT' et al) inside pipe.c, and don't
create pipe.h.

> 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-07-29 14:31:38.074047122 +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.  */
> +FILE *gdb_modify_io (struct ui_file *file, FILE *iostream_new);

We add the `extern' keyword before the function declaration.

Thanks,

Sergio.

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

[Jan Kratochvil]

On Fri, 29 Jul 2011 15:35:10 +0200, Abhijit Halder wrote:
On Fri, Jul 29, 2011 at 6:45 PM, Abhijit Halder <abhijit.k.halder@gmail.com> wrote:
> This is the implementation of a new gdb command, named 'pipe', to make
> ease of I/O communication between gdb and shell.
> The syntax of this command is shown as follows:
> (gdb) pipe [option] <dlim> gdb-cmd <dlim> shell-cmd
> List of options go with pipe command:
>  -r   gdb reads output of shell-command from pipe
>  -w  gdb passes output of a command to shell to process.

When is useful to feed input of a GDB command from some file/process?
It is IMO useful only for `run' but that will not work this way.


>  -    end of gdb option list
> dlim (delimiter) is a single ASCII character from the set below:
> {|/\'"`#@!$%^} (We actually can remove this restriction).

I would prefer first to agree on the proper syntax.  I do not find

(gdb) pipe | print 1 | less

to be something a new user will ever try whether it does not work.

The original idea of:

(gdb) print 1 | less

looks fine to me as in normal cases one never uses `|' in the GDB commands.
Still for example for the `echo' GDB command one may use `|' as both the
`echo' argument and also one may want to pass the output to some shell.

If there is pipe `|' GDB could support also redirections '>', '>>', '2>&1',
'&>', '|&' besides '|' etc.

There could be some disable of the option being enabled by default:

(gdb) set shell-metachars off


Thanks,
Jan

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

[Abhijit Halder]

On Tue, Aug 2, 2011 at 12:24 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 29 Jul 2011 15:35:10 +0200, Abhijit Halder wrote:
> On Fri, Jul 29, 2011 at 6:45 PM, Abhijit Halder <abhijit.k.halder@gmail.com> wrote:
>> This is the implementation of a new gdb command, named 'pipe', to make
>> ease of I/O communication between gdb and shell.
>> The syntax of this command is shown as follows:
>> (gdb) pipe [option] <dlim> gdb-cmd <dlim> shell-cmd
>> List of options go with pipe command:
>>  -r   gdb reads output of shell-command from pipe
>>  -w  gdb passes output of a command to shell to process.
>
> When is useful to feed input of a GDB command from some file/process?
> It is IMO useful only for `run' but that will not work this way.
>
Yes. I made this option (-r) available just to make the code
futuristic. To feed input to `run' command, we still need to make
changes in the `run' command interface itself.
>
>>  -    end of gdb option list
>> dlim (delimiter) is a single ASCII character from the set below:
>> {|/\'"`#@!$%^} (We actually can remove this restriction).
>
> I would prefer first to agree on the proper syntax.  I do not find
>
> (gdb) pipe | print 1 | less
>
> to be something a new user will ever try whether it does not work.
>
Can't we introduce a new command and make the user educated to use
that (using documentation and all)? Introducing a new command instead
of using some meta-character (like '|') will give us more flexibility
and that will be ease of maintenance. At the same time this will be
more futuristic.
Pedro Alves mentioned in a similar line: "I think I like this better
than comming up with some syntax that we need to make sure works not
just with C, but all supported languages.  (and hope no future
supported language adds a conflict)." I also do think that the
introduction of a new command will be a better approach.
> The original idea of:
>
> (gdb) print 1 | less
>
> looks fine to me as in normal cases one never uses `|' in the GDB commands.
> Still for example for the `echo' GDB command one may use `|' as both the
> `echo' argument and also one may want to pass the output to some shell.
>
We can still support inline pipe (print 1 | less) along with pipe
command. But the question of maintainability will appear.
> If there is pipe `|' GDB could support also redirections '>', '>>', '2>&1',
> '&>', '|&' besides '|' etc.
>
The same can still be very much possible with pipe command in place. For e.g.
(gdb) pipe | thread apply all bt | grep "foobar" 2>&1 | tee myLog.txt

> There could be some disable of the option being enabled by default:
>
> (gdb) set shell-metachars off
>
>
> Thanks,
> Jan
>

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

[Jan Kratochvil]

On Tue, 02 Aug 2011 10:52:27 +0200, Abhijit Halder wrote:
> On Tue, Aug 2, 2011 at 12:24 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> Yes. I made this option (-r) available just to make the code
> futuristic.

It is nice to have the syntax ready for future extensions but one does not
have to code the extensions when they still have no use.


> >>  -    end of gdb option list
> >> dlim (delimiter) is a single ASCII character from the set below:
> >> {|/\'"`#@!$%^} (We actually can remove this restriction).
> >
> > I would prefer first to agree on the proper syntax.  I do not find
> >
> > (gdb) pipe | print 1 | less
> >
> > to be something a new user will ever try whether it does not work.

I missed the very first thread named "PATCH" which already discussed this part
so I am fine now with the "pipe" command.


> > If there is pipe `|' GDB could support also redirections '>', '>>', '2>&1',
> > '&>', '|&' besides '|' etc.
> >
> The same can still be very much possible with pipe command in place. For e.g.
> (gdb) pipe | thread apply all bt | grep "foobar" 2>&1 | tee myLog.txt

I was referring to for example
(gdb) pipe print variable >somefile

GDB also prints its errors to gdb_stderr, therefore you may need:

(gdb) pipe print nonexistingvariable |& tee file

if you want to redirect also the error message of the GDB "print" command.

But it all means a single delimiter would not be enough.  Still one can
backslash the syntax:

(gdb) pipe print 1 \| 2 | tee file-will-contain-3

(gdb) echo <<<hello>>>\n
(gdb) pipe echo \<\<\<hello\>\>\>\\n >file

Sure maybe "pipe" is no longer the right name and "redirect" or so matches the
functionality better.


This is just a proposal.  I see I jumped late to the thread.  Still I would
prefer some final syntax agreement.


Thanks,
Jan

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

[Sergio Durigan Junior]

Jan Kratochvil <jan.kratochvil@redhat.com> writes:

> Sure maybe "pipe" is no longer the right name and "redirect" or so matches the
> functionality better.

FWIW I think that, given the current status of the patch, `redirect' is
better than `pipe', indeed.

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

[Abhijit Halder]

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

On Wed, Aug 3, 2011 at 5:09 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
>
>> Sure maybe "pipe" is no longer the right name and "redirect" or so matches the
>> functionality better.
>
> FWIW I think that, given the current status of the patch, `redirect' is
> better than `pipe', indeed.
>

I have made corrections suggested by Sergio Durigan Junior in his
code-review. Only the documentation section I am leaving for the time
being.

 Makefile.in |    4 -
 pipe.c      |  223 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-file.c   |   14 +++
 ui-file.h   |    3
 4 files changed, 242 insertions(+), 2 deletions(-)


Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command.patch --]
[-- Type: text/x-patch, Size: 8575 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-03 12:08:59.529850285 +0530
@@ -0,0 +1,223 @@
+/* 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 function.  */
+
+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"));
+                  do_cleanups (old_chain);
+                  return NULL;
+                }
+              pipe->mode = RD_TEXT;
+              found_mode = 1;
+              ++p;
+              break;
+
+            case 'w':
+              if (found_mode)
+                {
+                  error (_("Invalid option"));
+                  do_cleanups (old_chain);
+                  return NULL;
+                }
+              pipe->mode = WR_TEXT;
+              found_mode = 1;
+              ++p;
+              break;
+
+            case ' ':
+              pipe_opt_done = 1;
+              ++p;
+              break;
+
+            default:
+              error (_("Invalid option"));
+              do_cleanups (old_chain);
+              return NULL;
+            }
+        }
+
+      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);
+          do_cleanups (old_chain);
+          return NULL;
+        }
+
+      if ((p = strchr (p, pipe->dlim)) == NULL)
+        {
+          error (_("Found no shell command"));
+          do_cleanups (old_chain);
+          return NULL;
+        }
+
+      *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));
+          do_cleanups (old_chain);
+          return NULL;
+        }
+    }
+
+  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\
+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-07-29 14:32:07.838049413 +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.  */

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 315 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@symantec.com>

	Implementation of pipe to make I/O communication 
	between gdb and shell.

	* ui-file.c (gdb_modify_io): New function.
	* ui-file.h (gdb_modify_io): Function prototype.
	* pipe.c: New file.
	* Makefile.in (SFILES): Add pipe.c.
	(COMMON_OBS): Add pipe.o. 

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

[Sergio Durigan Junior]

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.

> +/* Prototype of local function.  */

`functions.'

> +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.

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.

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

[Abhijit Halder]

[-- 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.  */

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

[Jan Kratochvil]

On Thu, 04 Aug 2011 09:51:14 +0200, Abhijit Halder wrote:
> If in future some command started using gdb_stdin,

then s/he can Google it out from your mailing list post.  The GDB codebase
should be as simple as possible for easier contributions.

I really do not see any (normal) use for gdb_stdin in the GDB codebase now.


> Please suggest me whether I should remove these (-r as well as -w) options.

Yes.


> I am still not fully familiar with GNU style.

For us non-Emacsists
	http://www.gnu.org/prep/standards/standards.html
suggests
	indent -nbad -bap -nbc -bbo -bl -bli2 -bls -ncdb -nce -cp1 -cs -di2 -ndj -nfc1 -nfca -hnl -i2 -ip5 -lp -pcs -psl -nsc -nsob


I am sorry you did not comment on the idea of '>' / '>>' etc. redirections,
I found find it also useful myself.


Thanks,
Jan

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

[Pedro Alves]

On Thursday 04 August 2011 08:51:14, Abhijit Halder wrote:
> +  fstream = gdb_modify_io (gdb_stdio, pstream);
> +  execute_command (pipe->gdb_cmd, from_tty);
> +  pstream = gdb_modify_io (gdb_stdio, fstream);

Looks like this leaves gdb_stdio in an inconsistent
state if execute_command throws an error.

Do you really need the new gdb_modify_io function?
Can't ui_out_redirect (and stdio_file_new perhaps) do the job?

> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);

I'm not sure that'll build on all supported hosts.  
I think on Windows that may require use of _popen instead.

"struct pipe_t" sounds like a recipe for system namespace 
colision (and _t is reserved for posix, though we
have some precedent for abusing it), and is easily confused
with the ser*.c pipe machinery.  Can you find an
alternative name for the struct please?
Perhaps struct pipe_cmd_state.

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

I haven't really read this thread yet, but I wanted to chime in...

>> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);

Pedro> I'm not sure that'll build on all supported hosts.  
Pedro> I think on Windows that may require use of _popen instead.

libiberty has the already-portable 'pexecute' code for this kind of
thing.

Tom

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

[Abhijit Halder]

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

On Thu, Aug 4, 2011 at 2:59 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Thursday 04 August 2011 08:51:14, Abhijit Halder wrote:
>> +  fstream = gdb_modify_io (gdb_stdio, pstream);
>> +  execute_command (pipe->gdb_cmd, from_tty);
>> +  pstream = gdb_modify_io (gdb_stdio, fstream);
>
> Looks like this leaves gdb_stdio in an inconsistent
> state if execute_command throws an error.
>
> Do you really need the new gdb_modify_io function?
> Can't ui_out_redirect (and stdio_file_new perhaps) do the job?
>
Yes you are right. I could not foresee this.

>> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
>
> I'm not sure that'll build on all supported hosts.
> I think on Windows that may require use of _popen instead.
>
> "struct pipe_t" sounds like a recipe for system namespace
> colision (and _t is reserved for posix, though we
> have some precedent for abusing it), and is easily confused
> with the ser*.c pipe machinery.  Can you find an
> alternative name for the struct please?
> Perhaps struct pipe_cmd_state.
>
> --
> Pedro Alves
>

I have made the suggested corrections. Please review this.

Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command.patch --]
[-- Type: text/x-patch, Size: 5647 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-05 13:10:51.411046880 +0530
@@ -0,0 +1,161 @@
+/* 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 <ctype.h>
+#include "gdb_string.h"
+#include "ui-file.h"
+#include "ui-out.h"
+#include "cli/cli-utils.h"
+#include "gdbcmd.h"
+
+/* List of characters that can be used as delimiter to separate out
+   gdb-command and shell command.  */
+#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"
+
+/* The mode of stream operation.  */
+typedef char *iostream_mode_t;
+
+/* At present we support only write mode of operations to the pipe, i.e.,
+   gdb-command can only write to the pipe whose other terminal is owned by the
+   shell. In future we may start supporting read mode of operations as well.
+   But at present there is no need for that.  */
+#define WR_TEXT "w"
+
+struct pipe_object
+{
+  /* 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 supported mode of stream operations on gdb-end pipe.  */
+  iostream_mode_t mode;
+
+  /* The gdb-end stream pointer to the pipe.  */
+  FILE *handle;
+};
+
+/* Prototype of local functions.  */
+
+static struct pipe_object *construct_pipe (char *);
+
+static void execute_command_to_pipe (struct pipe_object *, int);
+
+static void destruct_pipe (struct pipe_object *);
+
+static void pipe_command (char *, int);
+
+static struct pipe_object *
+construct_pipe (char *p)
+{
+  struct pipe_object *pipe = NULL;
+  struct cleanup *old_chain;
+
+  if (p != NULL && *p != '\0')
+    {
+      pipe = xmalloc (sizeof (struct pipe_object));
+      old_chain = make_cleanup (xfree, pipe);
+      pipe->mode = WR_TEXT;
+
+      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) characters.  */
+      /* NOTE: If DLIM become null, P starts pointing to a bad memory
+	 location, 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
+execute_command_to_pipe (struct pipe_object *pipe, int from_tty)
+{
+  struct cleanup *cleanup;
+  struct ui_file *fp;
+
+  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
+  fp = stdio_fileopen (pipe->handle);
+  make_cleanup_ui_file_delete (fp);
+  make_cleanup_restore_ui_file (&gdb_stdout);
+
+  if (ui_out_redirect (uiout, fp) < 0)
+    warning (_("Current output protocol does not support redirection"));
+  else 
+    make_cleanup_ui_out_redirect_pop (uiout);
+
+  gdb_stdout = fp;
+  execute_command (pipe->gdb_cmd, from_tty);
+  do_cleanups (cleanup);
+}
+
+static void
+destruct_pipe (struct pipe_object *pipe)
+{
+  pclose (pipe->handle);
+  xfree (pipe);
+}
+
+static void
+pipe_command (char *arg, int from_tty)
+{
+  struct pipe_object *pipe;
+
+  pipe = construct_pipe (arg);
+  if (pipe != NULL)
+    {
+      execute_command_to_pipe (pipe, from_tty);
+      destruct_pipe (pipe);
+    }
+}
+
+void
+_initialize_pipe (void)
+{
+  add_cmd ("pipe", no_class, pipe_command, _("\
+Create pipe to pass gdb-command output to the shell for processing.\n\
+Arguments are a delimiter character, followed by a gdb-command, \
+followed by a shell-command."),
+	   &cmdlist);
+}

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 221 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@symantec.com>

	Implementation of pipe to make I/O communication 
	between gdb and shell.

	* pipe.c: New file.
	* Makefile.in (SFILES): Add pipe.c.
	(COMMON_OBS): Add pipe.o. 

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

[Jan Kratochvil]

On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote:
> --- src/gdb/pipe.c	2011-07-29 15:15:26.078048517 +0530
> +++ dst/gdb/pipe.c	2011-08-05 13:10:51.411046880 +0530
> @@ -0,0 +1,161 @@
> +/* 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 <ctype.h>
> +#include "gdb_string.h"
> +#include "ui-file.h"
> +#include "ui-out.h"
> +#include "cli/cli-utils.h"
> +#include "gdbcmd.h"
> +
> +/* List of characters that can be used as delimiter to separate out
> +   gdb-command and shell command.  */
> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"

I see you are not fond of redirections but at least one now will be able to:
	(gdb) pipe | command | cat >file

Why there should be such explicitly limited delimiter choice?  Couldn't the
pipe command default to '|' and otherwise take an arbitrary first word?  
(The word should never start with '-' to have the options extension possibility
in the future.)  That is to permit:
	(gdb) pipe info threads | less
	(gdb) pipe : print 1 | 2 : less
	(gdb) pipe FOO print 1 | 2 FOO less
	etc.


> +
> +/* The mode of stream operation.  */
> +typedef char *iostream_mode_t;
> +
> +/* At present we support only write mode of operations to the pipe, i.e.,
> +   gdb-command can only write to the pipe whose other terminal is owned by the
> +   shell. In future we may start supporting read mode of operations as well.
> +   But at present there is no need for that.  */
> +#define WR_TEXT "w"
> +
> +struct pipe_object

Missing struct comment.

> +{
> +  /* 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 supported mode of stream operations on gdb-end pipe.  */
> +  iostream_mode_t mode;

It is not redundant for the current code.

> +
> +  /* The gdb-end stream pointer to the pipe.  */
> +  FILE *handle;
> +};
> +
> +/* Prototype of local functions.  */
> +
> +static struct pipe_object *construct_pipe (char *);
> +
> +static void execute_command_to_pipe (struct pipe_object *, int);
> +
> +static void destruct_pipe (struct pipe_object *);
> +
> +static void pipe_command (char *, int);

All these prototypes are redundant as the functions definitions precedes first
use.

> +
> +static struct pipe_object *
> +construct_pipe (char *p)

Missing function comment.

> +{
> +  struct pipe_object *pipe = NULL;
> +  struct cleanup *old_chain;
> +
> +  if (p != NULL && *p != '\0')

P can never be NULL, and if it could be it should be rather
	gdb_assert (p != NULL);
That *p != '\0' I also do not understand, it should print an error message in
such case IMO.


> +    {
> +      pipe = xmalloc (sizeof (struct pipe_object));
> +      old_chain = make_cleanup (xfree, pipe);
> +      pipe->mode = WR_TEXT;
> +
> +      p = skip_spaces (p);
> +      pipe->dlim = *p++;

This can skip to unallocated memory if *p == 0.

> +      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) characters.  */
> +      /* NOTE: If DLIM become null, P starts pointing to a bad memory
> +	 location, 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);

There was already a review note `pexecute' should be used instead.


> +
> +      if (!pipe->handle)
> +	error (_("Failed to create pipe.\n%s"), strerror (errno));

safe_strerror.  Also see the error messages in GDB, it is normally not printed
on a new line, that is:
	error (_("Failed to create pipe: %s"), safe_strerror (errno));


> +
> +      discard_cleanups (old_chain);
> +    }
> +
> +  return pipe;
> +}
> +
> +static void
> +execute_command_to_pipe (struct pipe_object *pipe, int from_tty)

Missing function comment.

> +{
> +  struct cleanup *cleanup;
> +  struct ui_file *fp;
> +
> +  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
> +  fp = stdio_fileopen (pipe->handle);
> +  make_cleanup_ui_file_delete (fp);
> +  make_cleanup_restore_ui_file (&gdb_stdout);
> +
> +  if (ui_out_redirect (uiout, fp) < 0)
> +    warning (_("Current output protocol does not support redirection"));
> +  else 
> +    make_cleanup_ui_out_redirect_pop (uiout);
> +
> +  gdb_stdout = fp;

I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and
gdb_stdtargerr like set_logging_redirect does.  This is like if one
uses |& instead of | .  I understand someone may have a different opinion.
In such case there even may be some option of the command pipe for it.


> +  execute_command (pipe->gdb_cmd, from_tty);
> +  do_cleanups (cleanup);
> +}
> +
> +static void
> +destruct_pipe (struct pipe_object *pipe)

Missing function comment.

> +{
> +  pclose (pipe->handle);

It could report if the command terminates wrongly - exit status - of the
command.


> +  xfree (pipe);
> +}
> +
> +static void
> +pipe_command (char *arg, int from_tty)

Missing function comment.

> +{
> +  struct pipe_object *pipe;
> +
> +  pipe = construct_pipe (arg);
> +  if (pipe != NULL)
> +    {
> +      execute_command_to_pipe (pipe, from_tty);
> +      destruct_pipe (pipe);

execute_command_to_pipe can terminate prematurely, as execute_command can call
error(), `pipe' here would get leaked, destruct_pipe should be called from a
cleanup function.

> +    }
> +}
> +
> +void
> +_initialize_pipe (void)

There should be an extra declaration of _initialize_pipe as some compilers can
complain otherwise (GCC does not, I do not know which do myself).

> +{
> +  add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe to pass gdb-command output to the shell for processing.\n\
> +Arguments are a delimiter character, followed by a gdb-command, \
> +followed by a shell-command."),
> +	   &cmdlist);
> +}


There should be documentation in gdb/doc/gdb.texinfo for it.


Thanks,
Jan

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

[Abhijit Halder]

On Thu, Aug 4, 2011 at 2:12 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 04 Aug 2011 09:51:14 +0200, Abhijit Halder wrote:
>> If in future some command started using gdb_stdin,
>
> then s/he can Google it out from your mailing list post.  The GDB codebase
> should be as simple as possible for easier contributions.
>
> I really do not see any (normal) use for gdb_stdin in the GDB codebase now.
>
>
>> Please suggest me whether I should remove these (-r as well as -w) options.
>
> Yes.
>
>
>> I am still not fully familiar with GNU style.
>
> For us non-Emacsists
>        http://www.gnu.org/prep/standards/standards.html
> suggests
>        indent -nbad -bap -nbc -bbo -bl -bli2 -bls -ncdb -nce -cp1 -cs -di2 -ndj -nfc1 -nfca -hnl -i2 -ip5 -lp -pcs -psl -nsc -nsob
>
>
> I am sorry you did not comment on the idea of '>' / '>>' etc. redirections,
> I found find it also useful myself.
>
Sorry for this delay. I am not sure about the use-case of redirection.
If someone wants to redirect some specific command output to a file,
s/he can anyway do it using pipe and some useful shell command (e.g.
cat).
These two will be equivalent:
1. (gdb) print variable >somefile
2. (gdb) pipe | print variable | cat somefile
But it will not dump any error message to the file. User may not be
interested in the errors coming from gdb. That is the reason I
purposefully skipped the gdb_stderr to be redirected to pipe. But that
is again doable task.

>
> Thanks,
> Jan
>

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

[Pedro Alves]

On Friday 05 August 2011 09:29:47, Jan Kratochvil wrote:
> Why there should be such explicitly limited delimiter choice?  Couldn't the
> pipe command default to '|' and otherwise take an arbitrary first word?  
> (The word should never start with '-' to have the options extension possibility
> in the future.)  That is to permit:
>         (gdb) pipe info threads | less
>         (gdb) pipe : print 1 | 2 : less
>         (gdb) pipe FOO print 1 | 2 FOO less
>         etc.

That way you can't tell if FOO is a token or a random gdb
command, unless you forbit tokens that are command names.
I had suggested:

"We can tweak the syntax a bit to support
options to the pipe command.  E.g, define that
the pipe command always ends with a lone '-' before
the gdb command chain, and -t for pipe token, then:

No -t switch, assume `|' pipe token:

 (gdb) pipe - bt | vim -

Specify alternate pipe token:

 (gdb) pipe -t PIPE - p 1 | 2 PIPE vim -
"

We can get by without -t too, but we'd need
the `-', I think:

 (gdb) pipe - bt | vim -
 (gdb) pipe FOO - bt FOO vim -

But at this point, if we always need to spell out
`-', we might as well decide that you always
end the pipe command with the splitting token, that
is, the default of `|' doesn't really buy much:

 (gdb) pipe - bt | vim -
 (gdb) pipe FOO - bt FOO vim -
 (gdb) pipe BAR - bt BAR vim -

vs

 (gdb) pipe | bt | vim -
 (gdb) pipe FOO bt FOO vim -
 (gdb) pipe BAR bt BAR vim -

which brings us back to the current proposed
syntax, except for the split token not being limited to
some set of fixed characters.

-- 
Pedro Alves

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

[Jan Kratochvil]

On Fri, 05 Aug 2011 10:52:16 +0200, Pedro Alves wrote:
> On Friday 05 August 2011 09:29:47, Jan Kratochvil wrote:
> >         (gdb) pipe info threads | less
> >         (gdb) pipe : print 1 | 2 : less
> >         (gdb) pipe FOO print 1 | 2 FOO less
> >         etc.
> 
> That way you can't tell if FOO is a token or a random gdb
> command, unless you forbit tokens that are command names.

Oops, I noticed only after sending it...


> I had suggested:
> 
> "We can tweak the syntax a bit to support
> options to the pipe command.  E.g, define that
> the pipe command always ends with a lone '-' before

One point is it should be `--' instead of `-' to be GNU getopt-like compliant.

I am still not fond of any mandatory character there if in 99% of uses cases
one can just default to `|'.  `pipe' is there for convenience, one can already
get reach the same functionality with some defined canned command sequence
with `set logging'.


> which brings us back to the current proposed
> syntax, except for the split token not being limited to
> some set of fixed characters.

To make the most common use case with `|' the most simple to type:
	(gdb) pipe print 1 | less

one could consider the first argument a delimiter if it is a single-character
and ispunct().

	(gdb) pipe print 1 | less
	(gdb) pipe : print 1 | 2 : less

If one really needs to enter ispunct() single-character command one still can:

	(gdb) pipe | + plus-args

if + is a valid GDB command.


Thanks,
Jan

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote:
>> --- src/gdb/pipe.c    2011-07-29 15:15:26.078048517 +0530
>> +++ dst/gdb/pipe.c    2011-08-05 13:10:51.411046880 +0530
>> @@ -0,0 +1,161 @@
>> +/* 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 <ctype.h>
>> +#include "gdb_string.h"
>> +#include "ui-file.h"
>> +#include "ui-out.h"
>> +#include "cli/cli-utils.h"
>> +#include "gdbcmd.h"
>> +
>> +/* List of characters that can be used as delimiter to separate out
>> +   gdb-command and shell command.  */
>> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"
>
> I see you are not fond of redirections but at least one now will be able to:
>        (gdb) pipe | command | cat >file
>
> Why there should be such explicitly limited delimiter choice?  Couldn't the
> pipe command default to '|' and otherwise take an arbitrary first word?
> (The word should never start with '-' to have the options extension possibility
> in the future.)  That is to permit:
>        (gdb) pipe info threads | less
>        (gdb) pipe : print 1 | 2 : less
>        (gdb) pipe FOO print 1 | 2 FOO less
>        etc.
>
I am not sure whether this restriction is meaningful. Ideally we
should not support any alpha-numeric character as a delimiter just
because of readability purpose. e.g.
(gdb) pipe dthread apply all btdvim -
Here d is delimiter. I don't think the above one is acceptable. Please
suggest me if we simply can put a restriction of not using any
alpha-numeric character as delimiter and that will do.
Secondly I believe by FOO you meant a single character and not a
string. Between delimiter and command there is no restriction of
having any white-space.
>
>> +
>> +/* The mode of stream operation.  */
>> +typedef char *iostream_mode_t;
>> +
>> +/* At present we support only write mode of operations to the pipe, i.e.,
>> +   gdb-command can only write to the pipe whose other terminal is owned by the
>> +   shell. In future we may start supporting read mode of operations as well.
>> +   But at present there is no need for that.  */
>> +#define WR_TEXT "w"
>> +
>> +struct pipe_object
>
> Missing struct comment.
>
I thought the comment on individual fields reveal enough information
to a developer. In existing code also I have seen similar practice in
several places. Please suggest me whether I need to put some valuable
comment here.
>> +{
>> +  /* 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 supported mode of stream operations on gdb-end pipe.  */
>> +  iostream_mode_t mode;
>
> It is not redundant for the current code.
>
Yes it is. I thought it is just a better encapsulation. In future if
we are going to support any new mode (e.g. read mode) the mode will
come as an option. Then it will be a good design to keep that
information inside this structure.
>> +
>> +  /* The gdb-end stream pointer to the pipe.  */
>> +  FILE *handle;
>> +};
>> +
>> +/* Prototype of local functions.  */
>> +
>> +static struct pipe_object *construct_pipe (char *);
>> +
>> +static void execute_command_to_pipe (struct pipe_object *, int);
>> +
>> +static void destruct_pipe (struct pipe_object *);
>> +
>> +static void pipe_command (char *, int);
>
> All these prototypes are redundant as the functions definitions precedes first
> use.
Just a good practice.
>
>> +
>> +static struct pipe_object *
>> +construct_pipe (char *p)
>
> Missing function comment.
>
From function name hope things are revealed. Please suggest if
comments are really useful.
>> +{
>> +  struct pipe_object *pipe = NULL;
>> +  struct cleanup *old_chain;
>> +
>> +  if (p != NULL && *p != '\0')
>
> P can never be NULL, and if it could be it should be rather
>        gdb_assert (p != NULL);
Yes I agree.
> That *p != '\0' I also do not understand, it should print an error message in
> such case IMO.
In the following case *p will be '\0'.
(gdb)pipe ||echo "Do nothing"
This can be consider not an error. (Please suggest)
Again, if so, we should not go ahead (going ahead will not break
anything, just incur extra overhead in memory allocation and
de-allocation)
>
>
>> +    {
>> +      pipe = xmalloc (sizeof (struct pipe_object));
>> +      old_chain = make_cleanup (xfree, pipe);
>> +      pipe->mode = WR_TEXT;
>> +
>> +      p = skip_spaces (p);
>> +      pipe->dlim = *p++;
>
> This can skip to unallocated memory if *p == 0.
Control will never come here under such condition.
>
>> +      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) characters.  */
>> +      /* NOTE: If DLIM become null, P starts pointing to a bad memory
>> +      location, 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);
>
> There was already a review note `pexecute' should be used instead.
Sorry I missed that. I will change it.
>
>
>> +
>> +      if (!pipe->handle)
>> +     error (_("Failed to create pipe.\n%s"), strerror (errno));
>
> safe_strerror.  Also see the error messages in GDB, it is normally not printed
> on a new line, that is:
>        error (_("Failed to create pipe: %s"), safe_strerror (errno));
>
I will make this change.
>
>> +
>> +      discard_cleanups (old_chain);
>> +    }
>> +
>> +  return pipe;
>> +}
>> +
>> +static void
>> +execute_command_to_pipe (struct pipe_object *pipe, int from_tty)
>
> Missing function comment.
>
>> +{
>> +  struct cleanup *cleanup;
>> +  struct ui_file *fp;
>> +
>> +  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
>> +  fp = stdio_fileopen (pipe->handle);
>> +  make_cleanup_ui_file_delete (fp);
>> +  make_cleanup_restore_ui_file (&gdb_stdout);
>> +
>> +  if (ui_out_redirect (uiout, fp) < 0)
>> +    warning (_("Current output protocol does not support redirection"));
>> +  else
>> +    make_cleanup_ui_out_redirect_pop (uiout);
>> +
>> +  gdb_stdout = fp;
>
> I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and
> gdb_stdtargerr like set_logging_redirect does.  This is like if one
> uses |& instead of | .  I understand someone may have a different opinion.
> In such case there even may be some option of the command pipe for it.
I understand your point here, but this will break out very basic
assumption that delimiter is single character.
Can we skip this at this point? Or simply we can redirect gdb_stderror
to pipe as in most of the cases that will be equivalent to gdb_stdout.
(My understanding was that the error from gdb can be captured anyway
by error logging mechanism, the use-case of using pipe, I believe, is
when the gdb output is huge and one wants to process that huge
output.) Please suggest.
>
>
>> +  execute_command (pipe->gdb_cmd, from_tty);
>> +  do_cleanups (cleanup);
>> +}
>> +
>> +static void
>> +destruct_pipe (struct pipe_object *pipe)
>
> Missing function comment.
>
>> +{
>> +  pclose (pipe->handle);
Similar to _popen there is _pclose for windows platform. Can I safely
use pclose here?
>
> It could report if the command terminates wrongly - exit status - of the
> command.
>
>
>> +  xfree (pipe);
>> +}
>> +
>> +static void
>> +pipe_command (char *arg, int from_tty)
>
> Missing function comment.
>
>> +{
>> +  struct pipe_object *pipe;
>> +
>> +  pipe = construct_pipe (arg);
>> +  if (pipe != NULL)
>> +    {
>> +      execute_command_to_pipe (pipe, from_tty);
>> +      destruct_pipe (pipe);
>
> execute_command_to_pipe can terminate prematurely, as execute_command can call
> error(), `pipe' here would get leaked, destruct_pipe should be called from a
> cleanup function.
Yes. I will make this change.
>
>> +    }
>> +}
>> +
>> +void
>> +_initialize_pipe (void)
>
> There should be an extra declaration of _initialize_pipe as some compilers can
> complain otherwise (GCC does not, I do not know which do myself).
I believe that is there in framework. init.c, created at runtime, does
contain this. Please correct me if I am wrong.
>
>> +{
>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe to pass gdb-command output to the shell for processing.\n\
>> +Arguments are a delimiter character, followed by a gdb-command, \
>> +followed by a shell-command."),
>> +        &cmdlist);
>> +}
>
>
> There should be documentation in gdb/doc/gdb.texinfo for it.
Sure, I am presently working on it.
>
>
> Thanks,
> Jan
>

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

[Jan Kratochvil]

On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote:
> On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
> > On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote:
> >> --- src/gdb/pipe.c    2011-07-29 15:15:26.078048517 +0530
> >> +++ dst/gdb/pipe.c    2011-08-05 13:10:51.411046880 +0530
> >> @@ -0,0 +1,161 @@
> >> +/* 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 <ctype.h>
> >> +#include "gdb_string.h"
> >> +#include "ui-file.h"
> >> +#include "ui-out.h"
> >> +#include "cli/cli-utils.h"
> >> +#include "gdbcmd.h"
> >> +
> >> +/* List of characters that can be used as delimiter to separate out
> >> +   gdb-command and shell command.  */
> >> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"
> >
> > I see you are not fond of redirections but at least one now will be able to:
> >        (gdb) pipe | command | cat >file
> >
> > Why there should be such explicitly limited delimiter choice?  Couldn't the
> > pipe command default to '|' and otherwise take an arbitrary first word?
> > (The word should never start with '-' to have the options extension possibility
> > in the future.)  That is to permit:
> >        (gdb) pipe info threads | less
> >        (gdb) pipe : print 1 | 2 : less
> >        (gdb) pipe FOO print 1 | 2 FOO less
> >        etc.
> >
> I am not sure whether this restriction is meaningful. Ideally we
> should not support any alpha-numeric character as a delimiter just
> because of readability purpose. e.g.
> (gdb) pipe dthread apply all btdvim -
> Here d is delimiter. I don't think the above one is acceptable. Please
> suggest me if we simply can put a restriction of not using any
> alpha-numeric character as delimiter and that will do.
> Secondly I believe by FOO you meant a single character and not a
> string. Between delimiter and command there is no restriction of
> having any white-space.
> >
> >> +
> >> +/* The mode of stream operation.  */
> >> +typedef char *iostream_mode_t;
> >> +
> >> +/* At present we support only write mode of operations to the pipe, i.e.,
> >> +   gdb-command can only write to the pipe whose other terminal is owned by the
> >> +   shell. In future we may start supporting read mode of operations as well.
> >> +   But at present there is no need for that.  */
> >> +#define WR_TEXT "w"
> >> +
> >> +struct pipe_object
> >
> > Missing struct comment.
> >
> I thought the comment on individual fields reveal enough information
> to a developer. In existing code also I have seen similar practice in
> several places. Please suggest me whether I need to put some valuable
> comment here.
> >> +{
> >> +  /* 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 supported mode of stream operations on gdb-end pipe.  */
> >> +  iostream_mode_t mode;
> >
> > It is not redundant for the current code.

It was a typo: It IS redundant for the current code.


> >
> Yes it is. I thought it is just a better encapsulation. In future if
> we are going to support any new mode (e.g. read mode) the mode will
> come as an option. Then it will be a good design to keep that
> information inside this structure.

We do not try to invent a new style.  We just try to follow the GDB style.
It took me some years to find out what the GDB style is.


> >> +/* Prototype of local functions.  */
> >> +
> >> +static struct pipe_object *construct_pipe (char *);
> >> +
> >> +static void execute_command_to_pipe (struct pipe_object *, int);
> >> +
> >> +static void destruct_pipe (struct pipe_object *);
> >> +
> >> +static void pipe_command (char *, int);
> >
> > All these prototypes are redundant as the functions definitions precedes first
> > use.
> Just a good practice.

Just follow the GDB style.


> >
> >> +
> >> +static struct pipe_object *
> >> +construct_pipe (char *p)
> >
> > Missing function comment.
> >
> >From function name hope things are revealed. Please suggest if
> comments are really useful.

Just follow the GDB style (of new patches; there exists a lot of old code not
having comments at the function entry).


[ rest skipped now ]



Thanks,
Jan

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

[Pedro Alves]

On Friday 05 August 2011 10:41:09, Abhijit Halder wrote:
> I am not sure whether this restriction is meaningful. Ideally we
> should not support any alpha-numeric character as a delimiter just
> because of readability purpose. e.g.
> (gdb) pipe dthread apply all btdvim -
> Here d is delimiter. I don't think the above one is acceptable. Please
> suggest me if we simply can put a restriction of not using any
> alpha-numeric character as delimiter and that will do.
> Secondly I believe by FOO you meant a single character and not a
> string. Between delimiter and command there is no restriction of
> having any white-space.

Speaking for myself, when I wrote PIPE and FOO before, I really
meant a string (with no whitespace), not a single character.

In your example above:

 (gdb) pipe dthread apply all btdvim -

the delimiter would be `dthread'.  I see no reason to
require it to be a single character.

-- 
Pedro Alves

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 3:15 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote:
>> On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil
>> <jan.kratochvil@redhat.com> wrote:
>> > On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote:
>> >> --- src/gdb/pipe.c    2011-07-29 15:15:26.078048517 +0530
>> >> +++ dst/gdb/pipe.c    2011-08-05 13:10:51.411046880 +0530
>> >> @@ -0,0 +1,161 @@
>> >> +/* 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 <ctype.h>
>> >> +#include "gdb_string.h"
>> >> +#include "ui-file.h"
>> >> +#include "ui-out.h"
>> >> +#include "cli/cli-utils.h"
>> >> +#include "gdbcmd.h"
>> >> +
>> >> +/* List of characters that can be used as delimiter to separate out
>> >> +   gdb-command and shell command.  */
>> >> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"
>> >
>> > I see you are not fond of redirections but at least one now will be able to:
>> >        (gdb) pipe | command | cat >file
>> >
>> > Why there should be such explicitly limited delimiter choice?  Couldn't the
>> > pipe command default to '|' and otherwise take an arbitrary first word?
>> > (The word should never start with '-' to have the options extension possibility
>> > in the future.)  That is to permit:
>> >        (gdb) pipe info threads | less
>> >        (gdb) pipe : print 1 | 2 : less
>> >        (gdb) pipe FOO print 1 | 2 FOO less
>> >        etc.
>> >
>> I am not sure whether this restriction is meaningful. Ideally we
>> should not support any alpha-numeric character as a delimiter just
>> because of readability purpose. e.g.
>> (gdb) pipe dthread apply all btdvim -
>> Here d is delimiter. I don't think the above one is acceptable. Please
>> suggest me if we simply can put a restriction of not using any
>> alpha-numeric character as delimiter and that will do.
>> Secondly I believe by FOO you meant a single character and not a
>> string. Between delimiter and command there is no restriction of
>> having any white-space.
>> >
>> >> +
>> >> +/* The mode of stream operation.  */
>> >> +typedef char *iostream_mode_t;
>> >> +
>> >> +/* At present we support only write mode of operations to the pipe, i.e.,
>> >> +   gdb-command can only write to the pipe whose other terminal is owned by the
>> >> +   shell. In future we may start supporting read mode of operations as well.
>> >> +   But at present there is no need for that.  */
>> >> +#define WR_TEXT "w"
>> >> +
>> >> +struct pipe_object
>> >
>> > Missing struct comment.
>> >
>> I thought the comment on individual fields reveal enough information
>> to a developer. In existing code also I have seen similar practice in
>> several places. Please suggest me whether I need to put some valuable
>> comment here.
>> >> +{
>> >> +  /* 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 supported mode of stream operations on gdb-end pipe.  */
>> >> +  iostream_mode_t mode;
>> >
>> > It is not redundant for the current code.
>
> It was a typo: It IS redundant for the current code.
>
>
>> >
>> Yes it is. I thought it is just a better encapsulation. In future if
>> we are going to support any new mode (e.g. read mode) the mode will
>> come as an option. Then it will be a good design to keep that
>> information inside this structure.
>
> We do not try to invent a new style.  We just try to follow the GDB style.
> It took me some years to find out what the GDB style is.
>
>
>> >> +/* Prototype of local functions.  */
>> >> +
>> >> +static struct pipe_object *construct_pipe (char *);
>> >> +
>> >> +static void execute_command_to_pipe (struct pipe_object *, int);
>> >> +
>> >> +static void destruct_pipe (struct pipe_object *);
>> >> +
>> >> +static void pipe_command (char *, int);
>> >
>> > All these prototypes are redundant as the functions definitions precedes first
>> > use.
>> Just a good practice.
>
> Just follow the GDB style.
>
>
>> >
>> >> +
>> >> +static struct pipe_object *
>> >> +construct_pipe (char *p)
>> >
>> > Missing function comment.
>> >
>> >From function name hope things are revealed. Please suggest if
>> comments are really useful.
>
> Just follow the GDB style (of new patches; there exists a lot of old code not
> having comments at the function entry).

Sure, I will make the suggested changes. Since I am very new, I was
blindly referring to some existing files. Hope I will able to learn
things fast.
>
>
> [ rest skipped now ]
>
>
>
> Thanks,
> Jan
>

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 3:26 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 05 August 2011 10:41:09, Abhijit Halder wrote:
>> I am not sure whether this restriction is meaningful. Ideally we
>> should not support any alpha-numeric character as a delimiter just
>> because of readability purpose. e.g.
>> (gdb) pipe dthread apply all btdvim -
>> Here d is delimiter. I don't think the above one is acceptable. Please
>> suggest me if we simply can put a restriction of not using any
>> alpha-numeric character as delimiter and that will do.
>> Secondly I believe by FOO you meant a single character and not a
>> string. Between delimiter and command there is no restriction of
>> having any white-space.
>
> Speaking for myself, when I wrote PIPE and FOO before, I really
> meant a string (with no whitespace), not a single character.
>
> In your example above:
>
>  (gdb) pipe dthread apply all btdvim -
>
> the delimiter would be `dthread'.  I see no reason to
> require it to be a single character.
>
Just a thought. Can't using a single character make the life simple?
That will surely reduce the options for delimiter. But as you said,
like sed this can be used.
> --
> Pedro Alves
>

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 3:54 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Fri, Aug 5, 2011 at 3:26 PM, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Friday 05 August 2011 10:41:09, Abhijit Halder wrote:
>>> I am not sure whether this restriction is meaningful. Ideally we
>>> should not support any alpha-numeric character as a delimiter just
>>> because of readability purpose. e.g.
>>> (gdb) pipe dthread apply all btdvim -
>>> Here d is delimiter. I don't think the above one is acceptable. Please
>>> suggest me if we simply can put a restriction of not using any
>>> alpha-numeric character as delimiter and that will do.
>>> Secondly I believe by FOO you meant a single character and not a
>>> string. Between delimiter and command there is no restriction of
>>> having any white-space.
>>
>> Speaking for myself, when I wrote PIPE and FOO before, I really
>> meant a string (with no whitespace), not a single character.
>>
>> In your example above:
>>
>>  (gdb) pipe dthread apply all btdvim -
>>
>> the delimiter would be `dthread'.  I see no reason to
>> require it to be a single character.
>>
> Just a thought. Can't using a single character make the life simple?
Just a bit clarification on my statement. If we use some string as
delimiter, space has be used again as a delimiter between between two
tokens. If someone use simply a single character as a delimiter, he
has to provide a space between delimiters and commands.
(gdb) pipe |
> That will surely reduce the options for delimiter. But as you said,
> like sed this can be used.
>> --
>> Pedro Alves
>>
>

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

[Pedro Alves]

On Friday 05 August 2011 11:24:54, Abhijit Halder wrote:

> > the delimiter would be `dthread'.  I see no reason to
> > require it to be a single character.
> >
> Just a thought. Can't using a single character make the life simple?

To who?  I imagine myself stumbling on some expression in some
language that conflicts with |, and without much thinking,
just writing:

 pipe XXX randomexpr XXX randomcommand

instead of having to think, or possibly try/fail more than one
time, which characters are allowed and wouldn't conflict with
both randomexpr and randomcommand.

> That will surely reduce the options for delimiter. But as you said,
> like sed this can be used.

Well, I said like a grandchild of sed and cat EOF, the latter
refering to "here documents", which accept strings as
delimiters.  :-)  Sorry for not being clear.

-- 
Pedro Alves

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 4:06 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 05 August 2011 11:24:54, Abhijit Halder wrote:
>
>> > the delimiter would be `dthread'.  I see no reason to
>> > require it to be a single character.
>> >
>> Just a thought. Can't using a single character make the life simple?
>
> To who?  I imagine myself stumbling on some expression in some
> language that conflicts with |, and without much thinking,
> just writing:
>
>  pipe XXX randomexpr XXX randomcommand
>
> instead of having to think, or possibly try/fail more than one
> time, which characters are allowed and wouldn't conflict with
> both randomexpr and randomcommand.
>
Here I got your point. Thanks.
>> That will surely reduce the options for delimiter. But as you said,
>> like sed this can be used.
>
> Well, I said like a grandchild of sed and cat EOF, the latter
> refering to "here documents", which accept strings as
> delimiters.  :-)  Sorry for not being clear.
>
> --
> Pedro Alves
>

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

[Abhijit Halder]

Has pexecute deprecated by any chance? There is a comment in the code
advising not to use this function in new code. Further, pexecute does
not provide similar functionality. It just executes a program and
returns the status of the program, not provide the text output from
the program it executes. Is it a good idea to write a function (say,
gdb_popen) as below:

FILE *
gdb_popen (const char *cmd, const char *mode)
{
#ifdef _WIN32
return _popen (cmd, mode);
#else
return popen (cmd, mode);
}

On Thu, Aug 4, 2011 at 7:51 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
>
> I haven't really read this thread yet, but I wanted to chime in...
>
>>> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
>
> Pedro> I'm not sure that'll build on all supported hosts.
> Pedro> I think on Windows that may require use of _popen instead.
>
> libiberty has the already-portable 'pexecute' code for this kind of
> thing.
>
> Tom
>

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 8:35 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> Has pexecute deprecated by any chance? There is a comment in the code
> advising not to use this function in new code. Further, pexecute does
> not provide similar functionality. It just executes a program and
> returns the status of the program, not provide the text output from
> the program it executes. Is it a good idea to write a function (say,
> gdb_popen) as below:
>
> FILE *
> gdb_popen (const char *cmd, const char *mode)
> {
> #ifdef _WIN32
> return _popen (cmd, mode);
> #else
> return popen (cmd, mode);
> }
>
Sorry for top posting.

> On Thu, Aug 4, 2011 at 7:51 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
>>
>> I haven't really read this thread yet, but I wanted to chime in...
>>
>>>> +      pipe->handle = popen (pipe->shell_cmd, pipe->mode);
>>
>> Pedro> I'm not sure that'll build on all supported hosts.
>> Pedro> I think on Windows that may require use of _popen instead.
>>
>> libiberty has the already-portable 'pexecute' code for this kind of
>> thing.
>>
Has pexecute deprecated by any chance? There is a comment in the code
advising not to use this function in new code. Further, pexecute does
not provide similar functionality. It just executes a program and
returns the status of the program, not provide the text output from
the program it executes. Is it a good idea to write a function (say,
gdb_popen) as below:

FILE *
gdb_popen (const char *cmd, const char *mode)
{
#ifdef _WIN32
return _popen (cmd, mode);
#else
return popen (cmd, mode);
}

>> Tom
>>
>

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

[Eli Zaretskii]

> Date: Fri, 5 Aug 2011 20:35:40 +0530
> From: Abhijit Halder <abhijit.k.halder@gmail.com>
> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org, 	Sergio Durigan Junior <sergiodj@redhat.com>, Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Further, pexecute does not provide similar functionality. It just
> executes a program and returns the status of the program, not
> provide the text output from the program it executes.

??? Did you read the documentation (on libiberty/pexecute.txh)?
pexecute certainly does more than that!

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

[Abhijit Halder]

On Fri, Aug 5, 2011 at 8:42 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Fri, 5 Aug 2011 20:35:40 +0530
>> From: Abhijit Halder <abhijit.k.halder@gmail.com>
>> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org,         Sergio Durigan Junior <sergiodj@redhat.com>, Jan Kratochvil <jan.kratochvil@redhat.com>
>>
>> Further, pexecute does not provide similar functionality. It just
>> executes a program and returns the status of the program, not
>> provide the text output from the program it executes.
>
> ??? Did you read the documentation (on libiberty/pexecute.txh)?
> pexecute certainly does more than that!
Thanks for the reference. Yes I have found the one I was looking for.
>

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

[Abhijit Halder]

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

On Sat, Aug 6, 2011 at 2:41 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Fri, Aug 5, 2011 at 8:42 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> Date: Fri, 5 Aug 2011 20:35:40 +0530
>>> From: Abhijit Halder <abhijit.k.halder@gmail.com>
>>> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org,         Sergio Durigan Junior <sergiodj@redhat.com>, Jan Kratochvil <jan.kratochvil@redhat.com>
>>>
>>> Further, pexecute does not provide similar functionality. It just
>>> executes a program and returns the status of the program, not
>>> provide the text output from the program it executes.
>>
>> ??? Did you read the documentation (on libiberty/pexecute.txh)?
>> pexecute certainly does more than that!
> Thanks for the reference. Yes I have found the one I was looking for.
>>
>
I have changed the earlier implementation of using popen, instead now
I am using libiberty. I have also incorporated other suggestions
during code review. Please review this new patch.

Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command-corrected.patch --]
[-- Type: text/x-patch, Size: 6211 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-06 15:05:02.816346435 +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 <ctype.h>
+#include "gdb_string.h"
+#include "ui-file.h"
+#include "ui-out.h"
+#include "cli/cli-utils.h"
+#include "gdbcmd.h"
+#include "libiberty.h"
+
+/* Structure to encapsulate all entities associated with pipe.  */
+
+struct pipe_obj
+{
+  /* The delimiter to separate out gdb-command and shell-command. This can be
+     any arbitrary string without containing any whitespace.  */
+  char *dlim;
+
+  /* The gdb-command.  */
+  char *gdb_cmd;
+
+  /* The shell-command.  */
+  char *shell_cmd;
+
+  /* The gdb-side stream pointer to the pipe.  */
+  FILE *handle;
+
+  /* The pex object use to create pipeline between gdb and shell.  */
+  struct pex_obj *pex;
+};
+
+/* Construct a pipe object by parsing argument to the pipe command.  */
+
+static struct pipe_obj *
+construct_pipe (char *p)
+{
+  char *separator;
+  struct pipe_obj *pipe = NULL;
+  struct cleanup *cleanup;
+
+  if (p == NULL)
+    error (_("No argument is specified"));
+
+  pipe = XCNEW (struct pipe_obj);
+  cleanup = make_cleanup (xfree, pipe);
+
+  pipe->dlim = p;
+
+  p = skip_to_space (p);
+  if (*p != '\0') *p++ = '\0';
+
+  p = skip_spaces (p);
+  pipe->gdb_cmd = p;
+
+  if (*pipe->gdb_cmd == '\0')
+    error (_("No gdb-command is specified"));
+
+  for (pipe->shell_cmd = "";
+       (separator = skip_to_space (p)) != NULL;
+       p = skip_spaces (separator))
+    {
+      int mismatch = memcmp (p, pipe->dlim, (separator-p));
+
+      if (!mismatch)
+	{
+	  *p = '\0';
+	  pipe->shell_cmd = skip_spaces (separator);
+	  break;
+	}
+    }
+
+  if (*pipe->shell_cmd == '\0')
+    error (_("No shell-command is specified"));
+
+  discard_cleanups (cleanup);
+
+  return pipe;
+}
+
+/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not
+   display it to the screen.  */
+
+static void
+execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
+{
+  char **argv;
+  struct cleanup *cleanup;
+  struct ui_file *fp;
+
+  argv = gdb_buildargv (pipe->shell_cmd);
+  cleanup = make_cleanup_freeargv (argv);
+
+  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
+
+  if (pipe->pex == NULL)
+    do_cleanups (cleanup);
+
+  pipe->handle = pex_input_pipe (pipe->pex, 0);
+
+  if (pipe->handle == NULL)
+    error (_("Failed to create pipe"));
+
+    {
+      int status;
+      const char *err
+       = pex_run (pipe->pex,
+		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
+		  argv[0], argv,
+		  NULL, NULL,
+		  &status);
+      if (err)
+	error (_("Failed to execute %s"), argv[0]);
+
+      do_cleanups (cleanup);
+    }
+
+  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
+  fp = stdio_fileopen (pipe->handle);
+  make_cleanup_ui_file_delete (fp);
+  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);
+
+  if (ui_out_redirect (uiout, fp) < 0)
+    warning (_("Current output protocol does not support redirection"));
+  else 
+    make_cleanup_ui_out_redirect_pop (uiout);
+
+  gdb_stdout = fp;
+  gdb_stderr = fp;
+  gdb_stdlog = fp;
+  gdb_stdtarg = fp;
+  gdb_stdtargerr = fp;
+  execute_command (pipe->gdb_cmd, from_tty);
+  do_cleanups (cleanup);
+}
+
+/* Destruct pipe object.  */
+
+static void
+destruct_pipe (void *arg)
+{
+  struct pipe_obj *pipe = (struct pipe_obj *)arg;
+
+  if (pipe->handle)
+    fclose (pipe->handle);
+
+  if (pipe->pex)
+    {
+      int status;
+
+      pex_get_status (pipe->pex, 1, &status);
+      pex_free (pipe->pex);
+    }
+
+  xfree (pipe);
+}
+
+/* Execute the pipe command.  */
+
+static void
+pipe_command (char *arg, int from_tty)
+{
+  struct pipe_obj *pipe = construct_pipe (arg);
+
+  if (pipe != NULL)
+    {
+      struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
+
+      execute_command_to_pipe (pipe, from_tty);
+      do_cleanups (cleanup);
+    }
+}
+
+/* Module initialization.  */
+
+void
+_initialize_pipe (void)
+{
+  add_cmd ("pipe", no_class, pipe_command, _("\
+Create pipe to pass gdb-command output to the shell for processing.\n\
+Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
+again and finally a shell-command."),
+	   &cmdlist);
+}

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

[Sergio Durigan Junior]

Hi Abhijit,

Some minor nits (again).

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> +struct pipe_obj
> +{
> +  /* The delimiter to separate out gdb-command and shell-command. This can be
                                                                    ^^

Two spaces after the period.

> +  /* The pex object use to create pipeline between gdb and shell.  */

Some typos:

"/*The pex object used to create a pipeline between GDB and shell.  */"

> +  if (*p != '\0') *p++ = '\0';

I'm not found of this style.  Please, put the assignment on the next
line:

        if (*p != '\0')
          *p++ = '\0';


> +  for (pipe->shell_cmd = "";

Sorry, I didn't understand this line.  Is this needed?  If so, I think
it's better if you do `pipe->shell_cmd = NULL'.

> +      int mismatch = memcmp (p, pipe->dlim, (separator-p));

Space between `separator', `-' and `p'.  If you want, you can put this
`memcmp' call directly on the `if' condition, and then you wouldn't need
the braces on the `for'.

> +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not
                                             ^^
Two spaces after period.

> +  if (pipe->handle == NULL)
> +    error (_("Failed to create pipe"));
> +
> +    {
> +      int status;
> +      const char *err
> +       = pex_run (pipe->pex,
> +		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
> +		  argv[0], argv,
> +		  NULL, NULL,
> +		  &status);
> +      if (err)
> +	error (_("Failed to execute %s"), argv[0]);
> +
> +      do_cleanups (cleanup);
> +    }

Why the braces?

> +  if (pipe->pex)
> +    {
> +      int status;
> +
> +      pex_get_status (pipe->pex, 1, &status);
> +      pex_free (pipe->pex);

Do you really need to call `pex_get_status' here?  I'm really asking,
because I don't know about pex.

Thanks for your work on this patch.

Regards,

Sergio.

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

[Abhijit Halder]

On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Hi Abhijit,
>
> Some minor nits (again).
>
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> +struct pipe_obj
>> +{
>> +  /* The delimiter to separate out gdb-command and shell-command. This can be
>                                                                    ^^
Sorry, here I am not clear. Do you mean to say I should remove - in
between gdb and command?
>
> Two spaces after the period.
>
>> +  /* The pex object use to create pipeline between gdb and shell.  */
>
Here I guess I have put 2 space character between . (period) and */.
> Some typos:
>
> "/*The pex object used to create a pipeline between GDB and shell.  */"
>
>> +  if (*p != '\0') *p++ = '\0';
>
> I'm not found of this style.  Please, put the assignment on the next
> line:
Yes a slip. I am modifying it.
>
>        if (*p != '\0')
>          *p++ = '\0';
>
>
>> +  for (pipe->shell_cmd = "";
>
> Sorry, I didn't understand this line.  Is this needed?  If so, I think
> it's better if you do `pipe->shell_cmd = NULL'.
>
>> +      int mismatch = memcmp (p, pipe->dlim, (separator-p));
>
> Space between `separator', `-' and `p'.  If you want, you can put this
> `memcmp' call directly on the `if' condition, and then you wouldn't need
> the braces on the `for'.
>
Modifying this part.
>> +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not
>                                             ^^
> Two spaces after period.
>
>> +  if (pipe->handle == NULL)
>> +    error (_("Failed to create pipe"));
>> +
>> +    {
>> +      int status;
>> +      const char *err
>> +       = pex_run (pipe->pex,
>> +               PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
>> +               argv[0], argv,
>> +               NULL, NULL,
>> +               &status);
>> +      if (err)
>> +     error (_("Failed to execute %s"), argv[0]);
>> +
>> +      do_cleanups (cleanup);
>> +    }
>
> Why the braces?
Since pex_run returns a const char * I have to assign err at declaration time.
>
>> +  if (pipe->pex)
>> +    {
>> +      int status;
>> +
>> +      pex_get_status (pipe->pex, 1, &status);
>> +      pex_free (pipe->pex);
>
> Do you really need to call `pex_get_status' here?  I'm really asking,
> because I don't know about pex.
I tried without using pex_get_status, but pex_run is killing the child
process without calling waitpid (or equivalent). This is causing an
immediate exit of shell command. I may be missing something. Let me
dig out further inside code.
>
> Thanks for your work on this patch.
>
> Regards,
>
> Sergio.
>

Finally thank you all for your valuable time, showing me directions
and through review of the code. Shortly I will come back with
suggested modifications.

Regards,
Abhijit Halder

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

[Abhijit Halder]

On Mon, Aug 8, 2011 at 3:30 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> Hi Abhijit,
>>
>> Some minor nits (again).
>>
>> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>>
>>> +struct pipe_obj
>>> +{
>>> +  /* The delimiter to separate out gdb-command and shell-command. This can be
>>                                                                    ^^
> Sorry, here I am not clear. Do you mean to say I should remove - in
> between gdb and command?
>>
>> Two spaces after the period.
>>
>>> +  /* The pex object use to create pipeline between gdb and shell.  */
>>
> Here I guess I have put 2 space character between . (period) and */.
>> Some typos:
>>
>> "/*The pex object used to create a pipeline between GDB and shell.  */"
>>
>>> +  if (*p != '\0') *p++ = '\0';
>>
>> I'm not found of this style.  Please, put the assignment on the next
>> line:
> Yes a slip. I am modifying it.
>>
>>        if (*p != '\0')
>>          *p++ = '\0';
>>
>>
>>> +  for (pipe->shell_cmd = "";
>>
>> Sorry, I didn't understand this line.  Is this needed?  If so, I think
>> it's better if you do `pipe->shell_cmd = NULL'.
>>
>>> +      int mismatch = memcmp (p, pipe->dlim, (separator-p));
>>
>> Space between `separator', `-' and `p'.  If you want, you can put this
>> `memcmp' call directly on the `if' condition, and then you wouldn't need
>> the braces on the `for'.
>>
> Modifying this part.
>>> +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not
>>                                             ^^
>> Two spaces after period.
>>
>>> +  if (pipe->handle == NULL)
>>> +    error (_("Failed to create pipe"));
>>> +
>>> +    {
>>> +      int status;
>>> +      const char *err
>>> +       = pex_run (pipe->pex,
>>> +               PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
>>> +               argv[0], argv,
>>> +               NULL, NULL,
>>> +               &status);
>>> +      if (err)
>>> +     error (_("Failed to execute %s"), argv[0]);
>>> +
>>> +      do_cleanups (cleanup);
>>> +    }
>>
>> Why the braces?
> Since pex_run returns a const char * I have to assign err at declaration time.
>>
>>> +  if (pipe->pex)
>>> +    {
>>> +      int status;
>>> +
>>> +      pex_get_status (pipe->pex, 1, &status);
>>> +      pex_free (pipe->pex);
>>
>> Do you really need to call `pex_get_status' here?  I'm really asking,
>> because I don't know about pex.
> I tried without using pex_get_status, but pex_free is killing the child
> process without calling waitpid (or equivalent). This is causing an
> immediate exit of shell command. I may be missing something. Let me
> dig out further inside code.
Probably I got the answer here. In pex_free we close the std
descriptors before calling pex_get_status_and_time, hence we do not
get any output. I think we have to call pex_get_status before calling
pex_free.
>>
>> Thanks for your work on this patch.
>>
>> Regards,
>>
>> Sergio.
>>
>
> Finally thank you all for your valuable time, showing me directions
> and through review of the code. Shortly I will come back with
> suggested modifications.
>
> Regards,
> Abhijit Halder
>

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

[Sergio Durigan Junior]

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>>> +struct pipe_obj
>>> +{
>>> +  /* The delimiter to separate out gdb-command and shell-command. This can be
>>                                                                    ^^
> Sorry, here I am not clear. Do you mean to say I should remove - in
> between gdb and command?

No.  I meant you should add two spaces after the period.  Instead of:

"... and shell-command. This can be"

you should write:

"... and shell-command.  This can be"

>>> +  /* The pex object use to create pipeline between gdb and shell.  */
>>
> Here I guess I have put 2 space character between . (period) and */.

Yes.

Regards.

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

[Abhijit Halder]

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

On Tue, Aug 9, 2011 at 8:32 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior
>> <sergiodj@redhat.com> wrote:
>>>> +struct pipe_obj
>>>> +{
>>>> +  /* The delimiter to separate out gdb-command and shell-command. This can be
>>>                                                                    ^^
>> Sorry, here I am not clear. Do you mean to say I should remove - in
>> between gdb and command?
>
> No.  I meant you should add two spaces after the period.  Instead of:
>
> "... and shell-command. This can be"
>
> you should write:
>
> "... and shell-command.  This can be"
>
>>>> +  /* The pex object use to create pipeline between gdb and shell.  */
>>>
>> Here I guess I have put 2 space character between . (period) and */.
>
> Yes.
>
> Regards.
>

I made the corrections suggested during code review.

Thanks,
Abhijit Halder

[-- Attachment #2: gdb-pipe-command-corrected.patch --]
[-- Type: text/x-patch, Size: 6411 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-09 15:53:48.462145884 +0530
@@ -0,0 +1,210 @@
+/* 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 <ctype.h>
+#include "gdb_string.h"
+#include "ui-file.h"
+#include "ui-out.h"
+#include "cli/cli-utils.h"
+#include "gdbcmd.h"
+#include "libiberty.h"
+
+/* Structure to encapsulate all entities associated with pipe.  */
+
+struct pipe_obj
+{
+  /* The delimiter to separate out gdb-command and shell-command.  This can be
+     any arbitrary string without containing any whitespace.  */
+  char *dlim;
+
+  /* The gdb-command.  */
+  char *gdb_cmd;
+
+  /* The shell-command.  */
+  char *shell_cmd;
+
+  /* The gdb-side stream pointer to the pipe.  */
+  FILE *handle;
+
+  /* The pex object used to create pipeline between gdb and shell.  */
+  struct pex_obj *pex;
+};
+
+/* Construct a pipe object by parsing argument to the pipe command.  */
+
+static struct pipe_obj *
+construct_pipe (char *p)
+{
+  char *t;
+  struct pipe_obj *pipe = NULL;
+  struct cleanup *cleanup;
+
+  if (p == NULL)
+    error (_("No argument is specified"));
+
+  pipe = XCNEW (struct pipe_obj);
+  cleanup = make_cleanup (xfree, pipe);
+
+  pipe->dlim = p;
+
+  t = skip_to_space (p);
+  p = skip_spaces (t);
+
+  if (*p == '\0')
+    error (_("No gdb-command is specified"));
+
+  *t = '\0';
+  pipe->gdb_cmd = p;
+
+  for (;;)
+    {
+      t = skip_to_space (p);
+
+      if (*t == '\0')
+	error (_("No shell-command is specified"));
+
+      /* Check whether the token separated by whitespace matches with
+	 delimiter.  */ 
+      if (memcmp (p, pipe->dlim, (t - p)) == 0)
+	{
+	  *p = '\0';
+	  pipe->shell_cmd = skip_spaces (t);
+	  break;
+	}
+
+       p = skip_spaces (t);
+    }
+
+  discard_cleanups (cleanup);
+  return pipe;
+}
+
+/* Run execute_command for P and FROM_TTY.  Write output to the pipe, do not
+   display it to the screen.  */
+
+static void
+execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
+{
+  char **argv;
+  struct cleanup *cleanup;
+  struct ui_file *fp;
+
+  argv = gdb_buildargv (pipe->shell_cmd);
+  cleanup = make_cleanup_freeargv (argv);
+
+  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
+
+  if (pipe->pex == NULL)
+    do_cleanups (cleanup);
+
+  pipe->handle = pex_input_pipe (pipe->pex, 0);
+
+  if (pipe->handle == NULL)
+    error (_("Failed to create pipe"));
+
+    {
+      int status;
+      const char *err
+       = pex_run (pipe->pex,
+		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
+		  argv[0], argv,
+		  NULL, NULL,
+		  &status);
+      if (err != NULL)
+	error (_("Failed to execute %s"), argv[0]);
+
+      do_cleanups (cleanup);
+    }
+
+  /* GDB_STDOUT should be better already restored during these
+     restoration callbacks.  */
+  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
+  fp = stdio_fileopen (pipe->handle);
+  make_cleanup_ui_file_delete (fp);
+  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);
+
+  if (ui_out_redirect (uiout, fp) < 0)
+    warning (_("Current output protocol does not support redirection"));
+  else 
+    make_cleanup_ui_out_redirect_pop (uiout);
+
+  gdb_stdout = fp;
+  gdb_stderr = fp;
+  gdb_stdlog = fp;
+  gdb_stdtarg = fp;
+  gdb_stdtargerr = fp;
+  execute_command (pipe->gdb_cmd, from_tty);
+  do_cleanups (cleanup);
+}
+
+/* Destruct pipe object.  */
+
+static void
+destruct_pipe (void *arg)
+{
+  struct pipe_obj *pipe = (struct pipe_obj *)arg;
+
+  if (pipe->handle != NULL)
+    fclose (pipe->handle);
+
+  if (pipe->pex != NULL)
+    {
+      int status;
+
+      /* Wait till the process in the other side of the pipe completes its
+	 job before closing its file descryptors.  */
+      pex_get_status (pipe->pex, 1, &status);
+      pex_free (pipe->pex);
+    }
+
+  xfree (pipe);
+}
+
+/* Execute the pipe command.  */
+
+static void
+pipe_command (char *arg, int from_tty)
+{
+  struct pipe_obj *pipe = construct_pipe (arg);
+
+  if (pipe != NULL)
+    {
+      struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
+
+      execute_command_to_pipe (pipe, from_tty);
+      do_cleanups (cleanup);
+    }
+}
+
+/* Module initialization.  */
+
+void
+_initialize_pipe (void)
+{
+  add_cmd ("pipe", no_class, pipe_command, _("\
+Create pipe to pass gdb-command output to the shell for processing.\n\
+Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
+again and finally a shell-command."),
+	   &cmdlist);
+}

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

[Sergio Durigan Junior]

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> I made the corrections suggested during code review.

Well, from my perspective I don't think I have anything else about the
patch.  However, I have some questions:

1) I don't remember if you already answered that, but do you have a
copyright assignment (or have you started the process to obtain one)?

2) I believe you can start thinking about a testcase for this feature.
I am not sure how you would write this in order to test it on various
operating systems, but I think starting with GNU/Linux is OK for now.
You may want to take a look at the gdb/testsuite directory for some
examples.

3) I also believe that you can write the documentation part for this
patch.  You will also have to write a NEWS entry.

Maybe you can wait a little more until some maintainer reply before
start steps 2 and 3.  Anyway, it's good to start thinking about these
steps.

Regards,

Sergio.

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

[Abhijit Halder]

On Wed, Aug 10, 2011 at 11:24 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> I made the corrections suggested during code review.
>
> Well, from my perspective I don't think I have anything else about the
> patch.  However, I have some questions:
>
> 1) I don't remember if you already answered that, but do you have a
> copyright assignment (or have you started the process to obtain one)?
>
Hi, actually I am not aware of how to perform this. Can someone please
provide me some reference pointer to know about this process? It will
be a great help.

> 2) I believe you can start thinking about a testcase for this feature.
> I am not sure how you would write this in order to test it on various
> operating systems, but I think starting with GNU/Linux is OK for now.
> You may want to take a look at the gdb/testsuite directory for some
> examples.
>
> 3) I also believe that you can write the documentation part for this
> patch.  You will also have to write a NEWS entry.
>
> Maybe you can wait a little more until some maintainer reply before
> start steps 2 and 3.  Anyway, it's good to start thinking about these
> steps.
>
> Regards,
>
> Sergio.
>

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

[Sergio Durigan Junior]

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> On Wed, Aug 10, 2011 at 11:24 PM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> 1) I don't remember if you already answered that, but do you have a
>> copyright assignment (or have you started the process to obtain one)?
>>
> Hi, actually I am not aware of how to perform this. Can someone please
> provide me some reference pointer to know about this process? It will
> be a great help.

Please contact Tom Tromey privately and he can get you started on this.

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

[Jan Kratochvil]

On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote:
> On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and
> > gdb_stdtargerr like set_logging_redirect does.  This is like if one
> > uses |& instead of | .  I understand someone may have a different opinion.
> > In such case there even may be some option of the command pipe for it.
> I understand your point here, but this will break out very basic
> assumption that delimiter is single character.
> Can we skip this at this point? Or simply we can redirect gdb_stderror
> to pipe as in most of the cases that will be equivalent to gdb_stdout.
> (My understanding was that the error from gdb can be captured anyway
> by error logging mechanism, the use-case of using pipe, I believe, is
> when the gdb output is huge and one wants to process that huge
> output.) Please suggest.

The reason for redirecting also errors I find that when you type:

$ ls /tmp bar|less

you happilly see the content of /tmp but the message

ls: cannot access bar: No such file or directory

gets completely lost.  This is why I find |& safer for normal use cases.

(One can argue also the other way, that standard shell pipe | also does not
automatically redirect stderr, though.)


Thanks,
Jan

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

[Abhijit Halder]

On Sat, Aug 13, 2011 at 10:53 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote:
>> On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
>> > I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and
>> > gdb_stdtargerr like set_logging_redirect does.  This is like if one
>> > uses |& instead of | .  I understand someone may have a different opinion.
>> > In such case there even may be some option of the command pipe for it.
>> I understand your point here, but this will break out very basic
>> assumption that delimiter is single character.
>> Can we skip this at this point? Or simply we can redirect gdb_stderror
>> to pipe as in most of the cases that will be equivalent to gdb_stdout.
>> (My understanding was that the error from gdb can be captured anyway
>> by error logging mechanism, the use-case of using pipe, I believe, is
>> when the gdb output is huge and one wants to process that huge
>> output.) Please suggest.
>
> The reason for redirecting also errors I find that when you type:
>
> $ ls /tmp bar|less
>
> you happilly see the content of /tmp but the message
>
> ls: cannot access bar: No such file or directory
>
> gets completely lost.  This is why I find |& safer for normal use cases.
>
> (One can argue also the other way, that standard shell pipe | also does not
> automatically redirect stderr, though.)
>
I have also redirected the gdb_stderr to pipe.

+  fp = stdio_fileopen (pipe->handle);
+  make_cleanup_ui_file_delete (fp);
+  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);
+
+  if (ui_out_redirect (uiout, fp) < 0)
+    warning (_("Current output protocol does not support redirection"));
+  else
+    make_cleanup_ui_out_redirect_pop (uiout);
+
+  gdb_stdout = fp;
+  gdb_stderr = fp;
+  gdb_stdlog = fp;
+  gdb_stdtarg = fp;
+  gdb_stdtargerr = fp;
+  execute_command (pipe->gdb_cmd, from_tty);

But the error is still coming on screen.

(gdb) pipe | print xxx | vim -
Vim: Reading from stdin...

No symbol table is loaded.  Use the "file" command.
(gdb) pipe | print 2 | vim -
Vim: Reading from stdin...

(gdb)

>
> Thanks,
> Jan
>

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

[Jan Kratochvil]

On Tue, 09 Aug 2011 12:42:25 +0200, Abhijit Halder wrote:
[...]
> --- src/gdb/pipe.c	2011-07-29 15:15:26.078048517 +0530
> +++ dst/gdb/pipe.c	2011-08-09 15:53:48.462145884 +0530
> @@ -0,0 +1,210 @@
> +/* 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 <ctype.h>
> +#include "gdb_string.h"
> +#include "ui-file.h"
> +#include "ui-out.h"
> +#include "cli/cli-utils.h"
> +#include "gdbcmd.h"
> +#include "libiberty.h"
> +
> +/* Structure to encapsulate all entities associated with pipe.  */
> +
> +struct pipe_obj
> +{
> +  /* The delimiter to separate out gdb-command and shell-command.  This can be
> +     any arbitrary string without containing any whitespace.  */
                               not   containing
> +  char *dlim;
> +
> +  /* The gdb-command.  */
> +  char *gdb_cmd;
> +
> +  /* The shell-command.  */
> +  char *shell_cmd;
> +
> +  /* The gdb-side stream pointer to the pipe.  */
> +  FILE *handle;
> +
> +  /* The pex object used to create pipeline between gdb and shell.  */
> +  struct pex_obj *pex;
> +};
> +
> +/* Construct a pipe object by parsing argument to the pipe command.  */

It is formal but there should be a note what is P.


> +
> +static struct pipe_obj *
> +construct_pipe (char *p)
> +{
> +  char *t;
> +  struct pipe_obj *pipe = NULL;
> +  struct cleanup *cleanup;
> +
> +  if (p == NULL)
> +    error (_("No argument is specified"));
> +
> +  pipe = XCNEW (struct pipe_obj);
> +  cleanup = make_cleanup (xfree, pipe);
> +
> +  pipe->dlim = p;

If we should keep the command `pipe' extensible in future by the options here
p[0] should be forbidden to contain '-'.


Currently returned pipe_obj still references P.  Either xstrdup + xfree all
the strings inside pipe_obj or xstrdup P itself first (and xfree it
afterwards), you can also use pipe_obj->last_field[1] string for storing
P etc.  This is not a bug but I find construct_pipe not useful as standalone
function.  Otherwise you can also inline the code inside pipe_command,
pipe_obj then should contain just few/two/one entries neededed for the cleanup
function destruct_pipe.  Currently when pipe_obj references external data its
`char *' should be in fact `const char *' but referencing external data is
wrong anyway.


> +
> +  t = skip_to_space (p);
> +  p = skip_spaces (t);
> +
> +  if (*p == '\0')
> +    error (_("No gdb-command is specified"));
> +
> +  *t = '\0';
> +  pipe->gdb_cmd = p;
> +
> +  for (;;)
> +    {
> +      t = skip_to_space (p);
> +
> +      if (*t == '\0')
> +	error (_("No shell-command is specified"));
> +
> +      /* Check whether the token separated by whitespace matches with
> +	 delimiter.  */ 
> +      if (memcmp (p, pipe->dlim, (t - p)) == 0)

Here is missing: && pipe->dlim[t - p] == 0
otherwise this invalid line is valid:
(gdb) pipe abc print 1 ab cat


> +	{
> +	  *p = '\0';
> +	  pipe->shell_cmd = skip_spaces (t);
> +	  break;
> +	}
> +
> +       p = skip_spaces (t);
> +    }
> +
> +  discard_cleanups (cleanup);
> +  return pipe;
> +}
> +
> +/* Run execute_command for P and FROM_TTY.  Write output to the pipe, do not

There is no parameter called P.  Maybe `the pipe' should have been `PIPE' if
you mean that parameter (you may not).


> +   display it to the screen.  */
> +
> +static void
> +execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
> +{
> +  char **argv;
> +  struct cleanup *cleanup;
> +  struct ui_file *fp;
> +
> +  argv = gdb_buildargv (pipe->shell_cmd);
> +  cleanup = make_cleanup_freeargv (argv);
> +
> +  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
> +
> +  if (pipe->pex == NULL)
> +    do_cleanups (cleanup);

In such case do_cleanup is not enough, you have to return as it no longer
makes sense to do anything here, best to just call `error'.  But pex_init can
never fail and one case in GDB already does not expect it could fail so even
removal of the two lines would be also OK.


> +
> +  pipe->handle = pex_input_pipe (pipe->pex, 0);
> +
> +  if (pipe->handle == NULL)
> +    error (_("Failed to create pipe"));
> +
> +    {

This indentation is not correct, it should be two spaces, not four spaces.
But I also do not see why to start a new block here, just move the status
+ err variables to the function variables block.


> +      int status;
> +      const char *err
> +       = pex_run (pipe->pex,
> +		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,

Why PEX_STDERR_TO_STDOUT?  I am not so much against it but I do not see
a reason for it.


> +		  argv[0], argv,

It does not work in one of the intended modes:
(gdb) pipe | print 1 | cat >/dev/null
cat: >/dev/null: No such file or directory

You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c",
COMMAND, NULL (sure no PEX_SEARCH needed then).


> +		  NULL, NULL,
> +		  &status);

There should be an empty space between declarations and code statements.


> +      if (err != NULL)
> +	error (_("Failed to execute %s"), argv[0]);

It has returned the ERR erroe message, thus ERR should be also printed,
together with STATUS interpreted like errno.


> +
> +      do_cleanups (cleanup);

Formally do_cleanups indentation should match creating of that cleanup, if
possible, like it is here.  But I think after the other comments this cleanup
will no longer be there anyway.


> +    }
> +
> +  /* GDB_STDOUT should be better already restored during these
> +     restoration callbacks.  */
> +  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
> +  fp = stdio_fileopen (pipe->handle);
> +  make_cleanup_ui_file_delete (fp);
> +  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);
> +
> +  if (ui_out_redirect (uiout, fp) < 0)

It has been renamed to current_uiout.


> +    warning (_("Current output protocol does not support redirection"));
> +  else 
> +    make_cleanup_ui_out_redirect_pop (uiout);
> +
> +  gdb_stdout = fp;
> +  gdb_stderr = fp;
> +  gdb_stdlog = fp;
> +  gdb_stdtarg = fp;
> +  gdb_stdtargerr = fp;
> +  execute_command (pipe->gdb_cmd, from_tty);
> +  do_cleanups (cleanup);
> +}
> +
> +/* Destruct pipe object.  */

Here should be described ARG and that it is compliant to the cleanup handler
prototype.


> +
> +static void
> +destruct_pipe (void *arg)
> +{
> +  struct pipe_obj *pipe = (struct pipe_obj *)arg;

GCS (GNU Coding Style) says: (struct pipe_obj *) arg;

> +
> +  if (pipe->handle != NULL)
> +    fclose (pipe->handle);
> +
> +  if (pipe->pex != NULL)
> +    {
> +      int status;
> +
> +      /* Wait till the process in the other side of the pipe completes its
> +	 job before closing its file descryptors.  */

My English is far from perfect but at least use "on the other side" and
"descriptors", I would write:
/* Wait till the process on the other right side of the pipe completes its job
   before killing it due to the PIPE close.  */


> +      pex_get_status (pipe->pex, 1, &status);

Here could be a warning on non-successful pipe command exit.


> +      pex_free (pipe->pex);
> +    }
> +
> +  xfree (pipe);
> +}
> +
> +/* Execute the pipe command.  */
> +
> +static void
> +pipe_command (char *arg, int from_tty)
> +{
> +  struct pipe_obj *pipe = construct_pipe (arg);
> +
> +  if (pipe != NULL)

construct_pipe never returns NULL.  It would be questionable whether write an
error message in such case etc. but it is irrelevant.


> +    {
> +      struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
> +
> +      execute_command_to_pipe (pipe, from_tty);
> +      do_cleanups (cleanup);
> +    }
> +}
> +
> +/* Module initialization.  */
> +
> +void
> +_initialize_pipe (void)

$ make CFLAGS=-Wmissing-prototypes pipe.o
pipe.c:203:1: error: no previous prototype for ‘_initialize_pipe’ [-Werror=missing-prototypes]

I do not know why GDB does not use -Wmissing-prototypes by default but the
code is ready for it, such as:
	/* Provide a prototype to silence -Wmissing-prototypes.  */
	extern initialize_file_ftype _initialize_linux_nat;

	void
	_initialize_linux_nat (void)


> +{
> +  add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe to pass gdb-command output to the shell for processing.\n\
> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
> +again and finally a shell-command."),
> +	   &cmdlist);
> +}

And the documentation, NEWS entry and testcase is missing as already noted.


Thanks,
Jan

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

[Jan Kratochvil]

On Sat, 13 Aug 2011 20:09:57 +0200, Abhijit Halder wrote:
> I have also redirected the gdb_stderr to pipe.
[...]
> +  gdb_stdout = fp;
> +  gdb_stderr = fp;
> +  gdb_stdlog = fp;
> +  gdb_stdtarg = fp;
> +  gdb_stdtargerr = fp;
> +  execute_command (pipe->gdb_cmd, from_tty);
> 
> But the error is still coming on screen.
> 
> (gdb) pipe | print xxx | vim -
> Vim: Reading from stdin...
> 
> No symbol table is loaded.  Use the "file" command.
> (gdb) pipe | print 2 | vim -
> Vim: Reading from stdin...
> 
> (gdb)

Hmm, true, the error message is stored in the block but the error message is
printed only after the exception cleanups were run.  You can use there (do not
use this indentation):

{ volatile struct gdb_exception except;
  TRY_CATCH (except, RETURN_MASK_ERROR)
    {
      execute_command (pipe->gdb_cmd, from_tty);
    }
  if (except.reason < 0)
    exception_print (gdb_stderr, except);
}


Thanks,
Jan

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

[Abhijit Halder]

>> +               argv[0], argv,
>
> It does not work in one of the intended modes:
> (gdb) pipe | print 1 | cat >/dev/null
> cat: >/dev/null: No such file or directory
>
> You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c",
> COMMAND, NULL (sure no PEX_SEARCH needed then).
>
I am sure, but will this wrok in windows platform?
> And the documentation, NEWS entry and testcase is missing as already noted.
>
>
> Thanks,
> Jan
>

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

[Abhijit Halder]

On Sun, Aug 14, 2011 at 4:46 AM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
>>> +               argv[0], argv,
>>
>> It does not work in one of the intended modes:
>> (gdb) pipe | print 1 | cat >/dev/null
>> cat: >/dev/null: No such file or directory
>>
>> You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c",
>> COMMAND, NULL (sure no PEX_SEARCH needed then).
>>
> I am sure, but will this wrok in windows platform?
>> And the documentation, NEWS entry and testcase is missing as already noted.
>>
Sorry for the typo. I meant to say I am NOT sure whether the given
solution will work in windows platform.
>>
>> Thanks,
>> Jan
>>
>

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

[Jan Kratochvil]

On Sun, 14 Aug 2011 01:18:34 +0200, Abhijit Halder wrote:
> On Sun, Aug 14, 2011 at 4:46 AM, Abhijit Halder
> <abhijit.k.halder@gmail.com> wrote:
> >>> +               argv[0], argv,
> >>
> >> It does not work in one of the intended modes:
> >> (gdb) pipe | print 1 | cat >/dev/null
> >> cat: >/dev/null: No such file or directory
> >>
> >> You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c",
> >> COMMAND, NULL (sure no PEX_SEARCH needed then).
> >>
> > I am sure, but will this wrok in windows platform?
> >> And the documentation, NEWS entry and testcase is missing as already noted.
> >>
> Sorry for the typo. I meant to say I am NOT sure whether the given
> solution will work in windows platform.

I guess it will work in Cygwin but it probably does not work in MinGW but that
is outside of my knowledge, MinGW advice needed, thanks for noticing it.


Thanks,
Jan

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

[Eli Zaretskii]

> Date: Sun, 14 Aug 2011 14:14:08 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Sergio Durigan Junior <sergiodj@redhat.com>, Eli Zaretskii <eliz@gnu.org>,
>         tromey@redhat.com, pedro@codesourcery.com, gdb-patches@sourceware.org
> 
> > Sorry for the typo. I meant to say I am NOT sure whether the given
> > solution will work in windows platform.
> 
> I guess it will work in Cygwin but it probably does not work in MinGW but that
> is outside of my knowledge, MinGW advice needed, thanks for noticing it.

Where's the issue about which you need MinGW advice?  What exactly
might not work?  Can you please point me to the relevant message(s)?
This has been a long thread, and I admit I didn't read all of it.  I
tried to locate the issue a few messages back, but couldn't find
anything appropriate.

Thanks.

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

[Jan Kratochvil]

On Sun, 14 Aug 2011 16:08:32 +0200, Eli Zaretskii wrote:
> Where's the issue about which you need MinGW advice?

http://sourceware.org/ml/gdb-patches/2011-08/msg00278.html
On Sat, 13 Aug 2011 22:50:54 +0200, Jan Kratochvil wrote:
# On Tue, 09 Aug 2011 12:42:25 +0200, Abhijit Halder wrote:
# > +       = pex_run (pipe->pex,
# > +		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
# 
# Why PEX_STDERR_TO_STDOUT?  I am not so much against it but I do not see
# a reason for it.
# 
# 
# > +		  argv[0], argv,
# 
# It does not work in one of the intended modes:
# (gdb) pipe | print 1 | cat >/dev/null
# cat: >/dev/null: No such file or directory
# 
# You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c",
# COMMAND, NULL (sure no PEX_SEARCH needed then).


> What exactly might not work?

Abhijit Halder has suggested the suggested
	const char *argv[] = { "sh", "-c", "cat >/dev/null", NULL };
	pex_run (pipe->pex, PEX_LAST, "/bin/sh", argv, NULL, NULL, &status);
works on UNIX but it will probably not work on MinGW as there is no "/bin/sh",
is it?

The current implementation:
	const char *argv[] = { "cat", ">/dev/null", NULL };
	pex_run (pipe->pex, PEX_SEARCH | PEX_LAST, "cat", argv, NULL, NULL, &status);
produces that - therefore not working on UNIX:
	cat: >/dev/null: No such file or directory


Thanks,
Jan

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

[Eli Zaretskii]

> Date: Sun, 14 Aug 2011 19:01:36 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: abhijit.k.halder@gmail.com, sergiodj@redhat.com, tromey@redhat.com,
>         pedro@codesourcery.com, gdb-patches@sourceware.org
> 
> > What exactly might not work?
> 
> Abhijit Halder has suggested the suggested
> 	const char *argv[] = { "sh", "-c", "cat >/dev/null", NULL };
> 	pex_run (pipe->pex, PEX_LAST, "/bin/sh", argv, NULL, NULL, &status);
> works on UNIX but it will probably not work on MinGW as there is no "/bin/sh",
> is it?

The file name of the shell, the "-c" switch, and the null device
should all be system-dependent.  For MinGW, they should be,
respectively, "cmd.exe", "/c", and "nul".

> The current implementation:
> 	const char *argv[] = { "cat", ">/dev/null", NULL };
> 	pex_run (pipe->pex, PEX_SEARCH | PEX_LAST, "cat", argv, NULL, NULL, &status);
> produces that - therefore not working on UNIX:
> 	cat: >/dev/null: No such file or directory

How hard is it to parse the redirection characters?

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

[Jan Kratochvil]

On Sun, 14 Aug 2011 21:41:57 +0200, Eli Zaretskii wrote:
> > Abhijit Halder has suggested the suggested
> > 	const char *argv[] = { "sh", "-c", "cat >/dev/null", NULL };
> > 	pex_run (pipe->pex, PEX_LAST, "/bin/sh", argv, NULL, NULL, &status);
> > works on UNIX but it will probably not work on MinGW as there is no "/bin/sh",
> > is it?
> 
> The file name of the shell, the "-c" switch, and the null device
> should all be system-dependent.  For MinGW, they should be,
> respectively, "cmd.exe", "/c", and "nul".

"cat >/dev/null" itself is user-entered command so that part is OK to be
non-cross-platform (therefore also "nul" is not valid).

OK, "cmd.exe" with "/c" seems easy enough.


> > The current implementation:
> > 	const char *argv[] = { "cat", ">/dev/null", NULL };
> > 	pex_run (pipe->pex, PEX_SEARCH | PEX_LAST, "cat", argv, NULL, NULL, &status);
> > produces that - therefore not working on UNIX:
> > 	cat: >/dev/null: No such file or directory
> 
> How hard is it to parse the redirection characters?

Very, >, >>, &>, >&, &>>, x>&y, x>&y-, x<>y, many more. 


Thanks,
Jan

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

[Abhijit Halder]

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

On Mon, Aug 15, 2011 at 1:25 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sun, 14 Aug 2011 21:41:57 +0200, Eli Zaretskii wrote:
>> > Abhijit Halder has suggested the suggested
>> >     const char *argv[] = { "sh", "-c", "cat >/dev/null", NULL };
>> >     pex_run (pipe->pex, PEX_LAST, "/bin/sh", argv, NULL, NULL, &status);
>> > works on UNIX but it will probably not work on MinGW as there is no "/bin/sh",
>> > is it?
>>
>> The file name of the shell, the "-c" switch, and the null device
>> should all be system-dependent.  For MinGW, they should be,
>> respectively, "cmd.exe", "/c", and "nul".
>
> "cat >/dev/null" itself is user-entered command so that part is OK to be
> non-cross-platform (therefore also "nul" is not valid).
>
> OK, "cmd.exe" with "/c" seems easy enough.
>
>
>> > The current implementation:
>> >     const char *argv[] = { "cat", ">/dev/null", NULL };
>> >     pex_run (pipe->pex, PEX_SEARCH | PEX_LAST, "cat", argv, NULL, NULL, &status);
>> > produces that - therefore not working on UNIX:
>> >     cat: >/dev/null: No such file or directory
>>
>> How hard is it to parse the redirection characters?
>
> Very, >, >>, &>, >&, &>>, x>&y, x>&y-, x<>y, many more.
>
>
> Thanks,
> Jan
>

Corrected the pattern matching code and added code to take care MinGW.

Working on testcase and doc part.

[-- Attachment #2: gdb-pipe-command-corrected.patch --]
[-- Type: text/x-patch, Size: 7420 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-08-14 07:31:15.802233004 +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/NEWS dst/gdb/NEWS
--- src/gdb/NEWS	2011-08-14 07:27:51.582233000 +0530
+++ dst/gdb/NEWS	2011-08-14 07:50:38.562234498 +0530
@@ -64,6 +64,9 @@
 
 * New commands "info macros", and "info definitions" have been added.
 
+* New command "pipe" has been added to make GDB command output available to the
+  shell for processing. 
+
 * Changed commands
 
 watch EXPRESSION mask MASK_VALUE
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-16 14:05:56.771665999 +0530
@@ -0,0 +1,227 @@
+/* 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 <ctype.h>
+#include "gdb_string.h"
+#include "ui-file.h"
+#include "ui-out.h"
+#include "cli/cli-utils.h"
+#include "gdbcmd.h"
+#include "libiberty.h"
+#include "exceptions.h"
+
+#if defined(__MINGW32__)
+#define SHELL "cmd.exe"
+#define OPTION_TO_SHELL "/c"
+#else
+#define SHELL "/bin/sh"
+#define OPTION_TO_SHELL "-c"
+#endif
+
+/* Structure to encapsulate all entities associated with pipe.  */
+
+struct pipe_obj
+{
+  /* The delimiter to separate out gdb-command and shell-command.  This can be
+     any arbitrary string without containing any whitespace.  */
+  char *dlim;
+
+  /* The gdb-command.  */
+  char *gdb_cmd;
+
+  /* The shell-command.  */
+  char *shell_cmd;
+
+  /* The gdb-side stream pointer to the pipe.  */
+  FILE *handle;
+
+  /* The pex object used to create pipeline between gdb and shell.  */
+  struct pex_obj *pex;
+};
+
+/* Destruct pipe object referenced by ARG.  */
+
+static void
+destruct_pipe (void *arg)
+{
+  struct pipe_obj *pipe = (struct pipe_obj *) arg;
+
+  if (pipe->handle != NULL)
+    fclose (pipe->handle);
+
+  if (pipe->pex != NULL)
+    {
+      int status;
+
+      /* Wait till the process on the other side of the pipe completes its
+	 job before closing its file descriptors.  */
+      pex_get_status (pipe->pex, 1, &status);
+
+      if (!WIFEXITED (status))
+	warning (_("Execution of shell-command `%s' may not be completed"),
+		 pipe->shell_cmd);
+
+      pex_free (pipe->pex);
+    }
+
+  xfree (pipe->dlim);
+  xfree (pipe);
+}
+
+/* Construct a pipe object by parsing argument ARG to the pipe command.  */
+
+static struct pipe_obj *
+construct_pipe (char *arg)
+{
+  char *p, *t;
+  struct pipe_obj *pipe = NULL;
+  struct cleanup *cleanup;
+
+  if (arg == NULL)
+    error (_("No argument is specified"));
+
+  pipe = XCNEW (struct pipe_obj);
+  cleanup = make_cleanup (destruct_pipe, pipe);
+
+  p = xstrdup (arg);
+  pipe->dlim = p;
+
+  if (*pipe->dlim == '-')
+    error (_("Delimiter pattern should not start with '-'"));
+
+  t = skip_to_space (p);
+  p = skip_spaces (t);
+
+  if (*p == '\0')
+    error (_("No gdb-command is specified"));
+
+  *t = '\0';
+  pipe->gdb_cmd = p;
+
+  for (;;)
+    {
+      t = skip_to_space (p);
+
+      if (*t == '\0')
+	error (_("No shell-command is specified"));
+
+      /* Check whether the token separated by whitespace matches with
+	 delimiter.  */ 
+      if (memcmp (p, pipe->dlim, (t - p)) == 0
+	  && pipe->dlim[t - p] == '\0')
+	{
+	  *p = '\0';
+	  pipe->shell_cmd = skip_spaces (t);
+	  break;
+	}
+
+       p = skip_spaces (t);
+    }
+
+  discard_cleanups (cleanup);
+  return pipe;
+}
+
+/* Run execute_command for PIPE and FROM_TTY.  Write output to the pipe, do not
+   display it to the screen.  */
+
+static void
+execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
+{
+  char *argv[4];
+  struct cleanup *cleanup;
+  struct ui_file *fp;
+  int status;
+  const char *errmsg;
+  volatile struct gdb_exception exception;
+
+  argv[0] = SHELL;
+  argv[1] = OPTION_TO_SHELL;
+  argv[2] = pipe->shell_cmd;
+  argv[3] = NULL;
+
+  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
+  pipe->handle = pex_input_pipe (pipe->pex, 0);
+
+  if (pipe->handle == NULL)
+    error (_("Failed to create pipe"));
+
+  errmsg = pex_run (pipe->pex, PEX_LAST, argv[0], argv, NULL, NULL, &status);
+
+  if (errmsg != NULL)
+    error (_("Failed to execute shell-command `%s' on pipe: %s"),
+	   pipe->shell_cmd, errmsg);
+
+  /* GDB_STDOUT should be better already restored during these
+     restoration callbacks.  */
+  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
+  fp = stdio_fileopen (pipe->handle);
+  make_cleanup_ui_file_delete (fp);
+  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);
+
+  if (ui_out_redirect (current_uiout, fp) < 0)
+    warning (_("Current output protocol does not support redirection"));
+  else 
+    make_cleanup_ui_out_redirect_pop (current_uiout);
+
+  gdb_stdout = fp;
+  gdb_stderr = fp;
+  gdb_stdlog = fp;
+  gdb_stdtarg = fp;
+  gdb_stdtargerr = fp;
+
+  TRY_CATCH (exception, RETURN_MASK_ERROR)
+    {
+      execute_command (pipe->gdb_cmd, from_tty);
+    }
+
+  if (exception.reason < 0)
+    exception_print (gdb_stderr, exception);
+
+  do_cleanups (cleanup);
+}
+
+/* Execute the pipe command with argument ARG and FROM_TTY.  */
+
+static void
+pipe_command (char *arg, int from_tty)
+{
+  struct pipe_obj *pipe = construct_pipe (arg);
+  struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
+
+  execute_command_to_pipe (pipe, from_tty);
+  do_cleanups (cleanup);
+}
+
+/* Module initialization.  */
+
+void
+_initialize_pipe (void)
+{
+  add_cmd ("pipe", no_class, pipe_command, _("\
+Create pipe to pass gdb-command output to the shell for processing.\n\
+Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
+again and finally a shell-command."),
+	   &cmdlist);
+}

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 269 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@gmail.com>

	Implementation of pipe to make I/O communication 
	between gdb and shell.

	* pipe.c: New file.
	* Makefile.in (SFILES): Add pipe.c.
	(COMMON_OBS): Add pipe.o. 
	* NEWS: Add news item for the new command `pipe'.

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

[Eli Zaretskii]

> Date: Tue, 16 Aug 2011 18:15:19 +0530
> From: Abhijit Halder <abhijit.k.halder@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, sergiodj@redhat.com, tromey@redhat.com, 
> 	pedro@codesourcery.com, gdb-patches@sourceware.org
> 
> --- src/gdb/NEWS	2011-08-14 07:27:51.582233000 +0530
> +++ dst/gdb/NEWS	2011-08-14 07:50:38.562234498 +0530
> @@ -64,6 +64,9 @@
>  
>  * New commands "info macros", and "info definitions" have been added.
>  
> +* New command "pipe" has been added to make GDB command output available to the
> +  shell for processing. 
> +
>  * Changed commands

This part is okay.

> +void
> +_initialize_pipe (void)
> +{
> +  add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe to pass gdb-command output to the shell for processing.\n\
> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
> +again and finally a shell-command."),

This doc string should say something about what can be the delimiter.

What about the manual?  (Apologies if I already reviewed that part:
this has been a very long thread.)

Thanks.

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

[Abhijit Halder]

On Tue, Aug 16, 2011 at 7:08 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Tue, 16 Aug 2011 18:15:19 +0530
>> From: Abhijit Halder <abhijit.k.halder@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, sergiodj@redhat.com, tromey@redhat.com,
>>       pedro@codesourcery.com, gdb-patches@sourceware.org
>>
>> --- src/gdb/NEWS      2011-08-14 07:27:51.582233000 +0530
>> +++ dst/gdb/NEWS      2011-08-14 07:50:38.562234498 +0530
>> @@ -64,6 +64,9 @@
>>
>>  * New commands "info macros", and "info definitions" have been added.
>>
>> +* New command "pipe" has been added to make GDB command output available to the
>> +  shell for processing.
>> +
>>  * Changed commands
>
> This part is okay.
>
>> +void
>> +_initialize_pipe (void)
>> +{
>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe to pass gdb-command output to the shell for processing.\n\
>> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
>> +again and finally a shell-command."),
>
> This doc string should say something about what can be the delimiter.
>
> What about the manual?  (Apologies if I already reviewed that part:
> this has been a very long thread.)
I have not yet got a chance to work on document part. I am currently
working on it.
>
> Thanks.
>

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

[Abhijit Halder]

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

On Tue, Aug 16, 2011 at 7:39 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> On Tue, Aug 16, 2011 at 7:08 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> Date: Tue, 16 Aug 2011 18:15:19 +0530
>>> From: Abhijit Halder <abhijit.k.halder@gmail.com>
>>> Cc: Eli Zaretskii <eliz@gnu.org>, sergiodj@redhat.com, tromey@redhat.com,
>>>       pedro@codesourcery.com, gdb-patches@sourceware.org
>>>
>>> --- src/gdb/NEWS      2011-08-14 07:27:51.582233000 +0530
>>> +++ dst/gdb/NEWS      2011-08-14 07:50:38.562234498 +0530
>>> @@ -64,6 +64,9 @@
>>>
>>>  * New commands "info macros", and "info definitions" have been added.
>>>
>>> +* New command "pipe" has been added to make GDB command output available to the
>>> +  shell for processing.
>>> +
>>>  * Changed commands
>>
>> This part is okay.
>>
>>> +void
>>> +_initialize_pipe (void)
>>> +{
>>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>>> +Create pipe to pass gdb-command output to the shell for processing.\n\
>>> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
>>> +again and finally a shell-command."),
>>
>> This doc string should say something about what can be the delimiter.
>>
>> What about the manual?  (Apologies if I already reviewed that part:
>> this has been a very long thread.)
> I have not yet got a chance to work on document part. I am currently
> working on it.
>>
>> Thanks.
>>
>
Added test-case for this new command. Please review this.

Thanks,
Abhijit Halder

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

diff -rup src/gdb/testsuite/gdb.base/pipe.c dst/gdb/testsuite/gdb.base/pipe.c
--- src/gdb/testsuite/gdb.base/pipe.c	2011-08-16 22:37:37.785351001 +0530
+++ dst/gdb/testsuite/gdb.base/pipe.c	2011-08-16 22:32:40.213350709 +0530
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 Free Software Foundation, Inc.
+
+   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 <stdio.h>
+
+int main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+  return 0; /* end main */
+}
diff -rup src/gdb/testsuite/gdb.base/pipe.exp dst/gdb/testsuite/gdb.base/pipe.exp
--- src/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:37:45.969351119 +0530
+++ dst/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:35:34.117356781 +0530
@@ -0,0 +1,49 @@
+#   Copyright 2011 Free Software Foundation, Inc.
+
+# 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/>.
+
+#
+# test gdb pipe commands
+#
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile "pipe"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested pipe.exp
+    return -1
+}
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir ${srcdir}/${subdir}
+gdb_load ${binfile}
+
+set end_main [gdb_get_line_number " end main " $srcfile]
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "pipe | bt | grep \"main\"" "\#0.*main.*().*at.*${srcfile}:${end_main}"
+

[-- Attachment #3: ChangeLog --]
[-- Type: application/octet-stream, Size: 150 bytes --]

2011-07-29 Abhijit Halder <abhijit.k.halder@gmail.com>

	Testsuite for `pipe' command.

	* gdb.base/pipe.c: New file.
	* gdb.base/pipe.exp: New file.

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

[Jan Kratochvil]

On Tue, 16 Aug 2011 19:25:21 +0200, Abhijit Halder wrote:
> --- src/gdb/testsuite/gdb.base/pipe.c	2011-08-16 22:37:37.785351001 +0530
> +++ dst/gdb/testsuite/gdb.base/pipe.c	2011-08-16 22:32:40.213350709 +0530

There isn't needed any .c file.

> diff -rup src/gdb/testsuite/gdb.base/pipe.exp dst/gdb/testsuite/gdb.base/pipe.exp
> --- src/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:37:45.969351119 +0530
> +++ dst/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:35:34.117356781 +0530
> @@ -0,0 +1,49 @@
> +#   Copyright 2011 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +#
> +# test gdb pipe commands
> +#
> +
> +if $tracelevel then {
> +    strace $tracelevel
> +}

This is rather redundant, please see:
	http://sourceware.org/gdb/wiki/GDBTestcaseCookbook


> +
> +set testfile "pipe"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested pipe.exp
> +    return -1
> +}
> +
> +if [get_compiler_info ${binfile}] {
> +    return -1
> +}
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir ${srcdir}/${subdir}
> +gdb_load ${binfile}
> +
> +set end_main [gdb_get_line_number " end main " $srcfile]
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}

Just keep gdb_exit and gdb_start from all of this.


> +
> +gdb_test "pipe | bt | grep \"main\"" "\#0.*main.*().*at.*${srcfile}:${end_main}"
> +

I would not use `bt' which is very complicated and for the `pipe'
functionality arbitrary command - such as `print' - is enough.

But it is more important to test various many cases of pipe, such as various
non-standard delimiters besides "|", also the redirection of shell output,
using pipe in the shell part (double/triple pipe), testing the error cases
that they are caught correctly etc.


Thanks,
Jan

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

[Abhijit Halder]

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

On Tue, Aug 16, 2011 at 11:00 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 16 Aug 2011 19:25:21 +0200, Abhijit Halder wrote:
>> --- src/gdb/testsuite/gdb.base/pipe.c 2011-08-16 22:37:37.785351001 +0530
>> +++ dst/gdb/testsuite/gdb.base/pipe.c 2011-08-16 22:32:40.213350709 +0530
>
> There isn't needed any .c file.
>
>> diff -rup src/gdb/testsuite/gdb.base/pipe.exp dst/gdb/testsuite/gdb.base/pipe.exp
>> --- src/gdb/testsuite/gdb.base/pipe.exp       2011-08-16 22:37:45.969351119 +0530
>> +++ dst/gdb/testsuite/gdb.base/pipe.exp       2011-08-16 22:35:34.117356781 +0530
>> @@ -0,0 +1,49 @@
>> +#   Copyright 2011 Free Software Foundation, Inc.
>> +
>> +# 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/>.
>> +
>> +#
>> +# test gdb pipe commands
>> +#
>> +
>> +if $tracelevel then {
>> +    strace $tracelevel
>> +}
>
> This is rather redundant, please see:
>        http://sourceware.org/gdb/wiki/GDBTestcaseCookbook
>
>
>> +
>> +set testfile "pipe"
>> +set srcfile ${testfile}.c
>> +set binfile ${objdir}/${subdir}/${testfile}
>> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
>> +    untested pipe.exp
>> +    return -1
>> +}
>> +
>> +if [get_compiler_info ${binfile}] {
>> +    return -1
>> +}
>> +
>> +gdb_exit
>> +gdb_start
>> +gdb_reinitialize_dir ${srcdir}/${subdir}
>> +gdb_load ${binfile}
>> +
>> +set end_main [gdb_get_line_number " end main " $srcfile]
>> +
>> +if ![runto_main] then {
>> +    fail "Can't run to main"
>> +    return 0
>> +}
>
> Just keep gdb_exit and gdb_start from all of this.
>
>
>> +
>> +gdb_test "pipe | bt | grep \"main\"" "\#0.*main.*().*at.*${srcfile}:${end_main}"
>> +
>
> I would not use `bt' which is very complicated and for the `pipe'
> functionality arbitrary command - such as `print' - is enough.
>
> But it is more important to test various many cases of pipe, such as various
> non-standard delimiters besides "|", also the redirection of shell output,
> using pipe in the shell part (double/triple pipe), testing the error cases
> that they are caught correctly etc.
>
>
> Thanks,
> Jan
>

Here is the correction.

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

diff -rup src/gdb/testsuite/gdb.base/pipe.exp dst/gdb/testsuite/gdb.base/pipe.exp
--- src/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:37:45.969351119 +0530
+++ dst/gdb/testsuite/gdb.base/pipe.exp	2011-08-17 13:49:26.195383001 +0530
@@ -0,0 +1,37 @@
+#   Copyright 2011 Free Software Foundation, Inc.
+
+# 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/>.
+
+#
+# test gdb pipe commands
+#
+
+set testfile "pipe"
+set tempfile "temp.xxx"
+
+gdb_exit
+gdb_start
+
+gdb_test "pipe" "No argument is specified"
+gdb_test "pipe |" "No gdb-command is specified"
+gdb_test "pipe -x" "Delimiter pattern should not start with '-'"
+gdb_test "pipe | print 'x'" "No shell-command is specified"
+gdb_test "pipe | print 'x' |< cat" "No shell-command is specified"
+gdb_test "pipe |< print 'x' | cat" "No shell-command is specified"
+gdb_test "pipe | print 'x' >| cat" "No shell-command is specified"
+gdb_test "pipe >| print 'x' | cat" "No shell-command is specified"
+gdb_test "pipe | print 'x' | cat" " = 120 'x'"
+gdb_test "pipe | print 'x' | cat | cat" " = 120 'x'"
+gdb_test "pipe | p 'x' | cat >$tempfile ; if \[ -f $tempfile \] ; \
+          then echo \"SUCCESS\" ; rm -f $tempfile ; fi" "SUCCESS"

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

[Jan Kratochvil]

On Wed, 17 Aug 2011 10:26:36 +0200, Abhijit Halder wrote:
> --- src/gdb/testsuite/gdb.base/pipe.exp	2011-08-16 22:37:45.969351119 +0530
> +++ dst/gdb/testsuite/gdb.base/pipe.exp	2011-08-17 13:49:26.195383001 +0530
> @@ -0,0 +1,37 @@
> +#   Copyright 2011 Free Software Foundation, Inc.

Excessive two spaces.

[...]
> +set testfile "pipe"
> +set tempfile "temp.xxx"
[...]
> +gdb_test "pipe | p 'x' | cat >$tempfile ; if \[ -f $tempfile \] ; \
> +          then echo \"SUCCESS\" ; rm -f $tempfile ; fi" "SUCCESS"

This will create file in current directory, that should not happen.  Please
use ${objdir}/${subdir} and also ${testfile} prefix, something like:
set tempfile ${objdir}/${subdir}/${testfile}.x

$tempfile should be deleted first as otherwise it may create false PASSes.

Optional - not required but I would find better: You could use TCL `file
exists' and `file delete' instead of making some more dependencies on host
shell compatibility.  You could also check the content of such file, whether
the redirection was successful, lib/gdb.exp build_id_debug_filename_get
contains reading of a file by TCL.


Thanks,
Jan

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

[Jan Kratochvil]

On Tue, 16 Aug 2011 14:45:19 +0200, Abhijit Halder wrote:
> +#include "defs.h"
> +#include <ctype.h>
> +#include "gdb_string.h"
> +#include "ui-file.h"
> +#include "ui-out.h"
> +#include "cli/cli-utils.h"
> +#include "gdbcmd.h"
> +#include "libiberty.h"
> +#include "exceptions.h"
> +
> +#if defined(__MINGW32__)
> +#define SHELL "cmd.exe"
> +#define OPTION_TO_SHELL "/c"
> +#else
> +#define SHELL "/bin/sh"
> +#define OPTION_TO_SHELL "-c"
> +#endif

GNU Coding style would say:
#if defined (__MINGW32__)
but #ifdef would be also OK IMO.

and indent, please:

# define SHELL "cmd.exe"
# define OPTION_TO_SHELL "/c"
#else
# define SHELL "/bin/sh"
# define OPTION_TO_SHELL "-c"
#endif

It would be nice to get the __MINGW32__ tested but OK to check it in if no
test appears, some __MINGW32__ fix later may be easy.


> +
> +/* Structure to encapsulate all entities associated with pipe.  */
> +
> +struct pipe_obj
> +{
> +  /* The delimiter to separate out gdb-command and shell-command.  This can be
> +     any arbitrary string without containing any whitespace.  */
> +  char *dlim;
> +
> +  /* The gdb-command.  */
> +  char *gdb_cmd;
> +
> +  /* The shell-command.  */
> +  char *shell_cmd;
> +
> +  /* The gdb-side stream pointer to the pipe.  */
> +  FILE *handle;
> +
> +  /* The pex object used to create pipeline between gdb and shell.  */
> +  struct pex_obj *pex;
> +};
> +
> +/* Destruct pipe object referenced by ARG.  */
> +
> +static void
> +destruct_pipe (void *arg)
> +{
> +  struct pipe_obj *pipe = (struct pipe_obj *) arg;
> +
> +  if (pipe->handle != NULL)
> +    fclose (pipe->handle);
> +
> +  if (pipe->pex != NULL)
> +    {
> +      int status;
> +
> +      /* Wait till the process on the other side of the pipe completes its
> +	 job before closing its file descriptors.  */
> +      pex_get_status (pipe->pex, 1, &status);
> +
> +      if (!WIFEXITED (status))
> +	warning (_("Execution of shell-command `%s' may not be completed"),
> +		 pipe->shell_cmd);

WEXITSTATUS could be checked but you may not agree, OK if so.


> +
> +      pex_free (pipe->pex);
> +    }
> +
> +  xfree (pipe->dlim);
> +  xfree (pipe);
> +}
> +
> +/* Construct a pipe object by parsing argument ARG to the pipe command.  */
> +
> +static struct pipe_obj *
> +construct_pipe (char *arg)

ARG can be now `const char *' when it is xstrdup-ed anyway.

> +{
> +  char *p, *t;
> +  struct pipe_obj *pipe = NULL;
> +  struct cleanup *cleanup;
> +
> +  if (arg == NULL)
> +    error (_("No argument is specified"));
> +
> +  pipe = XCNEW (struct pipe_obj);
> +  cleanup = make_cleanup (destruct_pipe, pipe);
> +
> +  p = xstrdup (arg);
> +  pipe->dlim = p;
> +
> +  if (*pipe->dlim == '-')
> +    error (_("Delimiter pattern should not start with '-'"));
> +
> +  t = skip_to_space (p);
> +  p = skip_spaces (t);
> +
> +  if (*p == '\0')
> +    error (_("No gdb-command is specified"));
> +
> +  *t = '\0';
> +  pipe->gdb_cmd = p;
> +
> +  for (;;)
> +    {
> +      t = skip_to_space (p);
> +
> +      if (*t == '\0')
> +	error (_("No shell-command is specified"));
> +
> +      /* Check whether the token separated by whitespace matches with
> +	 delimiter.  */ 
> +      if (memcmp (p, pipe->dlim, (t - p)) == 0
> +	  && pipe->dlim[t - p] == '\0')
> +	{
> +	  *p = '\0';
> +	  pipe->shell_cmd = skip_spaces (t);
> +	  break;
> +	}
> +
> +       p = skip_spaces (t);
> +    }
> +
> +  discard_cleanups (cleanup);
> +  return pipe;
> +}
> +
> +/* Run execute_command for PIPE and FROM_TTY.  Write output to the pipe, do not
> +   display it to the screen.  */
> +
> +static void
> +execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
> +{
> +  char *argv[4];
> +  struct cleanup *cleanup;
> +  struct ui_file *fp;
> +  int status;
> +  const char *errmsg;
> +  volatile struct gdb_exception exception;
> +
> +  argv[0] = SHELL;
> +  argv[1] = OPTION_TO_SHELL;
> +  argv[2] = pipe->shell_cmd;
> +  argv[3] = NULL;
> +
> +  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
> +  pipe->handle = pex_input_pipe (pipe->pex, 0);
> +
> +  if (pipe->handle == NULL)
> +    error (_("Failed to create pipe"));
> +
> +  errmsg = pex_run (pipe->pex, PEX_LAST, argv[0], argv, NULL, NULL, &status);
> +
> +  if (errmsg != NULL)
> +    error (_("Failed to execute shell-command `%s' on pipe: %s"),
> +	   pipe->shell_cmd, errmsg);

Also output: safe_strerror (status)


> +
> +  /* GDB_STDOUT should be better already restored during these
> +     restoration callbacks.  */
> +  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
> +  fp = stdio_fileopen (pipe->handle);
> +  make_cleanup_ui_file_delete (fp);
> +  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);
> +
> +  if (ui_out_redirect (current_uiout, fp) < 0)
> +    warning (_("Current output protocol does not support redirection"));
> +  else 
> +    make_cleanup_ui_out_redirect_pop (current_uiout);
> +
> +  gdb_stdout = fp;
> +  gdb_stderr = fp;
> +  gdb_stdlog = fp;
> +  gdb_stdtarg = fp;
> +  gdb_stdtargerr = fp;
> +
> +  TRY_CATCH (exception, RETURN_MASK_ERROR)
> +    {
> +      execute_command (pipe->gdb_cmd, from_tty);
> +    }
> +
> +  if (exception.reason < 0)
> +    exception_print (gdb_stderr, exception);
> +
> +  do_cleanups (cleanup);
> +}
> +
> +/* Execute the pipe command with argument ARG and FROM_TTY.  */
> +
> +static void
> +pipe_command (char *arg, int from_tty)
> +{
> +  struct pipe_obj *pipe = construct_pipe (arg);
> +  struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
> +
> +  execute_command_to_pipe (pipe, from_tty);
> +  do_cleanups (cleanup);
> +}
> +
> +/* Module initialization.  */
> +
> +void
> +_initialize_pipe (void)
> +{
> +  add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe to pass gdb-command output to the shell for processing.\n\
> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
> +again and finally a shell-command."),
> +	   &cmdlist);
> +}


But otherwise OK to check it in with those few changes, approved doc and the
testcase.


Thanks,
Jan

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

[Abhijit Halder]

On Sun, Aug 21, 2011 at 12:45 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 16 Aug 2011 14:45:19 +0200, Abhijit Halder wrote:
>> +#include "defs.h"
>> +#include <ctype.h>
>> +#include "gdb_string.h"
>> +#include "ui-file.h"
>> +#include "ui-out.h"
>> +#include "cli/cli-utils.h"
>> +#include "gdbcmd.h"
>> +#include "libiberty.h"
>> +#include "exceptions.h"
>> +
>> +#if defined(__MINGW32__)
>> +#define SHELL "cmd.exe"
>> +#define OPTION_TO_SHELL "/c"
>> +#else
>> +#define SHELL "/bin/sh"
>> +#define OPTION_TO_SHELL "-c"
>> +#endif
>
> GNU Coding style would say:
> #if defined (__MINGW32__)
> but #ifdef would be also OK IMO.
>
> and indent, please:
>
> # define SHELL "cmd.exe"
> # define OPTION_TO_SHELL "/c"
> #else
> # define SHELL "/bin/sh"
> # define OPTION_TO_SHELL "-c"
> #endif
>
> It would be nice to get the __MINGW32__ tested but OK to check it in if no
> test appears, some __MINGW32__ fix later may be easy.
>
>
>> +
>> +/* Structure to encapsulate all entities associated with pipe.  */
>> +
>> +struct pipe_obj
>> +{
>> +  /* The delimiter to separate out gdb-command and shell-command.  This can be
>> +     any arbitrary string without containing any whitespace.  */
>> +  char *dlim;
>> +
>> +  /* The gdb-command.  */
>> +  char *gdb_cmd;
>> +
>> +  /* The shell-command.  */
>> +  char *shell_cmd;
>> +
>> +  /* The gdb-side stream pointer to the pipe.  */
>> +  FILE *handle;
>> +
>> +  /* The pex object used to create pipeline between gdb and shell.  */
>> +  struct pex_obj *pex;
>> +};
>> +
>> +/* Destruct pipe object referenced by ARG.  */
>> +
>> +static void
>> +destruct_pipe (void *arg)
>> +{
>> +  struct pipe_obj *pipe = (struct pipe_obj *) arg;
>> +
>> +  if (pipe->handle != NULL)
>> +    fclose (pipe->handle);
>> +
>> +  if (pipe->pex != NULL)
>> +    {
>> +      int status;
>> +
>> +      /* Wait till the process on the other side of the pipe completes its
>> +      job before closing its file descriptors.  */
>> +      pex_get_status (pipe->pex, 1, &status);
>> +
>> +      if (!WIFEXITED (status))
>> +     warning (_("Execution of shell-command `%s' may not be completed"),
>> +              pipe->shell_cmd);
>
> WEXITSTATUS could be checked but you may not agree, OK if so.
>
>
>> +
>> +      pex_free (pipe->pex);
>> +    }
>> +
>> +  xfree (pipe->dlim);
>> +  xfree (pipe);
>> +}
>> +
>> +/* Construct a pipe object by parsing argument ARG to the pipe command.  */
>> +
>> +static struct pipe_obj *
>> +construct_pipe (char *arg)
>
> ARG can be now `const char *' when it is xstrdup-ed anyway.
>
>> +{
>> +  char *p, *t;
>> +  struct pipe_obj *pipe = NULL;
>> +  struct cleanup *cleanup;
>> +
>> +  if (arg == NULL)
>> +    error (_("No argument is specified"));
>> +
>> +  pipe = XCNEW (struct pipe_obj);
>> +  cleanup = make_cleanup (destruct_pipe, pipe);
>> +
>> +  p = xstrdup (arg);
>> +  pipe->dlim = p;
>> +
>> +  if (*pipe->dlim == '-')
>> +    error (_("Delimiter pattern should not start with '-'"));
>> +
>> +  t = skip_to_space (p);
>> +  p = skip_spaces (t);
>> +
>> +  if (*p == '\0')
>> +    error (_("No gdb-command is specified"));
>> +
>> +  *t = '\0';
>> +  pipe->gdb_cmd = p;
>> +
>> +  for (;;)
>> +    {
>> +      t = skip_to_space (p);
>> +
>> +      if (*t == '\0')
>> +     error (_("No shell-command is specified"));
>> +
>> +      /* Check whether the token separated by whitespace matches with
>> +      delimiter.  */
>> +      if (memcmp (p, pipe->dlim, (t - p)) == 0
>> +       && pipe->dlim[t - p] == '\0')
>> +     {
>> +       *p = '\0';
>> +       pipe->shell_cmd = skip_spaces (t);
>> +       break;
>> +     }
>> +
>> +       p = skip_spaces (t);
>> +    }
>> +
>> +  discard_cleanups (cleanup);
>> +  return pipe;
>> +}
>> +
>> +/* Run execute_command for PIPE and FROM_TTY.  Write output to the pipe, do not
>> +   display it to the screen.  */
>> +
>> +static void
>> +execute_command_to_pipe (struct pipe_obj *pipe, int from_tty)
>> +{
>> +  char *argv[4];
>> +  struct cleanup *cleanup;
>> +  struct ui_file *fp;
>> +  int status;
>> +  const char *errmsg;
>> +  volatile struct gdb_exception exception;
>> +
>> +  argv[0] = SHELL;
>> +  argv[1] = OPTION_TO_SHELL;
>> +  argv[2] = pipe->shell_cmd;
>> +  argv[3] = NULL;
>> +
>> +  pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL);
>> +  pipe->handle = pex_input_pipe (pipe->pex, 0);
>> +
>> +  if (pipe->handle == NULL)
>> +    error (_("Failed to create pipe"));
>> +
>> +  errmsg = pex_run (pipe->pex, PEX_LAST, argv[0], argv, NULL, NULL, &status);
>> +
>> +  if (errmsg != NULL)
>> +    error (_("Failed to execute shell-command `%s' on pipe: %s"),
>> +        pipe->shell_cmd, errmsg);
>
> Also output: safe_strerror (status)
>
>
>> +
>> +  /* GDB_STDOUT should be better already restored during these
>> +     restoration callbacks.  */
>> +  cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
>> +  fp = stdio_fileopen (pipe->handle);
>> +  make_cleanup_ui_file_delete (fp);
>> +  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);
>> +
>> +  if (ui_out_redirect (current_uiout, fp) < 0)
>> +    warning (_("Current output protocol does not support redirection"));
>> +  else
>> +    make_cleanup_ui_out_redirect_pop (current_uiout);
>> +
>> +  gdb_stdout = fp;
>> +  gdb_stderr = fp;
>> +  gdb_stdlog = fp;
>> +  gdb_stdtarg = fp;
>> +  gdb_stdtargerr = fp;
>> +
>> +  TRY_CATCH (exception, RETURN_MASK_ERROR)
>> +    {
>> +      execute_command (pipe->gdb_cmd, from_tty);
>> +    }
>> +
>> +  if (exception.reason < 0)
>> +    exception_print (gdb_stderr, exception);
>> +
>> +  do_cleanups (cleanup);
>> +}
>> +
>> +/* Execute the pipe command with argument ARG and FROM_TTY.  */
>> +
>> +static void
>> +pipe_command (char *arg, int from_tty)
>> +{
>> +  struct pipe_obj *pipe = construct_pipe (arg);
>> +  struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe);
>> +
>> +  execute_command_to_pipe (pipe, from_tty);
>> +  do_cleanups (cleanup);
>> +}
>> +
>> +/* Module initialization.  */
>> +
>> +void
>> +_initialize_pipe (void)
>> +{
>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe to pass gdb-command output to the shell for processing.\n\
>> +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \
>> +again and finally a shell-command."),
>> +        &cmdlist);
>> +}
>
>
> But otherwise OK to check it in with those few changes, approved doc and the
> testcase.
>
Sorry for bit delay in response. I am working on the doc part and
making corrections as suggested. At the same making progress on
copyright assignment (it may take few days). Thanks a lot for
reviewing this piece of code.
>
> Thanks,
> Jan
>