From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12125 invoked by alias); 5 Aug 2011 09:41:48 -0000 Received: (qmail 12111 invoked by uid 22791); 5 Aug 2011 09:41:45 -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-ey0-f169.google.com (HELO mail-ey0-f169.google.com) (209.85.215.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 05 Aug 2011 09:41:10 +0000 Received: by eye22 with SMTP id 22so1758846eye.0 for ; Fri, 05 Aug 2011 02:41:09 -0700 (PDT) MIME-Version: 1.0 Received: by 10.213.114.66 with SMTP id d2mr618649ebq.102.1312537269177; Fri, 05 Aug 2011 02:41:09 -0700 (PDT) Received: by 10.213.16.210 with HTTP; Fri, 5 Aug 2011 02:41:09 -0700 (PDT) In-Reply-To: <20110805082947.GA5020@host1.jankratochvil.net> References: <201108041029.37721.pedro@codesourcery.com> <20110805082947.GA5020@host1.jankratochvil.net> Date: Fri, 05 Aug 2011 09:41: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/msg00097.txt.bz2 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 modi= fy >> + =A0 it under the terms of the GNU General Public License as published = 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 License >> + =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 able = to: > =A0 =A0 =A0 =A0(gdb) pipe | command | cat >file > > Why there should be such explicitly limited delimiter choice? =A0Couldn't= the > pipe command default to '|' and otherwise take an arbitrary first word? > (The word should never start with '-' to have the options extension possi= bility > 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 own= ed by the >> + =A0 shell. In future we may start supporting read mode of operations a= s 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. > 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. >> + >> + =A0/* The gdb-end stream pointer to the pipe. =A0*/ >> + =A0FILE *handle; >> +}; >> + >> +/* 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 precedes = first > use. Just a good practice. > >> + >> +static struct pipe_object * >> +construct_pipe (char *p) > > Missing function comment. > =46rom function name hope things are revealed. Please suggest if comments are really useful. >> +{ >> + =A0struct pipe_object *pipe =3D NULL; >> + =A0struct cleanup *old_chain; >> + >> + =A0if (p !=3D NULL && *p !=3D '\0') > > P can never be NULL, and if it could be it should be rather > =A0 =A0 =A0 =A0gdb_assert (p !=3D NULL); Yes I agree. > That *p !=3D '\0' I also do not understand, it should print an error mess= age in > such case IMO. In the following case *p will be '\0'. (gdb)pipe ||echo "Do nothing" This can be consider not an error. (Please suggest) Again, if so, we should not go ahead (going ahead will not break anything, just incur extra overhead in memory allocation and de-allocation) > > >> + =A0 =A0{ >> + =A0 =A0 =A0pipe =3D xmalloc (sizeof (struct pipe_object)); >> + =A0 =A0 =A0old_chain =3D make_cleanup (xfree, pipe); >> + =A0 =A0 =A0pipe->mode =3D WR_TEXT; >> + >> + =A0 =A0 =A0p =3D skip_spaces (p); >> + =A0 =A0 =A0pipe->dlim =3D *p++; > > This can skip to unallocated memory if *p =3D=3D 0. Control will never come here under such condition. > >> + =A0 =A0 =A0p =3D skip_spaces (p); >> + =A0 =A0 =A0pipe->gdb_cmd =3D p; >> + >> + =A0 =A0 =A0/* Validate the delimiter from a pre-defined whitelist char= acters. This >> + =A0 =A0 =A0will enforce not to use special (e.g. alpha-numeric) charac= ters. =A0*/ >> + =A0 =A0 =A0/* NOTE: If DLIM become null, P starts pointing to a bad me= mory >> + =A0 =A0 =A0location, hence before doing further processing of P we sho= uld check >> + =A0 =A0 =A0DLIM. =A0*/ >> + =A0 =A0 =A0if (pipe->dlim =3D=3D '\0' >> + =A0 =A0 =A0 || strchr (PIPE_DELIMITER, pipe->dlim) =3D=3D NULL) >> + =A0 =A0 error (_("Invalid delimiter '%c'"), pipe->dlim); >> + >> + =A0 =A0 =A0if ((p =3D strchr (p, pipe->dlim)) =3D=3D NULL) >> + =A0 =A0 error (_("Found no shell command")); >> + >> + =A0 =A0 =A0*p++ =3D '\0'; >> + =A0 =A0 =A0pipe->shell_cmd =3D p; >> + >> + =A0 =A0 =A0pipe->handle =3D popen (pipe->shell_cmd, pipe->mode); > > There was already a review note `pexecute' should be used instead. Sorry I missed that. I will change it. > > >> + >> + =A0 =A0 =A0if (!pipe->handle) >> + =A0 =A0 error (_("Failed to create pipe.\n%s"), strerror (errno)); > > safe_strerror. =A0Also see the error messages in GDB, it is normally not = printed > on a new line, that is: > =A0 =A0 =A0 =A0error (_("Failed to create pipe: %s"), safe_strerror (errn= o)); > I will make this change. > >> + >> + =A0 =A0 =A0discard_cleanups (old_chain); >> + =A0 =A0} >> + >> + =A0return pipe; >> +} >> + >> +static void >> +execute_command_to_pipe (struct pipe_object *pipe, int from_tty) > > Missing function comment. > >> +{ >> + =A0struct cleanup *cleanup; >> + =A0struct ui_file *fp; >> + >> + =A0cleanup =3D set_batch_flag_and_make_cleanup_restore_page_info (); >> + =A0fp =3D stdio_fileopen (pipe->handle); >> + =A0make_cleanup_ui_file_delete (fp); >> + =A0make_cleanup_restore_ui_file (&gdb_stdout); >> + >> + =A0if (ui_out_redirect (uiout, fp) < 0) >> + =A0 =A0warning (_("Current output protocol does not support redirectio= n")); >> + =A0else >> + =A0 =A0make_cleanup_ui_out_redirect_pop (uiout); >> + >> + =A0gdb_stdout =3D fp; > > I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and > gdb_stdtargerr like set_logging_redirect does. =A0This is like if one > uses |& instead of | . =A0I understand someone may have a different opini= on. > In such case there even may be some option of the command pipe for it. I understand your point here, but this will break out very basic assumption that delimiter is single character. Can we skip this at this point? Or simply we can redirect gdb_stderror to pipe as in most of the cases that will be equivalent to gdb_stdout. (My understanding was that the error from gdb can be captured anyway by error logging mechanism, the use-case of using pipe, I believe, is when the gdb output is huge and one wants to process that huge output.) Please suggest. > > >> + =A0execute_command (pipe->gdb_cmd, from_tty); >> + =A0do_cleanups (cleanup); >> +} >> + >> +static void >> +destruct_pipe (struct pipe_object *pipe) > > Missing function comment. > >> +{ >> + =A0pclose (pipe->handle); Similar to _popen there is _pclose for windows platform. Can I safely use pclose here? > > It could report if the command terminates wrongly - exit status - of the > command. > > >> + =A0xfree (pipe); >> +} >> + >> +static void >> +pipe_command (char *arg, int from_tty) > > Missing function comment. > >> +{ >> + =A0struct pipe_object *pipe; >> + >> + =A0pipe =3D construct_pipe (arg); >> + =A0if (pipe !=3D NULL) >> + =A0 =A0{ >> + =A0 =A0 =A0execute_command_to_pipe (pipe, from_tty); >> + =A0 =A0 =A0destruct_pipe (pipe); > > execute_command_to_pipe can terminate prematurely, as execute_command can= call > error(), `pipe' here would get leaked, destruct_pipe should be called fro= m a > cleanup function. Yes. I will make this change. > >> + =A0 =A0} >> +} >> + >> +void >> +_initialize_pipe (void) > > There should be an extra declaration of _initialize_pipe as some compiler= s can > complain otherwise (GCC does not, I do not know which do myself). I believe that is there in framework. init.c, created at runtime, does contain this. Please correct me if I am wrong. > >> +{ >> + =A0add_cmd ("pipe", no_class, pipe_command, _("\ >> +Create pipe to pass gdb-command output to the shell for processing.\n\ >> +Arguments are a delimiter character, followed by a gdb-command, \ >> +followed by a shell-command."), >> + =A0 =A0 =A0 =A0&cmdlist); >> +} > > > There should be documentation in gdb/doc/gdb.texinfo for it. Sure, I am presently working on it. > > > Thanks, > Jan >