From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21295 invoked by alias); 8 Aug 2011 11:15:40 -0000 Received: (qmail 21286 invoked by uid 22791); 8 Aug 2011 11:15:39 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW 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; Mon, 08 Aug 2011 11:15:12 +0000 Received: by eye22 with SMTP id 22so3161220eye.0 for ; Mon, 08 Aug 2011 04:15:11 -0700 (PDT) MIME-Version: 1.0 Received: by 10.213.105.207 with SMTP id u15mr622457ebo.140.1312802111297; Mon, 08 Aug 2011 04:15:11 -0700 (PDT) Received: by 10.213.5.4 with HTTP; Mon, 8 Aug 2011 04:15:11 -0700 (PDT) In-Reply-To: References: <201108041029.37721.pedro@codesourcery.com> <83pqkjx578.fsf@gnu.org> Date: Mon, 08 Aug 2011 11:15:00 -0000 Message-ID: Subject: Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell. From: Abhijit Halder To: Sergio Durigan Junior Cc: Eli Zaretskii , tromey@redhat.com, pedro@codesourcery.com, gdb-patches@sourceware.org, jan.kratochvil@redhat.com 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/msg00137.txt.bz2 On Mon, Aug 8, 2011 at 3:30 PM, Abhijit Halder wrote: > On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior > wrote: >> Hi Abhijit, >> >> Some minor nits (again). >> >> Abhijit Halder writes: >> >>> +struct pipe_obj >>> +{ >>> + =A0/* The delimiter to separate out gdb-command and shell-command. Th= is can be >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^^ > Sorry, here I am not clear. Do you mean to say I should remove - in > between gdb and command? >> >> Two spaces after the period. >> >>> + =A0/* The pex object use to create pipeline between gdb and shell. = =A0*/ >> > Here I guess I have put 2 space character between . (period) and */. >> Some typos: >> >> "/*The pex object used to create a pipeline between GDB and shell. =A0*/" >> >>> + =A0if (*p !=3D '\0') *p++ =3D '\0'; >> >> I'm not found of this style. =A0Please, put the assignment on the next >> line: > Yes a slip. I am modifying it. >> >> =A0 =A0 =A0 =A0if (*p !=3D '\0') >> =A0 =A0 =A0 =A0 =A0*p++ =3D '\0'; >> >> >>> + =A0for (pipe->shell_cmd =3D ""; >> >> Sorry, I didn't understand this line. =A0Is this needed? =A0If so, I thi= nk >> it's better if you do `pipe->shell_cmd =3D NULL'. >> >>> + =A0 =A0 =A0int mismatch =3D memcmp (p, pipe->dlim, (separator-p)); >> >> Space between `separator', `-' and `p'. =A0If you want, you can put this >> `memcmp' call directly on the `if' condition, and then you wouldn't need >> the braces on the `for'. >> > Modifying this part. >>> +/* Run execute_command for P and FROM_TTY. Write output to the pipe, d= o not >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 ^^ >> Two spaces after period. >> >>> + =A0if (pipe->handle =3D=3D NULL) >>> + =A0 =A0error (_("Failed to create pipe")); >>> + >>> + =A0 =A0{ >>> + =A0 =A0 =A0int status; >>> + =A0 =A0 =A0const char *err >>> + =A0 =A0 =A0 =3D pex_run (pipe->pex, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STD= OUT, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 argv[0], argv, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 NULL, NULL, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 &status); >>> + =A0 =A0 =A0if (err) >>> + =A0 =A0 error (_("Failed to execute %s"), argv[0]); >>> + >>> + =A0 =A0 =A0do_cleanups (cleanup); >>> + =A0 =A0} >> >> Why the braces? > Since pex_run returns a const char * I have to assign err at declaration = time. >> >>> + =A0if (pipe->pex) >>> + =A0 =A0{ >>> + =A0 =A0 =A0int status; >>> + >>> + =A0 =A0 =A0pex_get_status (pipe->pex, 1, &status); >>> + =A0 =A0 =A0pex_free (pipe->pex); >> >> Do you really need to call `pex_get_status' here? =A0I'm really asking, >> because I don't know about pex. > I tried without using pex_get_status, but pex_free is killing the child > process without calling waitpid (or equivalent). This is causing an > immediate exit of shell command. I may be missing something. Let me > dig out further inside code. Probably I got the answer here. In pex_free we close the std descriptors before calling pex_get_status_and_time, hence we do not get any output. I think we have to call pex_get_status before calling pex_free. >> >> Thanks for your work on this patch. >> >> Regards, >> >> Sergio. >> > > Finally thank you all for your valuable time, showing me directions > and through review of the code. Shortly I will come back with > suggested modifications. > > Regards, > Abhijit Halder >