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

Sergio Durigan Junior
2011-08-03T17:46:53Z

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> I have made corrections suggested by Sergio Durigan Junior in his
> code-review. Only the documentation section I am leaving for the time
> being.

Almost there :-).

> +/* List of characters that can be used as delimiter to separate out
> + gdb-command and shell command. */
> +#define PIPE_DELIMITER "|/\\'\"`#@!$%<^>-"
> +
> +typedef char *iostream_mode_t;
> +
> +#define RD_TEXT "r"
> +#define WR_TEXT "w"

Based on the discussion with Jan, I thought you would get rid of the `r'
argument, right? I'm not sure, so please correct me if I'm wrong.

> +/* Prototype of local function. */

`functions.'

> +static struct pipe_t *
> +construct_pipe (char *p)
> +{
> + struct pipe_t *pipe = NULL;
> + int found_mode = 0, pipe_opt_done = 0;
> + struct cleanup *old_chain;
> +
> + if (p != NULL && *p != '\0')
> + {
> + pipe = xmalloc (sizeof (struct pipe_t));
> + old_chain = make_cleanup (xfree, pipe);
> +
> + /* Default mode of pipe. */
> + pipe->mode = WR_TEXT;
> +
> + while (!pipe_opt_done)
> + {
> + p = skip_spaces (p);
> +
> + /* If we don't get an argument started with '-' and which is not
> + even a value associated with some option, we consider it as a
> + potential delimiter and stop parsing for further option
> + arguments. */

Thanks for fixing the indentation problems. But I still see one nit:
according to the GNU Coding Standards, whenever you have 8 spaces, you
must convert them to a TAB character. I know it is kind of a pain, but
we try to follow the convention. If you use Vim, ping me offlist and I
can provide you a code that does that.

Also, I see you have an extra space in the end of some lines. Please
remove them.

> + case 'r':
> + if (found_mode)
> + {
> + error (_("Invalid option"));
> + do_cleanups (old_chain);
> + return NULL;
> + }

Some things I'd like to comment about this.

First, I think the error message could be better, explaining what is the
invalid option.

Second, the `error' routine already calls `do_cleanups', so you don't
need to call it directly here.

Third, you don't need to return because the `error' routine takes care
of returning the prompt to the user.

> + if (pipe->dlim == '\0'
> + || strchr (PIPE_DELIMITER, pipe->dlim) == NULL)
> + {
> + error (_("Invalid delimiter '%c'"), pipe->dlim);
> + do_cleanups (old_chain);
> + return NULL;
> + }

Same comment about `error' and `do_cleanups', here and in the other
places as well.

> + if (!pipe->handle)
> + {
> + error (_("Failed to create pipe.\n%s"), strerror (errno));
> + do_cleanups (old_chain);
> + return NULL;
> + }
> + }
> +
> + return pipe;
> +}

Before returning, you must call `discard_cleanups' if everything went OK.

> +void
> +_initialize_pipe (void)
> +{
> + add_cmd ("pipe", no_class, pipe_command, _("\
> +Create pipe between gdb and shell for I/O based communication.\n\
> +Arguments are option(s) to the command, then a delimiter character, \
> +then the gdb-command and finally the shell-command.\n\
> +If no option is given, the default behavior of pipe will be to \
> +pass the gdb-command output to the shell."),
> + &cmdlist);
> +}

Is the user able to use the `-w' argument? If so, I think you should
mention is here as well.

I think that is it. Thank you for your work on this patch. I would
appreciate if some maintainer could review it too, in order to catch
things that I didn't see.

Regards,

Sergio.