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