From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13342 invoked by alias); 6 Aug 2011 22:21:34 -0000 Received: (qmail 13333 invoked by uid 22791); 6 Aug 2011 22:21:33 -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 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, 06 Aug 2011 22:21:14 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p76ML194018577 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sat, 6 Aug 2011 18:21:01 -0400 Received: from psique ([10.3.112.5]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p76MKraa021733; Sat, 6 Aug 2011 18:20:55 -0400 From: Sergio Durigan Junior To: Abhijit Halder Cc: Eli Zaretskii , tromey@redhat.com, pedro@codesourcery.com, gdb-patches@sourceware.org, jan.kratochvil@redhat.com Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. References: <201108041029.37721.pedro@codesourcery.com> <83pqkjx578.fsf@gnu.org> Date: Sat, 06 Aug 2011 22:21:00 -0000 In-Reply-To: (Abhijit Halder's message of "Sat, 6 Aug 2011 15:28:41 +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/msg00130.txt.bz2 Hi Abhijit, Some minor nits (again). Abhijit Halder writes: > +struct pipe_obj > +{ > + /* The delimiter to separate out gdb-command and shell-command. This can be ^^ Two spaces after the period. > + /* The pex object use to create pipeline between gdb and shell. */ Some typos: "/*The pex object used to create a pipeline between GDB and shell. */" > + if (*p != '\0') *p++ = '\0'; I'm not found of this style. Please, put the assignment on the next line: if (*p != '\0') *p++ = '\0'; > + for (pipe->shell_cmd = ""; Sorry, I didn't understand this line. Is this needed? If so, I think it's better if you do `pipe->shell_cmd = NULL'. > + int mismatch = memcmp (p, pipe->dlim, (separator-p)); Space between `separator', `-' and `p'. If you want, you can put this `memcmp' call directly on the `if' condition, and then you wouldn't need the braces on the `for'. > +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not ^^ Two spaces after period. > + if (pipe->handle == NULL) > + error (_("Failed to create pipe")); > + > + { > + int status; > + const char *err > + = pex_run (pipe->pex, > + PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT, > + argv[0], argv, > + NULL, NULL, > + &status); > + if (err) > + error (_("Failed to execute %s"), argv[0]); > + > + do_cleanups (cleanup); > + } Why the braces? > + if (pipe->pex) > + { > + int status; > + > + pex_get_status (pipe->pex, 1, &status); > + pex_free (pipe->pex); Do you really need to call `pex_get_status' here? I'm really asking, because I don't know about pex. Thanks for your work on this patch. Regards, Sergio.