RE: [patch] MI telnet service

Abid, Hafiz
2012-07-12T14:37:00Z

> -----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/