This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] MI telnet service
- From: Gary Benson <gbenson at redhat dot com>
- To: "Abid, Hafiz" <Hafiz_Abid at mentor dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 12 Jul 2012 14:19:30 +0100
- Subject: Re: [patch] MI telnet service
- References: <EB3B29AD43CA924DA27099BC8519237629DF36@EU-MBX-03.mgc.mentorg.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/