From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1D3J9F-0003Kg-IV for mharc-grub-devel@gnu.org; Mon, 21 Feb 2005 14:23:13 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1D3J8t-00038c-CV for grub-devel@gnu.org; Mon, 21 Feb 2005 14:22:51 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1D3J8p-00036N-En for grub-devel@gnu.org; Mon, 21 Feb 2005 14:22:47 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1D3J8p-00035L-7Q for grub-devel@gnu.org; Mon, 21 Feb 2005 14:22:47 -0500 Received: from [145.74.66.11] (helo=mail-cn.han.nl) by monty-python.gnu.org with esmtp (Exim 4.34) id 1D3ItO-0000RI-9a for grub-devel@gnu.org; Mon, 21 Feb 2005 14:06:50 -0500 Received: from vscan-cn.han.nl (venus.han.nl [145.74.65.6]) by mail-cn.han.nl (Postfix) with ESMTP id 221649DA1 for ; Mon, 21 Feb 2005 20:01:10 +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 31255-06 for ; Mon, 21 Feb 2005 20:01:09 +0100 (CET) Received: from mail1.han.nl (mail1.han.nl [145.74.103.11]) by mail-cn.han.nl (Postfix) with ESMTP id E804B9D9A for ; Mon, 21 Feb 2005 20:01:08 +0100 (CET) Received: from localhost.localdomain (mgerards.xs4all.nl [82.92.27.129]) by mail1.han.nl (Postfix) with ESMTP id A2A64C05F for ; Mon, 21 Feb 2005 20:01:08 +0100 (CET) Mail-Copies-To: metgerards@student.han.nl To: The development of GRUB 2 References: <20050213165452.GA4503@miracle> From: Marco Gerards Date: Mon, 21 Feb 2005 20:01:09 +0100 In-Reply-To: <20050213165452.GA4503@miracle> (Hollis Blanchard's message of "Sun, 13 Feb 2005 10:54:52 -0600") Message-ID: <874qg5pwju.fsf@student.han.nl> User-Agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.3 (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: [patch] set prefix on PPC 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, 21 Feb 2005 19:23:06 -0000 Hollis Blanchard writes: > This code sets the GRUB "prefix" environment variable from the OF > /chosen/bootpath property. It must therefore translate between the two > syntaxes. Sven just told me bootpath does not work on the pegasos II, although it will be fixed. So the argument will still be required, I guess. I will send in a patch for that later (one that will work with this patch applied). > I believe most of this code could be moved to a kern/ieee1275/device.c > file, but that can wait until there is another supported OF platform. Right. A lot of code can be moved and shared. > I have verified that this works (a config file is loaded) from disk, and > netbooting is not broken. The subdirectory stuff works as well. Ok. I still have to test it... :/ > 2005-02-13 Hollis Blanchard [...] > (GRUB_PARSE_PARTITION): Likewise. Just make this a new enum. So name the enum and say "New enum `foo'.". See my comment below. > Index: kern/powerpc/ieee1275/init.c [...] > +/* Translate an OF filesystem path (separated by backslashes), into a GRUB > + path (separated by forward slashes). */ > +static void > +translate_path (char *filepath) Can you add a prefix? > +static void > +grub_set_prefix (void) > +{ > + char bootpath[64]; /* XXX check length */ You could get the property length and use that. It would be clearer. See my patch for passing the prefix as an argument for an example of this. I think this was done like this before, but that should be changed as well. :) > Index: kern/powerpc/ieee1275/openfw.c [...] > +enum { Please put the { on a new line. > + GRUB_PARSE_FILENAME, > + GRUB_PARSE_PARTITION, > +}; I think there can be some problems with unnamed enums. I am not sure though... > + > /* Walk children of 'devpath', calling hook for each. */ > grub_err_t > grub_children_iterate (char *devpath, > @@ -64,7 +69,7 @@ grub_children_iterate (char *devpath, > if (actual == -1) > continue; > > - grub_sprintf(fullname, "%s/%s", devpath, childname); > + grub_sprintf (fullname, "%s/%s", devpath, childname); > > alias.type = childtype; > alias.path = childpath; > @@ -199,3 +204,133 @@ grub_claimmap (grub_addr_t addr, grub_si > > return 0; > } > + > +/* Get the device arguments of the Open Firmware device specifier `path'. */ > +static char * Device arguments are like the partition number or so? > +static char * > +grub_ieee1275_parse_args (const char *path, int field) > +{ > + char type[64]; /* XXX check size. */ Same stuff as before. :) > + char *device = grub_ieee1275_get_devname (path); > + char *args = grub_ieee1275_get_devargs (path); > + char *ret = 0; > + grub_ieee1275_phandle_t dev; > + > + if (!args) > + /* Shouldn't happen. */ > + return 0; If I understood your code correctly it can't happen at all, in that case the test would be useless. > + > + /* We need to know what type of device it is in order to parse the full > + file path properly. */ > + if (grub_ieee1275_finddevice (device, &dev)) > + { > + grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Device %s not found\n", device); > + goto out; > + } > + if (grub_ieee1275_get_property (dev, "device_type", &type, sizeof type, 0)) > + { > + grub_error (GRUB_ERR_UNKNOWN_DEVICE, > + "Device %s lacks a device_type property\n", device); > + goto out; > + } > + > + if (!grub_strcmp ("block", type)) > + { > + /* Example: "disk:,". */ Is it always like this without exceptions? Will this always be an alias or will this be copied from "boot"? > + ret = grub_malloc (grub_strlen (filepath) + 1); > + /* Make sure filepath has leading backslash. */ > + if (filepath[0] != '\\') > + grub_sprintf (ret, "\\%s", filepath); > + else > + grub_strcpy (ret, filepath); Why is this required? > +out: Please use "fail:", just to be consistent. > +char * > +grub_ieee1275_get_dev_encoding (const char *path) > +{ > + char *device = grub_ieee1275_get_devname (path); > + char *partition = grub_ieee1275_parse_args (path, GRUB_PARSE_PARTITION); > + char *encoding; > + > + if (partition) > + { > + unsigned int partno = grub_strtoul (partition, 0, 0); > + partno--; /* GRUB partition numbering is 0-based. */ Right. But how can you be sure both match? > + > + /* XXX Assume partno will only require two bytes? */ > + encoding = grub_malloc (grub_strlen (device) + 5); Can't you just calculate this? Otherwise you can better allocate a few bytes too much. Thanks, Marco