Re: [PATCH] An implementation of pipe to send gdb command output to the shell

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> I have implemented a feature which will allow one to pass the output
> of any gdb command to the shell for further processing. The syntax
> will be as follows:
> (gdb) gdb command | `{shell command}`

Thanks for working on this.  I have made comments below (nit-picking).

> @@ -358,6 +359,71 @@ prepare_execute_command (void)
>    return cleanup;
>  }
>  
> +char *
> +parse_for_shell_command (char *p)
> +{

If this function is not going to be exported through header files, it
should be static.  Also, you should add a comement before the function.

> +  char *sh_cmd, *cpos, *spos, *epos;
> +  int quote_cnt = 0;
> +
> +  if ((sh_cmd = strchr (p, '|')) != NULL)

This is more stylish, but we try to avoid excessive parenthesis (at
least I do), and also checks and attributions on the same line.
Something like:

     sh_cmd = strchr (p, '|');
     if (sh_cmd)
       {
         ...

> +    {
> +      spos = p;
> +      epos = p + strlen (p) - 1;
> +
> +      for (;;)
> +    {

The braces are misplaced.  It should be:

         for (;;)
           {
             ...

> +      for (cpos = spos; (cpos = memchr (cpos, '"', (sh_cmd-cpos))) != NULL; cpos++)

This line is too long.  You could break it on the semi-colons.

> +    cpos = spos;
> +    while (isspace(*cpos))
> +      cpos++;

Use gdb/cli/cli-utils.c:skip_spaces instead.

> +    if (*cpos != '`' || *(cpos+1) != '{')
> +      return parse_for_shell_command (cpos);

I think you have check if `cpos' and `cpos + 1' are not NULL in this
case, before dereferencing them.

> +    if (*cpos != '`' || *(cpos-1) != '}')
> +      return parse_for_shell_command (cpos);

Likewise.

> +/* Run execute_command for P and FROM_TTY. Write output in pipe, 
> +   do not display it to the screen.  */
> +
> +void
> +execute_command_to_pipe (char *p, int from_tty, FILE *pipe)
> +{

This function should be marked as static.

>  /* Execute the line P as a command, in the current user context.
>     Pass FROM_TTY as second argument to the defining function.  */
>  
> @@ -368,7 +434,16 @@ execute_command (char *p, int from_tty)
>    struct cmd_list_element *c;
>    enum language flang;
>    static int warned = 0;
> -  char *line;
> +  char *line, *sh_cmd;
> +
> +  if ((sh_cmd = parse_for_shell_command (p)) != NULL) 

Same comment about not doing attribution and check on the same line.

> diff -rup src//gdb/ui-file.h dst//gdb/ui-file.h
> --- src//gdb/ui-file.h	2011-05-13 22:58:20.000000000 +0530
> +++ dst//gdb/ui-file.h	2011-07-15 13:48:54.603912199 +0530
> @@ -126,6 +126,9 @@ extern struct ui_file *stdio_fileopen (F
>  /* Open NAME returning an STDIO based UI_FILE.  */
>  extern struct ui_file *gdb_fopen (char *name, char *mode);
>  
> +/* Modify the file pointer of an STDIO based UI_FILE. */

Two spaces after the dot.

> 2011-07-21 Abhijit Halder <abhijit.k.halder@symantec.com>

I think you have to add two spaces after the date, and two spaces after
your name.

Thanks again for working on this.