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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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