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

Jan Kratochvil
2011-08-05T08:30:50Z

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