From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17438 invoked by alias); 3 Aug 2011 17:46:53 -0000 Received: (qmail 17428 invoked by uid 22791); 3 Aug 2011 17:46:51 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Wed, 03 Aug 2011 17:46:35 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p73HkYYR022018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 3 Aug 2011 13:46:34 -0400 Received: from psique ([10.3.112.5]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p73HkTRl025729; Wed, 3 Aug 2011 13:46:31 -0400 From: Sergio Durigan Junior To: Abhijit Halder Cc: gdb-patches@sourceware.org, Jan Kratochvil Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. References: <20110802065411.GA23915@host1.jankratochvil.net> <20110802154450.GA19759@host1.jankratochvil.net> Date: Wed, 03 Aug 2011 17:46:00 -0000 In-Reply-To: (Abhijit Halder's message of "Wed, 3 Aug 2011 12:35:46 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg00051.txt.bz2 Abhijit Halder writes: > I have made corrections suggested by Sergio Durigan Junior in his > code-review. Only the documentation section I am leaving for the time > being. Almost there :-). > +/* List of characters that can be used as delimiter to separate out > + gdb-command and shell command. */ > +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>-" > + > +typedef char *iostream_mode_t; > + > +#define RD_TEXT "r" > +#define WR_TEXT "w" Based on the discussion with Jan, I thought you would get rid of the `r' argument, right? I'm not sure, so please correct me if I'm wrong. > +/* Prototype of local function. */ `functions.' > +static struct pipe_t * > +construct_pipe (char *p) > +{ > + struct pipe_t *pipe = NULL; > + int found_mode = 0, pipe_opt_done = 0; > + struct cleanup *old_chain; > + > + if (p != NULL && *p != '\0') > + { > + pipe = xmalloc (sizeof (struct pipe_t)); > + old_chain = make_cleanup (xfree, pipe); > + > + /* Default mode of pipe. */ > + pipe->mode = WR_TEXT; > + > + while (!pipe_opt_done) > + { > + p = skip_spaces (p); > + > + /* If we don't get an argument started with '-' and which is not > + even a value associated with some option, we consider it as a > + potential delimiter and stop parsing for further option > + arguments. */ Thanks for fixing the indentation problems. But I still see one nit: according to the GNU Coding Standards, whenever you have 8 spaces, you must convert them to a TAB character. I know it is kind of a pain, but we try to follow the convention. If you use Vim, ping me offlist and I can provide you a code that does that. Also, I see you have an extra space in the end of some lines. Please remove them. > + case 'r': > + if (found_mode) > + { > + error (_("Invalid option")); > + do_cleanups (old_chain); > + return NULL; > + } Some things I'd like to comment about this. First, I think the error message could be better, explaining what is the invalid option. Second, the `error' routine already calls `do_cleanups', so you don't need to call it directly here. Third, you don't need to return because the `error' routine takes care of returning the prompt to the user. > + if (pipe->dlim == '\0' > + || strchr (PIPE_DELIMITER, pipe->dlim) == NULL) > + { > + error (_("Invalid delimiter '%c'"), pipe->dlim); > + do_cleanups (old_chain); > + return NULL; > + } Same comment about `error' and `do_cleanups', here and in the other places as well. > + if (!pipe->handle) > + { > + error (_("Failed to create pipe.\n%s"), strerror (errno)); > + do_cleanups (old_chain); > + return NULL; > + } > + } > + > + return pipe; > +} Before returning, you must call `discard_cleanups' if everything went OK. > +void > +_initialize_pipe (void) > +{ > + add_cmd ("pipe", no_class, pipe_command, _("\ > +Create pipe between gdb and shell for I/O based communication.\n\ > +Arguments are option(s) to the command, then a delimiter character, \ > +then the gdb-command and finally the shell-command.\n\ > +If no option is given, the default behavior of pipe will be to \ > +pass the gdb-command output to the shell."), > + &cmdlist); > +} Is the user able to use the `-w' argument? If so, I think you should mention is here as well. I think that is it. Thank you for your work on this patch. I would appreciate if some maintainer could review it too, in order to catch things that I didn't see. Regards, Sergio.