From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10497 invoked by alias); 13 Aug 2011 20:51:57 -0000 Received: (qmail 10478 invoked by uid 22791); 13 Aug 2011 20:51:55 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RG,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; Sat, 13 Aug 2011 20:51:10 +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 p7DKp06T019701 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 13 Aug 2011 16:51:01 -0400 Received: from host1.jankratochvil.net (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7DKovd9006024 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 13 Aug 2011 16:50:59 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p7DKousN003339; Sat, 13 Aug 2011 22:50:56 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p7DKosT3003338; Sat, 13 Aug 2011 22:50:54 +0200 Date: Sat, 13 Aug 2011 20:51:00 -0000 From: Jan Kratochvil To: Abhijit Halder Cc: Sergio Durigan Junior , Eli Zaretskii , tromey@redhat.com, pedro@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. Message-ID: <20110813205053.GB22058@host1.jankratochvil.net> References: <201108041029.37721.pedro@codesourcery.com> <83pqkjx578.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-2022-jp 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/msg00278.txt.bz2 On Tue, 09 Aug 2011 12:42:25 +0200, Abhijit Halder wrote: [...] > --- src/gdb/pipe.c 2011-07-29 15:15:26.078048517 +0530 > +++ dst/gdb/pipe.c 2011-08-09 15:53:48.462145884 +0530 > @@ -0,0 +1,210 @@ > +/* 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" > +#include "libiberty.h" > + > +/* Structure to encapsulate all entities associated with pipe. */ > + > +struct pipe_obj > +{ > + /* The delimiter to separate out gdb-command and shell-command. This can be > + any arbitrary string without containing any whitespace. */ not containing > + char *dlim; > + > + /* The gdb-command. */ > + char *gdb_cmd; > + > + /* The shell-command. */ > + char *shell_cmd; > + > + /* The gdb-side stream pointer to the pipe. */ > + FILE *handle; > + > + /* The pex object used to create pipeline between gdb and shell. */ > + struct pex_obj *pex; > +}; > + > +/* Construct a pipe object by parsing argument to the pipe command. */ It is formal but there should be a note what is P. > + > +static struct pipe_obj * > +construct_pipe (char *p) > +{ > + char *t; > + struct pipe_obj *pipe = NULL; > + struct cleanup *cleanup; > + > + if (p == NULL) > + error (_("No argument is specified")); > + > + pipe = XCNEW (struct pipe_obj); > + cleanup = make_cleanup (xfree, pipe); > + > + pipe->dlim = p; If we should keep the command `pipe' extensible in future by the options here p[0] should be forbidden to contain '-'. Currently returned pipe_obj still references P. Either xstrdup + xfree all the strings inside pipe_obj or xstrdup P itself first (and xfree it afterwards), you can also use pipe_obj->last_field[1] string for storing P etc. This is not a bug but I find construct_pipe not useful as standalone function. Otherwise you can also inline the code inside pipe_command, pipe_obj then should contain just few/two/one entries neededed for the cleanup function destruct_pipe. Currently when pipe_obj references external data its `char *' should be in fact `const char *' but referencing external data is wrong anyway. > + > + t = skip_to_space (p); > + p = skip_spaces (t); > + > + if (*p == '\0') > + error (_("No gdb-command is specified")); > + > + *t = '\0'; > + pipe->gdb_cmd = p; > + > + for (;;) > + { > + t = skip_to_space (p); > + > + if (*t == '\0') > + error (_("No shell-command is specified")); > + > + /* Check whether the token separated by whitespace matches with > + delimiter. */ > + if (memcmp (p, pipe->dlim, (t - p)) == 0) Here is missing: && pipe->dlim[t - p] == 0 otherwise this invalid line is valid: (gdb) pipe abc print 1 ab cat > + { > + *p = '\0'; > + pipe->shell_cmd = skip_spaces (t); > + break; > + } > + > + p = skip_spaces (t); > + } > + > + discard_cleanups (cleanup); > + return pipe; > +} > + > +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not There is no parameter called P. Maybe `the pipe' should have been `PIPE' if you mean that parameter (you may not). > + display it to the screen. */ > + > +static void > +execute_command_to_pipe (struct pipe_obj *pipe, int from_tty) > +{ > + char **argv; > + struct cleanup *cleanup; > + struct ui_file *fp; > + > + argv = gdb_buildargv (pipe->shell_cmd); > + cleanup = make_cleanup_freeargv (argv); > + > + pipe->pex = pex_init (PEX_USE_PIPES, argv[0], NULL); > + > + if (pipe->pex == NULL) > + do_cleanups (cleanup); In such case do_cleanup is not enough, you have to return as it no longer makes sense to do anything here, best to just call `error'. But pex_init can never fail and one case in GDB already does not expect it could fail so even removal of the two lines would be also OK. > + > + pipe->handle = pex_input_pipe (pipe->pex, 0); > + > + if (pipe->handle == NULL) > + error (_("Failed to create pipe")); > + > + { This indentation is not correct, it should be two spaces, not four spaces. But I also do not see why to start a new block here, just move the status + err variables to the function variables block. > + int status; > + const char *err > + = pex_run (pipe->pex, > + PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT, Why PEX_STDERR_TO_STDOUT? I am not so much against it but I do not see a reason for it. > + argv[0], argv, It does not work in one of the intended modes: (gdb) pipe | print 1 | cat >/dev/null cat: >/dev/null: No such file or directory You do not have to parse ARGV yourself, one can just run "/bin/sh", "-c", COMMAND, NULL (sure no PEX_SEARCH needed then). > + NULL, NULL, > + &status); There should be an empty space between declarations and code statements. > + if (err != NULL) > + error (_("Failed to execute %s"), argv[0]); It has returned the ERR erroe message, thus ERR should be also printed, together with STATUS interpreted like errno. > + > + do_cleanups (cleanup); Formally do_cleanups indentation should match creating of that cleanup, if possible, like it is here. But I think after the other comments this cleanup will no longer be there anyway. > + } > + > + /* GDB_STDOUT should be better already restored during these > + restoration callbacks. */ > + 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); > + make_cleanup_restore_ui_file (&gdb_stderr); > + make_cleanup_restore_ui_file (&gdb_stdlog); > + make_cleanup_restore_ui_file (&gdb_stdtarg); > + make_cleanup_restore_ui_file (&gdb_stdtargerr); > + > + if (ui_out_redirect (uiout, fp) < 0) It has been renamed to current_uiout. > + warning (_("Current output protocol does not support redirection")); > + else > + make_cleanup_ui_out_redirect_pop (uiout); > + > + gdb_stdout = fp; > + gdb_stderr = fp; > + gdb_stdlog = fp; > + gdb_stdtarg = fp; > + gdb_stdtargerr = fp; > + execute_command (pipe->gdb_cmd, from_tty); > + do_cleanups (cleanup); > +} > + > +/* Destruct pipe object. */ Here should be described ARG and that it is compliant to the cleanup handler prototype. > + > +static void > +destruct_pipe (void *arg) > +{ > + struct pipe_obj *pipe = (struct pipe_obj *)arg; GCS (GNU Coding Style) says: (struct pipe_obj *) arg; > + > + if (pipe->handle != NULL) > + fclose (pipe->handle); > + > + if (pipe->pex != NULL) > + { > + int status; > + > + /* Wait till the process in the other side of the pipe completes its > + job before closing its file descryptors. */ My English is far from perfect but at least use "on the other side" and "descriptors", I would write: /* Wait till the process on the other right side of the pipe completes its job before killing it due to the PIPE close. */ > + pex_get_status (pipe->pex, 1, &status); Here could be a warning on non-successful pipe command exit. > + pex_free (pipe->pex); > + } > + > + xfree (pipe); > +} > + > +/* Execute the pipe command. */ > + > +static void > +pipe_command (char *arg, int from_tty) > +{ > + struct pipe_obj *pipe = construct_pipe (arg); > + > + if (pipe != NULL) construct_pipe never returns NULL. It would be questionable whether write an error message in such case etc. but it is irrelevant. > + { > + struct cleanup *cleanup = make_cleanup (destruct_pipe, pipe); > + > + execute_command_to_pipe (pipe, from_tty); > + do_cleanups (cleanup); > + } > +} > + > +/* Module initialization. */ > + > +void > +_initialize_pipe (void) $ make CFLAGS=-Wmissing-prototypes pipe.o pipe.c:203:1: error: no previous prototype for ‘_initialize_pipe’ [-Werror=missing-prototypes] I do not know why GDB does not use -Wmissing-prototypes by default but the code is ready for it, such as: /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_linux_nat; void _initialize_linux_nat (void) > +{ > + add_cmd ("pipe", no_class, pipe_command, _("\ > +Create pipe to pass gdb-command output to the shell for processing.\n\ > +Arguments are a delimiter, followed by a gdb-command, then the same delimiter \ > +again and finally a shell-command."), > + &cmdlist); > +} And the documentation, NEWS entry and testcase is missing as already noted. Thanks, Jan