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

Sergio Durigan Junior
2011-08-02T03:52:38Z

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.