From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7027 invoked by alias); 5 Aug 2011 08:30:50 -0000 Received: (qmail 7001 invoked by uid 22791); 5 Aug 2011 08:30:46 -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_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 08:30:05 +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 p758TuIa025889 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 5 Aug 2011 04:29:56 -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 p758Tpg0001451 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 5 Aug 2011 04:29:54 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p758Tn8B005954; Fri, 5 Aug 2011 10:29:49 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p758Tmmq005947; Fri, 5 Aug 2011 10:29:48 +0200 Date: Fri, 05 Aug 2011 08:30: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: <20110805082947.GA5020@host1.jankratochvil.net> References: <201108041029.37721.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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/msg00093.txt.bz2 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. > + > +/* 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. > +{ > + /* 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. > + > + /* The gdb-end stream pointer to the pipe. */ > + FILE *handle; > +}; > + > +/* 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. > + > +static struct pipe_object * > +construct_pipe (char *p) Missing function comment. > +{ > + struct pipe_object *pipe = NULL; > + struct cleanup *old_chain; > + > + if (p != NULL && *p != '\0') P can never be NULL, and if it could be it should be rather gdb_assert (p != NULL); That *p != '\0' I also do not understand, it should print an error message in such case IMO. > + { > + pipe = xmalloc (sizeof (struct pipe_object)); > + old_chain = make_cleanup (xfree, pipe); > + pipe->mode = WR_TEXT; > + > + p = skip_spaces (p); > + pipe->dlim = *p++; This can skip to unallocated memory if *p == 0. > + p = skip_spaces (p); > + pipe->gdb_cmd = p; > + > + /* Validate the delimiter from a pre-defined whitelist characters. This > + will enforce not to use special (e.g. alpha-numeric) characters. */ > + /* NOTE: If DLIM become null, P starts pointing to a bad memory > + location, hence before doing further processing of P we should check > + DLIM. */ > + if (pipe->dlim == '\0' > + || strchr (PIPE_DELIMITER, pipe->dlim) == NULL) > + error (_("Invalid delimiter '%c'"), pipe->dlim); > + > + if ((p = strchr (p, pipe->dlim)) == NULL) > + error (_("Found no shell command")); > + > + *p++ = '\0'; > + pipe->shell_cmd = p; > + > + pipe->handle = popen (pipe->shell_cmd, pipe->mode); There was already a review note `pexecute' should be used instead. > + > + if (!pipe->handle) > + error (_("Failed to create pipe.\n%s"), strerror (errno)); safe_strerror. Also see the error messages in GDB, it is normally not printed on a new line, that is: error (_("Failed to create pipe: %s"), safe_strerror (errno)); > + > + discard_cleanups (old_chain); > + } > + > + return pipe; > +} > + > +static void > +execute_command_to_pipe (struct pipe_object *pipe, int from_tty) Missing function comment. > +{ > + struct cleanup *cleanup; > + struct ui_file *fp; > + > + cleanup = set_batch_flag_and_make_cleanup_restore_page_info (); > + fp = stdio_fileopen (pipe->handle); > + make_cleanup_ui_file_delete (fp); > + make_cleanup_restore_ui_file (&gdb_stdout); > + > + if (ui_out_redirect (uiout, fp) < 0) > + warning (_("Current output protocol does not support redirection")); > + else > + make_cleanup_ui_out_redirect_pop (uiout); > + > + gdb_stdout = fp; I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and gdb_stdtargerr like set_logging_redirect does. This is like if one uses |& instead of | . I understand someone may have a different opinion. In such case there even may be some option of the command pipe for it. > + execute_command (pipe->gdb_cmd, from_tty); > + do_cleanups (cleanup); > +} > + > +static void > +destruct_pipe (struct pipe_object *pipe) Missing function comment. > +{ > + pclose (pipe->handle); It could report if the command terminates wrongly - exit status - of the command. > + xfree (pipe); > +} > + > +static void > +pipe_command (char *arg, int from_tty) Missing function comment. > +{ > + struct pipe_object *pipe; > + > + pipe = construct_pipe (arg); > + if (pipe != NULL) > + { > + execute_command_to_pipe (pipe, from_tty); > + destruct_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 from a cleanup function. > + } > +} > + > +void > +_initialize_pipe (void) There should be an extra declaration of _initialize_pipe as some compilers can complain otherwise (GCC does not, I do not know which do myself). > +{ > + add_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."), > + &cmdlist); > +} There should be documentation in gdb/doc/gdb.texinfo for it. Thanks, Jan