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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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.

> +/* Prototype of local function.  */

`functions.'

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

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.