Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell.

[Abhijit Halder <abhijit.k.halder@gmail.com>]

On Sun, Aug 7, 2011 at 3:50 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Hi Abhijit,
>
> Some minor nits (again).
>
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> +struct pipe_obj
>> +{
>> +  /* The delimiter to separate out gdb-command and shell-command. This can be
>                                                                    ^^
Sorry, here I am not clear. Do you mean to say I should remove - in
between gdb and command?
>
> Two spaces after the period.
>
>> +  /* The pex object use to create pipeline between gdb and shell.  */
>
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.  */"
>
>> +  if (*p != '\0') *p++ = '\0';
>
> I'm not found of this style.  Please, put the assignment on the next
> line:
Yes a slip. I am modifying it.
>
>        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'.
>
Modifying this part.
>> +/* 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?
Since pex_run returns a const char * I have to assign err at declaration time.
>
>> +  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.
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