RE: [patch] MI telnet service

["Abid, Hafiz" <Hafiz_Abid@mentor.com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Gary Benson
> Sent: Thursday, July 12, 2012 2:20 PM
> To: Abid, Hafiz
> Cc: gdb-patches@sourceware.org
> Subject: Re: [patch] MI telnet service
> 
> 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.

I thought we are not required to follow the convention in the test program source. Anyway, I will fix it with the rest. Thanks for reviewing.

> 
> Cheers,
> Gary
> 
> --
> http://gbenson.net/