From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1EtVdc-00069I-Kq for mharc-grub-devel@gnu.org; Mon, 02 Jan 2006 14:46:36 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1EtVdb-00068o-2M for grub-devel@gnu.org; Mon, 02 Jan 2006 14:46:35 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1EtVdZ-00068a-HH for grub-devel@gnu.org; Mon, 02 Jan 2006 14:46:34 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1EtVdZ-00068X-Ec for grub-devel@gnu.org; Mon, 02 Jan 2006 14:46:33 -0500 Received: from [194.109.24.29] (helo=smtp-vbr9.xs4all.nl) by monty-python.gnu.org with esmtp (Exim 4.34) id 1EtVef-0005yT-FA for grub-devel@gnu.org; Mon, 02 Jan 2006 14:47:41 -0500 Received: from localhost.localdomain (mgerards.xs4all.nl [82.92.27.129]) by smtp-vbr9.xs4all.nl (8.13.3/8.13.3) with ESMTP id k02JiqLS040314 for ; Mon, 2 Jan 2006 20:44:53 +0100 (CET) (envelope-from mgerards@xs4all.nl) Mail-Copies-To: mgerards@xs4all.nl To: The development of GRUB 2 References: <43B1DD7E.5090905@omniflux.com> From: Marco Gerards Date: Mon, 02 Jan 2006 20:44:53 +0100 In-Reply-To: <43B1DD7E.5090905@omniflux.com> (omniflux+lists@omniflux.com's message of "Tue, 27 Dec 2005 17:34:06 -0700") Message-ID: <878xtyjtxm.fsf@xs4all.nl> User-Agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Virus-Scanned: by XS4ALL Virus Scanner Subject: Re: [PATCH] User definable terminfo support X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jan 2006 19:46:35 -0000 Omniflux writes: Hi, Here is the review of the code that I promised. Please see my other email about generic comments. > +/* Delete terminfo definition from list. */ > +static void > +delete_definition (terminfo_t *ti) > { > - return term.name; > + terminfo_t *p, *q; Please just declare one declaration at a time. Better would be: terminfo_t *p; terminfo_t *q; > +/* Unescape a string. */ > +static char * > +unescape_string (const char *in) > +{ > + char *q, *new_string; Same here. > + new_string = (char *) grub_malloc (grub_strlen (in) + 1); /* New string will be <= current string. */ This line is way too long. Please make sure lines are 78 characters or smaller. The best thing to do here is putting the comment above this line. Same for the comments below. Most error strings below are also too long: > + if (i > 1) > + return grub_error (GRUB_ERR_INVALID_COMMAND, "add, modify, delete and view are exclusive operations."); You can put the string on a new line. If the string is still too long you can make two strings out of it, like: grub_error (FOO, "bar" "baz"); > + ti->name = grub_strdup (args[0]); Typo. > +static void > +setup_defaults (void) > +{ > + terminfo_t *ti; > + > + ti = (terminfo_t *) grub_malloc (sizeof (terminfo_t)); > + /* Do I need to test if malloc succeeded here? What do I do if it did not? */ Good one :-) In that case default could be set to NULL, if that is allowed? > + /* Default terminal definition vt100. */ > + ti->name = grub_strdup ("vt100"); > + ti->gotoxy = grub_strdup ("\e[%i%p1%d;%p2%dH"); > + ti->cls = grub_strdup ("\e[H\e[J"); > + ti->reverse_video_on = grub_strdup ("\e[7m"); > + ti->reverse_video_off = grub_strdup ("\e[m"); > + ti->cursor_on = grub_strdup ("\e[?25l"); > + ti->cursor_off = grub_strdup ("\e[?25h"); > + ti->next = 0; Thanks, Marco