From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7917 invoked by alias); 2 Aug 2011 03:52:38 -0000 Received: (qmail 7908 invoked by uid 22791); 2 Aug 2011 03:52:37 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 02 Aug 2011 03:52:21 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p723qJQW030558 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 1 Aug 2011 23:52:19 -0400 Received: from psique ([10.3.112.4]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p723qEff025016; Mon, 1 Aug 2011 23:52:16 -0400 From: Sergio Durigan Junior To: Abhijit Halder Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. References: Date: Tue, 02 Aug 2011 03:52:00 -0000 In-Reply-To: (Abhijit Halder's message of "Fri, 29 Jul 2011 19:05:10 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00021.txt.bz2 Abhijit Halder writes: > On Fri, Jul 29, 2011 at 6:45 PM, Abhijit Halder > 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] gdb-cmd shell-cmd >> List of options go with pipe command: >> =C2=A0-r =C2=A0 gdb reads output of shell-command from pipe >> =C2=A0-w =C2=A0gdb passes output of a command to shell to process. >> =C2=A0- =C2=A0 =C2=A0end 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 =3D 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, 2= 002, > + 2003, 2004, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, In= c. Since this is a new file, the copyright notice should mention only this year (2011). > +#include "defs.h" > +#include "gdbcmd.h" > +#include is already included by "defs.h". > +#include > +#include Others can correct me, but I think you should include "gdb_string.h" instead of . > +#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 =3D NULL; > + int found_mode =3D 0, pipe_opt_done =3D 0; > + > + if (p !=3D NULL && *p !=3D '\0') > + { > + pipe =3D 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=20 > + 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 =3D *p++; > + pipe->gdb_cmd =3D p; > + > + /* Validate the delimiter from a pre-defined > + whitelist characters. This will enforce=20 Sorry about the nit-picking. There must be a space between `characters.' and `This'. > + not to use special (e.g., alpha-numeric) list=20 > + 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 =3D=3D '\0' ||=20 > + strchr ("|/\\'\"`#@!$%^", pipe->dlim) =3D=3D 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) =3D=3D 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++ =3D '\0'; > + pipe->shell_cmd =3D p; > + > + pipe->handle =3D popen (pipe->shell_cmd, pipe->mode); > + > + if (!pipe->handle) > + { > + internal_error (__FILE__, __LINE__, > + _("construct_pipe: failed to create pipe.\n%s"),=20 > + 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."),=20 > + &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, 2= 000, > + 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 .= */ > + > +#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); >=20=20 > +/* 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.