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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Fri, 05 Aug 2011 11:41:09 +0200, Abhijit Halder wrote:
> 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.

It was a typo: It IS 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.

We do not try to invent a new style.  We just try to follow the GDB style.
It took me some years to find out what the GDB style is.


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

Just follow the GDB style.


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

Just follow the GDB style (of new patches; there exists a lot of old code not
having comments at the function entry).


[ rest skipped now ]



Thanks,
Jan