On Wed, Aug 3, 2011 at 11:16 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> Abhijit Halder <abhijit.k.halder@gmail.com> writes:
>
>> I have made corrections suggested by Sergio Durigan Junior in his
>> code-review. Only the documentation section I am leaving for the time
>> being.
>
> Almost there :-).
>
>> +/* List of characters that can be used as delimiter to separate out
>> +   gdb-command and shell command.  */
>> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>-"
>> +
>> +typedef char *iostream_mode_t;
>> +
>> +#define RD_TEXT "r"
>> +#define WR_TEXT "w"
>
> Based on the discussion with Jan, I thought you would get rid of the `r'
> argument, right?  I'm not sure, so please correct me if I'm wrong.
>
I thought I will keep the -r option available for future use. Just to
resemble with gdb_stdin I created read terminal of pipe. If in future
some command started using gdb_stdin, there will be no coding effort
to make use of that command with pipe.  Please suggest me whether I
should remove these (-r as well as -w) options.

>> +/* Prototype of local function.  */
>
> `functions.'
>
Sorry for this typo! I will correct this.

>> +static struct pipe_t *
>> +construct_pipe (char *p)
>> +{
>> +  struct pipe_t *pipe = NULL;
>> +  int found_mode = 0, pipe_opt_done = 0;
>> +  struct cleanup *old_chain;
>> +
>> +  if (p != NULL && *p != '\0')
>> +    {
>> +      pipe = xmalloc (sizeof (struct pipe_t));
>> +      old_chain = make_cleanup (xfree, pipe);
>> +
>> +      /* Default mode of pipe.  */
>> +      pipe->mode = WR_TEXT;
>> +
>> +      while (!pipe_opt_done)
>> +        {
>> +          p = skip_spaces (p);
>> +
>> +          /* If we don't get an argument started with '-' and which is not
>> +             even a value associated with some option, we consider it as a
>> +             potential delimiter and stop parsing for further option
>> +             arguments.  */
>
> Thanks for fixing the indentation problems.  But I still see one nit:
> according to the GNU Coding Standards, whenever you have 8 spaces, you
> must convert them to a TAB character.  I know it is kind of a pain, but
> we try to follow the convention.  If you use Vim, ping me offlist and I
> can provide you a code that does that.
>
I am still not fully familiar with GNU style. I am sending my .vimrc
to you. It will be a great help if you correct that.

> Also, I see you have an extra space in the end of some lines.  Please
> remove them.
>
>> +            case 'r':
>> +              if (found_mode)
>> +                {
>> +                  error (_("Invalid option"));
>> +                  do_cleanups (old_chain);
>> +                  return NULL;
>> +                }
>
> Some things I'd like to comment about this.
>
> First, I think the error message could be better, explaining what is the
> invalid option.
>
> Second, the `error' routine already calls `do_cleanups', so you don't
> need to call it directly here.
>
> Third, you don't need to return because the `error' routine takes care
> of returning the prompt to the user.
>
>> +      if (pipe->dlim == '\0'
>> +          || strchr (PIPE_DELIMITER, pipe->dlim) == NULL)
>> +        {
>> +          error (_("Invalid delimiter '%c'"), pipe->dlim);
>> +          do_cleanups (old_chain);
>> +          return NULL;
>> +        }
>
> Same comment about `error' and `do_cleanups', here and in the other
> places as well.
>
>> +      if (!pipe->handle)
>> +        {
>> +          error (_("Failed to create pipe.\n%s"), strerror (errno));
>> +          do_cleanups (old_chain);
>> +          return NULL;
>> +        }
>> +    }
>> +
>> +  return pipe;
>> +}
>
> Before returning, you must call `discard_cleanups' if everything went OK.
>
>> +void
>> +_initialize_pipe (void)
>> +{
>> +  add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe between gdb and shell for I/O based communication.\n\
>> +Arguments are option(s) to the command, then a delimiter character, \
>> +then the gdb-command and finally the shell-command.\n\
>> +If no option is given, the default behavior of pipe will be to \
>> +pass the gdb-command output to the shell."),
>> +           &cmdlist);
>> +}
>
> Is the user able to use the `-w' argument?  If so, I think you should
> mention is here as well.
>
> I think that is it.  Thank you for your work on this patch.  I would
> appreciate if some maintainer could review it too, in order to catch
> things that I didn't see.
>
> Regards,
>
> Sergio.
>
Please find the corrected patch.

Thanks,
Abhijit Halder