Re: [patch] MI telnet service

Gary Benson
2012-07-12T13:19:53Z

Hi,

This isn't a formal review, just some formatting issues:

Abid, Hafiz wrote:
> +/* Accept client connection and register it
> + with event_loop to get data from telnet client. */
> +static void telnet_accept_handler(int err, gdb_client_data);

Missing space before '('.

> +/* Hooks which are called at various points during telnet
> + command processing. */

Need double space before '*/'

> + memset ((char *)&serv, 0, sizeof (serv));

Missing space after cast.

> + if (bind (telnet_s, (const struct sockaddr *)&serv, sizeof (serv)) == -1)

Missing space after cast.

> + error (_("Error: port %d can't be bind"), port);

I'm not sure about this error message. Maybe it should be "bound", or
maybe the whole error should say something else.

> + if(str_file != NULL)

Missing space before '('.

> + if(get_answer_hook)

Missing space before '('.

> + /* Check the first letter in the answer. */
> + switch (cmd)
> + {
> + case 'y':
> + case 'Y':
> + retval = 1;
> + break;
> +
> + case 'n':
> + case 'N':
> + default:
> + retval = 0;
> + break;
> + }
> + }

"break"s should line up with the lines above.

> + /* If this is a valid command then add it to history. */
> + if (*line != '\0' )

Extra space before ')'.

> + if ( (hist != NULL) && (hist->line != NULL) && (*hist->line != '\0') )

Extra spaces between '( (' and ') )'.

> + /* figure out current interpreter */
> + if (current_interp_named_p (INTERP_MI))
> + {
> + old_interp = interp_lookup (INTERP_MI);
> + }

There are a few places in this file where a single line following an
'if' is enclosed in '{}'. I don't know if this is ok or not, but it
looks funny to me.

> + reply_msg ("Error: unexpectedly failed to use CLI the interpreter\r\n");

This line is probably too long.

> + s = accept (telnet_s, (struct sockaddr *)&client, &len);

Missing space after cast.

> + if((fd != NULL) && (*fd != -1) )

Missing space before '(' and extra space before ')'. The inner
parentheses are superfluous and should probably be removed.

> +/* Sends the reply to remote client with extra '\r' character
> + for every '\n' in the msg so that it will show up correctly on
> + telnet console. */
> +static void
> +reply_msg_with_carriage_return (const char *msg)

Missing newline between comment and function.

> + for(i=0; i < len; i++)

Missing space before '(' and around '='.

> + if(msg[i] == '\n')

Missing space before '('.

> + for (i = (size - 1); i >=0; i--)

Extra parens, and missing space after '>='.

> + if ( (buf[i] != '\n') && (buf[i] != '\r') )

Extra parens and space. I would write this line like this:

if (buf[i] != '\n' && buf[i] != '\r')

> +static char*

Missing space before '*'.

> +/* This function is called when data is available on the socket to read. */

Need double space before '*/'

> +/* This function setup the hooks that are called at various points.
> + The mode parameter gives us flexibility to support other modes in
> + future. */

Need double space before '*/'

> diff -N gdb/testsuite/gdb.mi/mi-telnet.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ gdb/testsuite/gdb.mi/mi-telnet.c 12 Jul 2012 10:35:17 -0000

The indentation in this file is completely wrong. There's also
a lot of missing space before '(' for function calls and missing
space after casts.

Cheers,
Gary

--