Re: [RFAv2 4/6] Implement | (pipe) command.

[Pedro Alves <palves@redhat.com>]

On 4/26/19 9:11 PM, Philippe Waroquiers wrote:
> The pipe command allows to run a GDB command, and pipe its output
> to a shell command:
>   (gdb) help pipe
>   Send the output of a gdb command to a shell command.
>   Usage: pipe [COMMAND] | SHELL_COMMAND
>   Usage: | [COMMAND] | SHELL_COMMAND
>   Usage: pipe -dX COMMAND X SHELL_COMMAND
>   Usage: | -dX COMMAND X SHELL_COMMAND
>   Executes COMMAND and sends its output to SHELL_COMMAND.
>   If COMMAND contains a | character, the option -dX indicates
>   to use the character X to separate COMMAND from SHELL_COMMAND.
>   With no COMMAND, repeat the last executed command
>   and send its output to SHELL_COMMAND.
>   (gdb)

I think that making "-dX" a single character is a mistake.  I'd rather
make that "-d STRING", so you can use use a string that is much less
likely to conflict.  Like bash herestrings.  So a user would be able
to do:

pipe -d XXXYYYXXX $command | $shell_command

I also think "-dX" without a space in between is very non-gdb-ish.
In gdb, you kind of either have "-OPT X", or "/OX", which is kind of
double "--" vs "-" with getopt, except with different leading
tokens.

> 
> For example:
>   (gdb) pipe print some_data_structure | grep -B3 -A3 something
> 
> The pipe character is defined as an alias for pipe command, so that
> the above can be typed as:
>   (gdb) | print some_data_structure | grep -B3 -A3 something
> 

I get that "it makes sense", but do you see yourself using the "|" command
in preference to "pipe"?  "pipe" can be typed as "pi", which is two
key strokes as well.  Kind of wondering whether the character could be
saved for something else in the future.  I don't have a use in mind,
just thinking out loud.

> If no GDB COMMAND is given, then the previous command is relaunched,
> and its output is sent to the given SHELL_COMMAND.
> 

> +  command = skip_spaces (command);
> +  std::string gdb_cmd (command, shell_command - command);
> +
> +  if (gdb_cmd.empty ())
> +    {
> +      repeat_previous ();
> +      gdb_cmd = skip_spaces (get_saved_command_line ());
> +      if (gdb_cmd.empty ())
> +	error (_("No previous command to relaunch"));
> +    }
> +
> +  shell_command++; /* skip the separator.  */

Uppercase "Skip".

> +  shell_command = skip_spaces (shell_command);
> +  if (*shell_command == 0)

  if (*shell_command == '\0')


> +    {
> +      if (separator == '|')

I don't think this check is 100% right, considering that you could do:

(gdb) pipe -d| SHELL_COMMAND

I.e., what you want to check is whether a separate was explicitly specified.

> +	error (_("Missing SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
> +      else
> +	error (_("Missing SHELL_COMMAND in "
> +		 "\"| -d%c COMMAND %c SHELL_COMMAND\""),
> +	       separator, separator);

Or instead of making this dynamic, just print something like:

	error (_("Missing SHELL_COMMAND.\n"
                 "Usage: \"| [-c SEP] [COMMAND] | SHELL_COMMAND\""));


> +    }
> +
> +  FILE *to_shell_command = popen (shell_command, "w");
> +
> +  if (to_shell_command == NULL)
> +    error (_("Error launching \"%s\""), shell_command);
> +
> +  try
> +    {
> +      stdio_file pipe_file (to_shell_command);
> +
> +      execute_command_to_ui_file (&pipe_file, gdb_cmd.c_str (), from_tty);
> +    }
> +  catch (const gdb_exception_error &ex)
> +    {
> +      pclose (to_shell_command);
> +      throw;
> +    }
> +
> +  int shell_command_status = pclose (to_shell_command);
> +
> +  if (shell_command_status < 0)
> +    error (_("shell command  \"%s\" errno %s"), shell_command,
> +           safe_strerror (errno));
> +  if (shell_command_status > 0)
> +    {
> +      if (WIFEXITED (shell_command_status))
> +	warning (_("shell command \"%s\" exit status %d"), shell_command,
> +		 WEXITSTATUS (shell_command_status));
> +      else if (WIFSIGNALED (shell_command_status))
> +	warning (_("shell command \"%s\" exit with signal %d"), shell_command,
> +		 WTERMSIG (shell_command_status));
> +      else
> +	warning (_("shell command \"%s\" status %d"), shell_command,
> +		 shell_command_status);
> +    }

Is there a reason this isn't using the pex routines?

Thanks,
Pedro Alves