Re: RFC: Introduce remote console for CLI interpreter via telnet

[Joel Brobecker <brobecker at adacore dot com>]

Grigory,

First, thank you for sending the patch.

> Attached is the patch that introduces telnet service which can
> accept and execute CLI commands from the remote user.

No one commented on the desireability of such a feature.
I'm a bit indifferent, to tell you the truth, and it would
be nice to hear from others. For now, I've only skimmed over
the patch...

You'll see that there are a lot of little rules, mostly regarding
the coding style used in GDB, and more generally in GNU projects.
I've mentioned each one of the only once, rather than picking on
them each and every time.

Before you go ahead and fix everything, I think you should wait
to see if others show interest as well. But I thought that the
review would save another more competent maintainer a little
bit of time. So here are my comments....

> It is controlled by two additional MI commands:
> "-start-telnet-service [port]" and "-stop-telnet-service".
[...]
> Current implementation assumes that the inferior is executed in
> async mode and requires "set target-async" to be set to "on".
> Two additional options should also be turned on if remote user is
> going to issue interactive commands (like "commands", etc). They are
> "set confirm on" and "set interactive on" and can be set on-the-fly
> by the remote user.

This is something I wanted to ask when I first heard about the
feature, so these are the operational constraints. OK. I am not
the right maintainer to really look at that, so that'll probably
explain my next question: Are there any potential problems with
concurrency? What if we enter commands from both interfaces at
the same time?

> +# Enable MI telnet.
> +AC_ARG_ENABLE(gdbmitel,
> +AS_HELP_STRING([--enable-gdbmitel], [enable remote interface over telnet]),

Because the telnet interface has to be explicitly enable to be active,
I would imagine that we don't need to provide a configure-time option.

If you do, though, I'd rather it was on by default, rather than
the opposite.  Otherwise, it's going to remain an obscure feature,
and no one is ever going to have it built in when they need it.
And I personally find the name of the option a little obscure.
I'd rather have --enable-gdb-mi-telnet, or --enable-remote-mi-access,
or something like that.

And finally, don't add -DENABLE_SOMETHING to the compilation flags.
Use config.h (generated from config.in by the configure script).

All these issues would go away if we just decide to remove the option
of configuring this feature out.

> Index: b/gdb/mi/mi-telnet.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ b/gdb/mi/mi-telnet.c	2011-11-11 16:20:48.582148706 +0300
> @@ -0,0 +1,426 @@
> +/* MI telnet support
> +
> +   Copyright (C) 2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009, 2010,
> +   2011 Free Software Foundation, Inc.

I am guessing that the only valid copyright year is 2011.

I am also wondering whether you might be able to use serial.h and
ser-tcp.h instead of using sockets directly.  This would solve
the portability problem, since you don't have BSD sockets on Windows
(we use Winsock instead). I haven't delved in that layer much,
but I think it's worth a look.

> +/*
> +  MI telnet support allows remote user to connect to gdb using telnet.
> +  GDB should be built with "--enable-gdbmitel=yes" option to enable MI telnet.

We typically start the first line of the comment on the same line as
the opening '/*'.

> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +
> +#include "defs.h"

"defs.h" should always be the first include. Move the fisrt two
includes down.  I'm concerned about including <netinet/in.h>.
For sure you have to conditionalize it with #ifdef HAVE_NETINET_IN_H,
which means the addition of a configure check.

> +#define DEFAULT_PORT 9950
> +#define MAX_CMD_LENGTH 65536
> +
> +/* socket for accepting telnet connections */
> +static int telnet_s = -1;
> +
> +/* client connection descriptor */
> +static int client_fd = -1;
> +
> +/* buffer for received command and for intermediate interactive questions */
> +static char cmd_buf[MAX_CMD_LENGTH];
> +static char cmd_buf_intermediate[MAX_CMD_LENGTH];
> +
> +/* for output redirecting */
> +static struct ui_file *str_file = NULL;

Globals like these are bad form. I understand that it might be necessary
in order to maintain a state of the feature, but at least put them all
in one structure. That way, it looks like we're manipulating *one*
global object instead of a collection of globals. And the day we have
a way to avoid that global, all the date will alreaby be in one structure.

It's also frowned upon to have hard-coded limits like you do
on the cmd_buf & cmd_buf_intermediate. Can't we find a way to
avoid this limitation?

> +/*
> + * Send the reply to remote client
> + */

First of all, THANK YOU for providing comments to the subprograms
you are implementing.  I feel that this is a required part of developing
in GDB, but not everyone follows that rule, or someones everyone follows
it a little differently from the others.

However, function documentation and comments in general are formatted
as follow:

    /* Send the reply to the remote client.
    
       This function does this, and does that, and it is wonderful,
       really.  */

We use full sentences, with an upper-case letter to start, and a period
at the end.  Always two spaces after a period.  And no '*' at the start
of each new line.

> +  if (client_fd != -1 && msg != NULL) {
> +    /* flush additional output if any */

Formatting: The curly brace should be on the next line.  And same
remark about the comment. Thus:

   if (client_fd != -1 && msg != NULL)
     {
       /* Flush additional output if any.  */

> +static const char *const async_commands[] = {
> +  "run",

Same here, curly brace on the next line, please.

> +  /* Is "&" already specified in command? */

Two spaces after the '?'.

-- 
Joel