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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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.