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

[Abhijit Halder <abhijit dot k dot halder at gmail dot 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