Re: [patch] MI telnet service

[Gary Benson <gbenson@redhat.com>]

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

-- 
http://gbenson.net/