This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell.
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Abhijit Halder <abhijit dot k dot halder at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 02 Aug 2011 00:52:13 -0300
- Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell.
- References: <CAOhZP9zU2K01zFDkChtutGT6yJjUeWYeGRNAAgPgM5rKwrgysg@mail.gmail.com> <CAOhZP9y6kB=v_qtMYnBWJOUYFE4vNEUghnYjardbBgv=CMJ4RQ@mail.gmail.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.