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

[Abhijit Halder <abhijit dot k dot halder at gmail dot com>]

On Fri, Aug 5, 2011 at 1:59 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 05 Aug 2011 09:58:49 +0200, Abhijit Halder wrote:
>> --- src/gdb/pipe.c ? ?2011-07-29 15:15:26.078048517 +0530
>> +++ dst/gdb/pipe.c ? ?2011-08-05 13:10:51.411046880 +0530
>> @@ -0,0 +1,161 @@
>> +/* Everything about pipe, for GDB.
>> +
>> + ? Copyright (C) 2011 Free Software Foundation, Inc.
>> +
>> + ? This file is part of GDB.
>> +
>> + ? This program is free software; you can redistribute it and/or modify
>> + ? it under the terms of the GNU General Public License as published by
>> + ? the Free Software Foundation; either version 3 of the License, or
>> + ? (at your option) any later version.
>> +
>> + ? This program is distributed in the hope that it will be useful,
>> + ? but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + ? MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>> + ? GNU General Public License for more details.
>> +
>> + ? You should have received a copy of the GNU General Public License
>> + ? along with this program. ?If not, see <http://www.gnu.org/licenses/>. ?*/
>> +
>> +#include "defs.h"
>> +#include <ctype.h>
>> +#include "gdb_string.h"
>> +#include "ui-file.h"
>> +#include "ui-out.h"
>> +#include "cli/cli-utils.h"
>> +#include "gdbcmd.h"
>> +
>> +/* List of characters that can be used as delimiter to separate out
>> + ? gdb-command and shell command. ?*/
>> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>"
>
> I see you are not fond of redirections but at least one now will be able to:
> ? ? ? ?(gdb) pipe | command | cat >file
>
> Why there should be such explicitly limited delimiter choice? ?Couldn't the
> pipe command default to '|' and otherwise take an arbitrary first word?
> (The word should never start with '-' to have the options extension possibility
> in the future.) ?That is to permit:
> ? ? ? ?(gdb) pipe info threads | less
> ? ? ? ?(gdb) pipe : print 1 | 2 : less
> ? ? ? ?(gdb) pipe FOO print 1 | 2 FOO less
> ? ? ? ?etc.
>
I am not sure whether this restriction is meaningful. Ideally we
should not support any alpha-numeric character as a delimiter just
because of readability purpose. e.g.
(gdb) pipe dthread apply all btdvim -
Here d is delimiter. I don't think the above one is acceptable. Please
suggest me if we simply can put a restriction of not using any
alpha-numeric character as delimiter and that will do.
Secondly I believe by FOO you meant a single character and not a
string. Between delimiter and command there is no restriction of
having any white-space.
>
>> +
>> +/* The mode of stream operation. ?*/
>> +typedef char *iostream_mode_t;
>> +
>> +/* At present we support only write mode of operations to the pipe, i.e.,
>> + ? gdb-command can only write to the pipe whose other terminal is owned by the
>> + ? shell. In future we may start supporting read mode of operations as well.
>> + ? But at present there is no need for that. ?*/
>> +#define WR_TEXT "w"
>> +
>> +struct pipe_object
>
> Missing struct comment.
>
I thought the comment on individual fields reveal enough information
to a developer. In existing code also I have seen similar practice in
several places. Please suggest me whether I need to put some valuable
comment here.
>> +{
>> + ?/* The shell-command. ?*/
>> + ?char *shell_cmd;
>> +
>> + ?/* The gdb-command. ?*/
>> + ?char *gdb_cmd;
>> +
>> + ?/* The delimiter to separate out gdb-command and shell-command. ?*/
>> + ?char dlim;
>> +
>> + ?/* The supported mode of stream operations on gdb-end pipe. ?*/
>> + ?iostream_mode_t mode;
>
> It is not redundant for the current code.
>
Yes it is. I thought it is just a better encapsulation. In future if
we are going to support any new mode (e.g. read mode) the mode will
come as an option. Then it will be a good design to keep that
information inside this structure.
>> +
>> + ?/* The gdb-end stream pointer to the pipe. ?*/
>> + ?FILE *handle;
>> +};
>> +
>> +/* Prototype of local functions. ?*/
>> +
>> +static struct pipe_object *construct_pipe (char *);
>> +
>> +static void execute_command_to_pipe (struct pipe_object *, int);
>> +
>> +static void destruct_pipe (struct pipe_object *);
>> +
>> +static void pipe_command (char *, int);
>
> All these prototypes are redundant as the functions definitions precedes first
> use.
Just a good practice.
>
>> +
>> +static struct pipe_object *
>> +construct_pipe (char *p)
>
> Missing function comment.
>
From function name hope things are revealed. Please suggest if
comments are really useful.
>> +{
>> + ?struct pipe_object *pipe = NULL;
>> + ?struct cleanup *old_chain;
>> +
>> + ?if (p != NULL && *p != '\0')
>
> P can never be NULL, and if it could be it should be rather
> ? ? ? ?gdb_assert (p != NULL);
Yes I agree.
> That *p != '\0' I also do not understand, it should print an error message in
> such case IMO.
In the following case *p will be '\0'.
(gdb)pipe ||echo "Do nothing"
This can be consider not an error. (Please suggest)
Again, if so, we should not go ahead (going ahead will not break
anything, just incur extra overhead in memory allocation and
de-allocation)
>
>
>> + ? ?{
>> + ? ? ?pipe = xmalloc (sizeof (struct pipe_object));
>> + ? ? ?old_chain = make_cleanup (xfree, pipe);
>> + ? ? ?pipe->mode = WR_TEXT;
>> +
>> + ? ? ?p = skip_spaces (p);
>> + ? ? ?pipe->dlim = *p++;
>
> This can skip to unallocated memory if *p == 0.
Control will never come here under such condition.
>
>> + ? ? ?p = skip_spaces (p);
>> + ? ? ?pipe->gdb_cmd = p;
>> +
>> + ? ? ?/* Validate the delimiter from a pre-defined whitelist characters. This
>> + ? ? ?will enforce not to use special (e.g. alpha-numeric) characters. ?*/
>> + ? ? ?/* NOTE: If DLIM become null, P starts pointing to a bad memory
>> + ? ? ?location, hence before doing further processing of P we should check
>> + ? ? ?DLIM. ?*/
>> + ? ? ?if (pipe->dlim == '\0'
>> + ? ? ? || strchr (PIPE_DELIMITER, pipe->dlim) == NULL)
>> + ? ? error (_("Invalid delimiter '%c'"), pipe->dlim);
>> +
>> + ? ? ?if ((p = strchr (p, pipe->dlim)) == NULL)
>> + ? ? error (_("Found no shell command"));
>> +
>> + ? ? ?*p++ = '\0';
>> + ? ? ?pipe->shell_cmd = p;
>> +
>> + ? ? ?pipe->handle = popen (pipe->shell_cmd, pipe->mode);
>
> There was already a review note `pexecute' should be used instead.
Sorry I missed that. I will change it.
>
>
>> +
>> + ? ? ?if (!pipe->handle)
>> + ? ? error (_("Failed to create pipe.\n%s"), strerror (errno));
>
> safe_strerror. ?Also see the error messages in GDB, it is normally not printed
> on a new line, that is:
> ? ? ? ?error (_("Failed to create pipe: %s"), safe_strerror (errno));
>
I will make this change.
>
>> +
>> + ? ? ?discard_cleanups (old_chain);
>> + ? ?}
>> +
>> + ?return pipe;
>> +}
>> +
>> +static void
>> +execute_command_to_pipe (struct pipe_object *pipe, int from_tty)
>
> Missing function comment.
>
>> +{
>> + ?struct cleanup *cleanup;
>> + ?struct ui_file *fp;
>> +
>> + ?cleanup = set_batch_flag_and_make_cleanup_restore_page_info ();
>> + ?fp = stdio_fileopen (pipe->handle);
>> + ?make_cleanup_ui_file_delete (fp);
>> + ?make_cleanup_restore_ui_file (&gdb_stdout);
>> +
>> + ?if (ui_out_redirect (uiout, fp) < 0)
>> + ? ?warning (_("Current output protocol does not support redirection"));
>> + ?else
>> + ? ?make_cleanup_ui_out_redirect_pop (uiout);
>> +
>> + ?gdb_stdout = fp;
>
> I would prefer to also redirect gdb_stderr, gdb_stdlog, gdb_stdtarg and
> gdb_stdtargerr like set_logging_redirect does. ?This is like if one
> uses |& instead of | . ?I understand someone may have a different opinion.
> In such case there even may be some option of the command pipe for it.
I understand your point here, but this will break out very basic
assumption that delimiter is single character.
Can we skip this at this point? Or simply we can redirect gdb_stderror
to pipe as in most of the cases that will be equivalent to gdb_stdout.
(My understanding was that the error from gdb can be captured anyway
by error logging mechanism, the use-case of using pipe, I believe, is
when the gdb output is huge and one wants to process that huge
output.) Please suggest.
>
>
>> + ?execute_command (pipe->gdb_cmd, from_tty);
>> + ?do_cleanups (cleanup);
>> +}
>> +
>> +static void
>> +destruct_pipe (struct pipe_object *pipe)
>
> Missing function comment.
>
>> +{
>> + ?pclose (pipe->handle);
Similar to _popen there is _pclose for windows platform. Can I safely
use pclose here?
>
> It could report if the command terminates wrongly - exit status - of the
> command.
>
>
>> + ?xfree (pipe);
>> +}
>> +
>> +static void
>> +pipe_command (char *arg, int from_tty)
>
> Missing function comment.
>
>> +{
>> + ?struct pipe_object *pipe;
>> +
>> + ?pipe = construct_pipe (arg);
>> + ?if (pipe != NULL)
>> + ? ?{
>> + ? ? ?execute_command_to_pipe (pipe, from_tty);
>> + ? ? ?destruct_pipe (pipe);
>
> execute_command_to_pipe can terminate prematurely, as execute_command can call
> error(), `pipe' here would get leaked, destruct_pipe should be called from a
> cleanup function.
Yes. I will make this change.
>
>> + ? ?}
>> +}
>> +
>> +void
>> +_initialize_pipe (void)
>
> There should be an extra declaration of _initialize_pipe as some compilers can
> complain otherwise (GCC does not, I do not know which do myself).
I believe that is there in framework. init.c, created at runtime, does
contain this. Please correct me if I am wrong.
>
>> +{
>> + ?add_cmd ("pipe", no_class, pipe_command, _("\
>> +Create pipe to pass gdb-command output to the shell for processing.\n\
>> +Arguments are a delimiter character, followed by a gdb-command, \
>> +followed by a shell-command."),
>> + ? ? ? ?&cmdlist);
>> +}
>
>
> There should be documentation in gdb/doc/gdb.texinfo for it.
Sure, I am presently working on it.
>
>
> Thanks,
> Jan
>