From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1EeWIV-00016e-KO for mharc-grub-devel@gnu.org; Tue, 22 Nov 2005 06:26:52 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1EeWI1-00012n-Br for grub-devel@gnu.org; Tue, 22 Nov 2005 06:26:25 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1EeUzz-00024d-LV for grub-devel@gnu.org; Tue, 22 Nov 2005 05:03:42 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1EeUkX-0000EC-IZ for grub-devel@gnu.org; Tue, 22 Nov 2005 04:47:43 -0500 Received: from [145.74.66.11] (helo=mail-cn.han.nl) by monty-python.gnu.org with esmtp (Exim 4.34) id 1EeUkX-0006s9-CB for grub-devel@gnu.org; Tue, 22 Nov 2005 04:47:41 -0500 Received: from vscan-cn.han.nl (venus.han.nl [145.74.65.6]) by mail-cn.han.nl (Postfix) with ESMTP id 94AF58D45 for ; Tue, 22 Nov 2005 10:47:39 +0100 (CET) Received: from mail-cn.han.nl ([145.74.66.11]) by vscan-cn.han.nl (venus.han.nl [145.74.65.6]) (amavisd-new, port 10024) with ESMTP id 23719-03 for ; Tue, 22 Nov 2005 10:47:38 +0100 (CET) Received: from mail1.han.nl (mail1.han.nl [145.74.103.11]) by mail-cn.han.nl (Postfix) with ESMTP id BBF5F8924 for ; Tue, 22 Nov 2005 10:47:38 +0100 (CET) Received: from localhost.localdomain (mgerards.xs4all.nl [82.92.27.129]) by mail1.han.nl (Postfix) with ESMTP id 5058CC063 for ; Tue, 22 Nov 2005 10:47:38 +0100 (CET) Mail-Copies-To: metgerards@student.han.nl To: The development of GRUB 2 References: <200511182237.33441.T.E.Baldwin99@members.leeds.ac.uk> <200511181727.24952.hollis@penguinppc.org> <200511191122.19345.T.E.Baldwin99@members.leeds.ac.uk> From: Marco Gerards Date: Tue, 22 Nov 2005 10:47:38 +0100 In-Reply-To: <200511191122.19345.T.E.Baldwin99@members.leeds.ac.uk> (Timothy Baldwin's message of "Sat, 19 Nov 2005 11:22:02 +0000") Message-ID: <87oe4ddnet.fsf@student.han.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 amavisd-new (2.2.0) at vscan-cn.han.nl Subject: Re: RISC OS rescue mode 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: Tue, 22 Nov 2005 11:26:34 -0000 Timothy Baldwin writes: Hi Timothy, Thanks a lot for your patch. This week I will have very little time, but I'll try to review it ASAP. Here are a few things I noticed while I was having a quick look at it. >> I'm a little confused about this "RISC_OS" naming. Is "RISC_OS" the name of >> the firmware that GRUB runs above? If not, I don't think that's an >> appropriate name. > > The firmware is called "RISC OS", not to be confused with "RISC/os". Putting > spaces in the filenames is asking for trouble. > > RISC OS is a co-operative multitasking operating system and is the firmware in > the systems on which it runs. Please use `risc_os' instead of `RISC_OS'. I don't like using capital names in files, we don't do that for other files. For example for IEEE 1275 we use ieee1275. Please only use capital names in case of macro's and enum's, see the GCS. > 2005-08-29 Timothy Baldwin > * boot/arm/RISC_OS/!Run,feb: New file. I would prefer another name and renaming when installing. I wonder what other people think about that. > * Makefile.in (RMKFILES): Add arm-RISC_OS.rmk and common.rmk to list. Wasn't common.rmk added already? > +# For grub_RO.img. > +grub_RO_ff8_SOURCES = kern/arm/aif_header.S \ What's RO_ff8? > +#define SEEK_SET 0 > +#define SEEK_END 2 can you use a prefix here? for example GRUB_RISC_OS_SEEK_SET. > +extern unsigned grub_RISC_OS_version, grub_RISC_OS_81C710_present; Please only put one variable declaration on a line. > +#define Cache_Control 0x280 [...] > +#define Service_PreReset 0x45 > +#define ERROR_NO_SUCH_SWI 0x1E6 Can you please change this so it is according to the GCS? And please use a prefix. Or did yo directly copy it from some other file? In that case we need to figure out the copyright issues. > +void * > +grub_memalign (grub_size_t align, grub_size_t size) > +{ > + void **mem = grub_RISC_OS_malloc (size + align + sizeof (void *)); > + if (mem == 0) > + { > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory"); > + return 0; > + } > + void **result = (void **) grub_align (align, (unsigned) (mem + 1)); > + result[-1] = mem; > + return result; > +} > + > +void * > +grub_malloc (grub_size_t size) > +{ > + void **result = grub_RISC_OS_malloc (size + sizeof (void *)); > + if (result == 0) > + { > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory"); > + return 0; > + } > + *result = result; > + return result + 1; > +} > + > +void * > +grub_realloc (void *ptr, grub_size_t size) > +{ > + void **ptr2 = ptr; > + void **result = > + grub_RISC_OS_realloc (ptr2 ? ptr2[-1] : 0, size + sizeof (void *)); > + if (result == 0) > + { > + grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory"); > + return 0; > + } > + *result = result; > + return result + 1; > +} Have you considered using the GRUB memory allocation routines? What are the advantages and disadvantages? And some general remarks: The copyright years are not always updated to 2005, is this correct? Sometimes you mentioned copyright with your name. Can that be changed? Not all C comments are formatted correctly. Always start a sentence with a capital letter and end with a `.'. Put two spaces after a sentence. A full review will follow. Thanks, Marco