From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19420 invoked by alias); 12 Jul 2012 14:37:00 -0000 Received: (qmail 19404 invoked by uid 22791); 12 Jul 2012 14:36:58 -0000 X-SWARE-Spam-Status: No, hits=-3.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Jul 2012 14:36:45 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SpKVY-0001B7-Pa from Hafiz_Abid@mentor.com ; Thu, 12 Jul 2012 07:36:44 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 12 Jul 2012 07:35:50 -0700 Received: from EU-MBX-03.mgc.mentorg.com ([169.254.2.57]) by SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) with mapi id 14.01.0289.001; Thu, 12 Jul 2012 15:36:43 +0100 From: "Abid, Hafiz" To: Gary Benson CC: "gdb-patches@sourceware.org" Subject: RE: [patch] MI telnet service Date: Thu, 12 Jul 2012 14:37:00 -0000 Message-ID: References: <20120712131930.GG29236@redhat.com> In-Reply-To: <20120712131930.GG29236@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-07/txt/msg00162.txt.bz2 > -----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 >=20 > Hi, >=20 > This isn't a formal review, just some formatting issues: >=20 > 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); >=20 > Missing space before '('. >=20 > > +/* Hooks which are called at various points during telnet > > + command processing. */ >=20 > Need double space before '*/' >=20 > > + memset ((char *)&serv, 0, sizeof (serv)); >=20 > Missing space after cast. >=20 > > + if (bind (telnet_s, (const struct sockaddr *)&serv, sizeof (serv)) > =3D=3D -1) >=20 > Missing space after cast. >=20 > > + error (_("Error: port %d can't be bind"), port); >=20 > I'm not sure about this error message. Maybe it should be "bound", or > maybe the whole error should say something else. >=20 > > + if(str_file !=3D NULL) >=20 > Missing space before '('. >=20 > > + if(get_answer_hook) >=20 > Missing space before '('. >=20 > > + /* Check the first letter in the answer. */ > > + switch (cmd) > > + { > > + case 'y': > > + case 'Y': > > + retval =3D 1; > > + break; > > + > > + case 'n': > > + case 'N': > > + default: > > + retval =3D 0; > > + break; > > + } > > + } >=20 > "break"s should line up with the lines above. >=20 > > + /* If this is a valid command then add it to history. */ > > + if (*line !=3D '\0' ) >=20 > Extra space before ')'. >=20 > > + if ( (hist !=3D NULL) && (hist->line !=3D NULL) && (*hist->line = !=3D > '\0') ) >=20 > Extra spaces between '( (' and ') )'. >=20 > > + /* figure out current interpreter */ > > + if (current_interp_named_p (INTERP_MI)) > > + { > > + old_interp =3D interp_lookup (INTERP_MI); > > + } >=20 > 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. >=20 > > + reply_msg ("Error: unexpectedly failed to use CLI the > interpreter\r\n"); >=20 > This line is probably too long. >=20 > > + s =3D accept (telnet_s, (struct sockaddr *)&client, &len); >=20 > Missing space after cast. >=20 > > + if((fd !=3D NULL) && (*fd !=3D -1) ) >=20 > Missing space before '(' and extra space before ')'. The inner > parentheses are superfluous and should probably be removed. >=20 > > +/* 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) >=20 > Missing newline between comment and function. >=20 > > + for(i=3D0; i < len; i++) >=20 > Missing space before '(' and around '=3D'. >=20 > > + if(msg[i] =3D=3D '\n') >=20 > Missing space before '('. >=20 > > + for (i =3D (size - 1); i >=3D0; i--) >=20 > Extra parens, and missing space after '>=3D'. >=20 > > + if ( (buf[i] !=3D '\n') && (buf[i] !=3D '\r') ) >=20 > Extra parens and space. I would write this line like this: >=20 > if (buf[i] !=3D '\n' && buf[i] !=3D '\r') >=20 > > +static char* >=20 > Missing space before '*'. >=20 > > +/* This function is called when data is available on the socket to > read. */ >=20 > Need double space before '*/' >=20 > > +/* This function setup the hooks that are called at various points. > > + The mode parameter gives us flexibility to support other modes in > > + future. */ >=20 > Need double space before '*/' >=20 > > 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 >=20 > 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. >=20 > Cheers, > Gary >=20 > -- > http://gbenson.net/