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

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Fri, 2019-05-03 at 19:59 +0100, Pedro Alves wrote:
> 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.

Ok, will change to -d STRING.


> 
> > 
> > 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.
Yes, I always use "|" (I even just checked the test to be sure that I
also tested "pipe" and not only "|").
pip must be used, as pi launches an interactive python.

Also, "|" allows to do:
(gdb) some_command
...
(gdb) ||grep something_in_some_command_output
(gdb) ||tee save_some_command_output

which is better than
(gdb) pip|grep something_in_some_command_output

> 
> > 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:
That part of the code will change heavily when the separator becomes a string.
> 
> (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?
pex routines do not help to handle the exit status (and are
also not used for "shell").

However your question now makes me believe that
it is not a good idea to systematically report to the user
the exit status of the launched command.
E.g. in a shell, if you do:
   $ echo something | grep somethingelse
the shell does not tell you:
  warning: grep has exited with status 1
which is what the GDB patch does now !
I think it is up to each command to have its way to 
report success or failure.

So, I believe the correct solution is to have a convenience variable
(e.g. $_exit_status) set with the exit status of the last launched
command (either by the GDB shell or pipe command).
This convenience variable can then be looked at by the user if there
is a doubt about how the last command worked.
And more interesting, this convenience variable can be used
in user defined commands.

I will prepare a RFAv3 based on this.

Thanks for the review

Philippe