From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27461 invoked by alias); 5 Aug 2011 10:19:03 -0000 Received: (qmail 27452 invoked by uid 22791); 5 Aug 2011 10:19:01 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,TW_BT,TW_TD X-Spam-Check-By: sourceware.org Received: from mail-ew0-f41.google.com (HELO mail-ew0-f41.google.com) (209.85.215.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 05 Aug 2011 10:18:27 +0000 Received: by ewy9 with SMTP id 9so1564768ewy.0 for ; Fri, 05 Aug 2011 03:18:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.213.20.200 with SMTP id g8mr632452ebb.140.1312539505946; Fri, 05 Aug 2011 03:18:25 -0700 (PDT) Received: by 10.213.16.210 with HTTP; Fri, 5 Aug 2011 03:18:25 -0700 (PDT) In-Reply-To: <20110805094500.GA20246@host1.jankratochvil.net> References: <201108041029.37721.pedro@codesourcery.com> <20110805082947.GA5020@host1.jankratochvil.net> <20110805094500.GA20246@host1.jankratochvil.net> Date: Fri, 05 Aug 2011 10:19:00 -0000 Message-ID: Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. From: Abhijit Halder To: Jan Kratochvil Cc: Pedro Alves , gdb-patches@sourceware.org, Sergio Durigan Junior , Tom Tromey Content-Type: text/plain; charset=ISO-8859-1 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/msg00101.txt.bz2 On Fri, Aug 5, 2011 at 3:15 PM, Jan Kratochvil wrote: > On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote: >> On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil >> wrote: >> > On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote: >> >> --- src/gdb/pipe.c =A0 =A02011-07-29 15:15:26.078048517 +0530 >> >> +++ dst/gdb/pipe.c =A0 =A02011-08-05 13:10:51.411046880 +0530 >> >> @@ -0,0 +1,161 @@ >> >> +/* Everything about pipe, for GDB. >> >> + >> >> + =A0 Copyright (C) 2011 Free Software Foundation, Inc. >> >> + >> >> + =A0 This file is part of GDB. >> >> + >> >> + =A0 This program is free software; you can redistribute it and/or m= odify >> >> + =A0 it under the terms of the GNU General Public License as publish= ed by >> >> + =A0 the Free Software Foundation; either version 3 of the License, = or >> >> + =A0 (at your option) any later version. >> >> + >> >> + =A0 This program is distributed in the hope that it will be useful, >> >> + =A0 but WITHOUT ANY WARRANTY; without even the implied warranty of >> >> + =A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0See the >> >> + =A0 GNU General Public License for more details. >> >> + >> >> + =A0 You should have received a copy of the GNU General Public Licen= se >> >> + =A0 along with this program. =A0If not, see . =A0*/ >> >> + >> >> +#include "defs.h" >> >> +#include >> >> +#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 >> >> + =A0 gdb-command and shell command. =A0*/ >> >> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>" >> > >> > I see you are not fond of redirections but at least one now will be ab= le to: >> > =A0 =A0 =A0 =A0(gdb) pipe | command | cat >file >> > >> > Why there should be such explicitly limited delimiter choice? =A0Could= n't the >> > pipe command default to '|' and otherwise take an arbitrary first word? >> > (The word should never start with '-' to have the options extension po= ssibility >> > in the future.) =A0That is to permit: >> > =A0 =A0 =A0 =A0(gdb) pipe info threads | less >> > =A0 =A0 =A0 =A0(gdb) pipe : print 1 | 2 : less >> > =A0 =A0 =A0 =A0(gdb) pipe FOO print 1 | 2 FOO less >> > =A0 =A0 =A0 =A0etc. >> > >> I am not sure whether this restriction is meaningful. Ideally we >> should not support any alpha-numeric character as a delimiter just >> because of readability purpose. e.g. >> (gdb) pipe dthread apply all btdvim - >> Here d is delimiter. I don't think the above one is acceptable. Please >> suggest me if we simply can put a restriction of not using any >> alpha-numeric character as delimiter and that will do. >> Secondly I believe by FOO you meant a single character and not a >> string. Between delimiter and command there is no restriction of >> having any white-space. >> > >> >> + >> >> +/* The mode of stream operation. =A0*/ >> >> +typedef char *iostream_mode_t; >> >> + >> >> +/* At present we support only write mode of operations to the pipe, = i.e., >> >> + =A0 gdb-command can only write to the pipe whose other terminal is = owned by the >> >> + =A0 shell. In future we may start supporting read mode of operation= s as well. >> >> + =A0 But at present there is no need for that. =A0*/ >> >> +#define WR_TEXT "w" >> >> + >> >> +struct pipe_object >> > >> > Missing struct comment. >> > >> I thought the comment on individual fields reveal enough information >> to a developer. In existing code also I have seen similar practice in >> several places. Please suggest me whether I need to put some valuable >> comment here. >> >> +{ >> >> + =A0/* The shell-command. =A0*/ >> >> + =A0char *shell_cmd; >> >> + >> >> + =A0/* The gdb-command. =A0*/ >> >> + =A0char *gdb_cmd; >> >> + >> >> + =A0/* The delimiter to separate out gdb-command and shell-command. = =A0*/ >> >> + =A0char dlim; >> >> + >> >> + =A0/* The supported mode of stream operations on gdb-end pipe. =A0*/ >> >> + =A0iostream_mode_t mode; >> > >> > It is not redundant for the current code. > > It was a typo: It IS redundant for the current code. > > >> > >> Yes it is. I thought it is just a better encapsulation. In future if >> we are going to support any new mode (e.g. read mode) the mode will >> come as an option. Then it will be a good design to keep that >> information inside this structure. > > We do not try to invent a new style. =A0We just try to follow the GDB sty= le. > It took me some years to find out what the GDB style is. > > >> >> +/* Prototype of local functions. =A0*/ >> >> + >> >> +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 preced= es first >> > use. >> Just a good practice. > > Just follow the GDB style. > > >> > >> >> + >> >> +static struct pipe_object * >> >> +construct_pipe (char *p) >> > >> > Missing function comment. >> > >> >From function name hope things are revealed. Please suggest if >> comments are really useful. > > Just follow the GDB style (of new patches; there exists a lot of old code= not > having comments at the function entry). Sure, I will make the suggested changes. Since I am very new, I was blindly referring to some existing files. Hope I will able to learn things fast. > > > [ rest skipped now ] > > > > Thanks, > Jan >