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 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
>