From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8667 invoked by alias); 8 Aug 2011 10:01:02 -0000 Received: (qmail 8650 invoked by uid 22791); 8 Aug 2011 10:01:00 -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-ew0-f41.google.com (HELO mail-ew0-f41.google.com) (209.85.215.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 08 Aug 2011 10:00:33 +0000 Received: by ewy9 with SMTP id 9so363738ewy.0 for ; Mon, 08 Aug 2011 03:00:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.213.105.199 with SMTP id u7mr58279ebo.26.1312797632473; Mon, 08 Aug 2011 03:00:32 -0700 (PDT) Received: by 10.213.5.4 with HTTP; Mon, 8 Aug 2011 03:00:32 -0700 (PDT) In-Reply-To: References: <201108041029.37721.pedro@codesourcery.com> <83pqkjx578.fsf@gnu.org> Date: Mon, 08 Aug 2011 10:01: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/msg00136.txt.bz2 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. Thi= s 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 think > 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, do= 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_STDO= UT, >> + =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 ti= me. > >> + =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_run 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. > > 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