From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14851 invoked by alias); 5 Aug 2011 09:45:49 -0000 Received: (qmail 14836 invoked by uid 22791); 5 Aug 2011 09:45:47 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BT,TW_TD 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; Fri, 05 Aug 2011 09:45:12 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p759j5XG007761 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 5 Aug 2011 05:45:05 -0400 Received: from host1.jankratochvil.net (ovpn-116-26.ams2.redhat.com [10.36.116.26]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p759j2Ao023327 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 5 Aug 2011 05:45:04 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p759j1no020371; Fri, 5 Aug 2011 11:45:01 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p759j0Bk020354; Fri, 5 Aug 2011 11:45:00 +0200 Date: Fri, 05 Aug 2011 09:45:00 -0000 From: Jan Kratochvil To: Abhijit Halder Cc: Pedro Alves , gdb-patches@sourceware.org, Sergio Durigan Junior , Tom Tromey Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. Message-ID: <20110805094500.GA20246@host1.jankratochvil.net> References: <201108041029.37721.pedro@codesourcery.com> <20110805082947.GA5020@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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/msg00099.txt.bz2 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    2011-07-29 15:15:26.078048517 +0530 > >> +++ dst/gdb/pipe.c    2011-08-05 13:10:51.411046880 +0530 > >> @@ -0,0 +1,161 @@ > >> +/* Everything about pipe, for GDB. > >> + > >> +   Copyright (C) 2011 Free Software Foundation, Inc. > >> + > >> +   This file is part of GDB. > >> + > >> +   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 .  */ > >> + > >> +#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 > >> +   gdb-command and shell command.  */ > >> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>" > > > > I see you are not fond of redirections but at least one now will be able to: > >        (gdb) pipe | command | cat >file > > > > Why there should be such explicitly limited delimiter choice?  Couldn't the > > pipe command default to '|' and otherwise take an arbitrary first word? > > (The word should never start with '-' to have the options extension possibility > > in the future.)  That is to permit: > >        (gdb) pipe info threads | less > >        (gdb) pipe : print 1 | 2 : less > >        (gdb) pipe FOO print 1 | 2 FOO less > >        etc. > > > 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.  */ > >> +typedef char *iostream_mode_t; > >> + > >> +/* At present we support only write mode of operations to the pipe, i.e., > >> +   gdb-command can only write to the pipe whose other terminal is owned by the > >> +   shell. In future we may start supporting read mode of operations as well. > >> +   But at present there is no need for that.  */ > >> +#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. > >> +{ > >> +  /* The shell-command.  */ > >> +  char *shell_cmd; > >> + > >> +  /* The gdb-command.  */ > >> +  char *gdb_cmd; > >> + > >> +  /* The delimiter to separate out gdb-command and shell-command.  */ > >> +  char dlim; > >> + > >> +  /* The supported mode of stream operations on gdb-end pipe.  */ > >> +  iostream_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. We just try to follow the GDB style. It took me some years to find out what the GDB style is. > >> +/* Prototype of local functions.  */ > >> + > >> +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 precedes 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). [ rest skipped now ] Thanks, Jan