From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2104 invoked by alias); 12 Jul 2012 13:19:53 -0000 Received: (qmail 2094 invoked by uid 22791); 12 Jul 2012 13:19:51 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,SPF_FAIL X-Spam-Check-By: sourceware.org Received: from gbenson.demon.co.uk (HELO gbenson.demon.co.uk) (80.177.220.214) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Jul 2012 13:19:35 +0000 Date: Thu, 12 Jul 2012 13:19:00 -0000 From: Gary Benson To: "Abid, Hafiz" Cc: "gdb-patches@sourceware.org" Subject: Re: [patch] MI telnet service Message-ID: <20120712131930.GG29236@redhat.com> Mail-Followup-To: "Abid, Hafiz" , "gdb-patches@sourceware.org" References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-IsSubscribed: yes 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/msg00160.txt.bz2 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/