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

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

On Mon, Aug 8, 2011 at 3:30 PM, Abhijit Halder
<abhijit.k.halder@gmail.com> wrote:
> 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_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
>