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

[Abhijit Halder <abhijit.k.halder@gmail.com>]

On Fri, Aug 5, 2011 at 3:15 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> 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).

Sure, I will make the suggested changes. Since I am very new, I was
blindly referring to some existing files. Hope I will able to learn
things fast.
>
>
> [ rest skipped now ]
>
>
>
> Thanks,
> Jan
>