[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [patch] set prefix on PPC

From: Hollis Blanchard
Subject: Re: [patch] set prefix on PPC
Date: Wed, 23 Feb 2005 22:40:40 -0600

On Feb 21, 2005, at 1:01 PM, Marco Gerards wrote:

+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.

All these stack-based get_property calls are an accident waiting to happen, but you've advocated them until now. :-P Instead, we need a wrapper function that returns a malloced buffer of the proper size. That's what you're suggesting open-coding here anyways.

However I would rather not do that right now. The few developers (myself included) have other things going on, and there are still significant functionality gaps. Right now I don't want to spend time developing patches that produce lots of code churn and no user-visible improvement. Others are more than welcome to, of course. :)

+/* Get the device arguments of the Open Firmware device specifier `path'. */
+static char *

Device arguments are like the partition number or so?

Yes. OF "node names" contain two parts, the device path and the device arguments. The device arguments are everything following a colon, and the syntax of the arguments is device-specific (but by convention they are separated by commas).

In the case of a node name like "disk:6,grubof", "6,grubof" are the device arguments.

I used the term "device specifier" incorrectly the comments; I will correct that. (IEEE1275 defines its terms at the beginning, which is quite helpful...)

+  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.

That is not the case:

+grub_ieee1275_get_devargs (const char *path)
+  char *colon = grub_strchr (path, ':');
+  char *end;
+  if (! colon)
+    return 0;

So if no colon is present, grub_ieee1275_get_devargs returns NULL (ahem "0" because we're silly), and then 'args' is NULL, and then we fail.

As a separate issue, I *believe* that bootpath should always be a fully-expanded node name, and so always contain a colon, but that depends on well-behaved firmware. Accordingly, I don't think a sanity check here is unreasonable.

+ /* 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:<partition number>,<file name>".  */

Is it always like this without exceptions?  Will this always be an
alias or will this be copied from "boot"?

Please see section 3.5, "Standard system nodes". From the /chosen table, we can see that bootpath contains a "device path". By definition that does not include the device arguments; it seems to have been the intent of the spec that the device arguments belong in bootargs.

However, as we can see experimentally, at least Apple firmware includes the device arguments in bootpath. My briQ's firmware does not, at least for netbooting:
        ok boot net:,grubof.chrp
        grub> suspend
        ok dev /chosen
        ok .properties
        bootargs              "briqboot"
        bootpath              "/address@hidden/address@hidden"
... where "briqboot" is the contents of the the boot-file environment variable. This is likely the behavior Sven said was broken.

+             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?

In OF, it is possible to boot from either "disk:foo" or "disk:\foo" (both are the same file).

However, our filesystem code fails when accessing a path like "(hd0,0)//foo", and it also fails on "(hd0,0)foo". The above code handles either case by ensuring that the resulting path always has a leading backslash (later translated into a forward slash before being passed to our filesystem code).

+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?

Eh? OF partition numbers are 1-based. To convert to GRUB's 0-based numbering, we subtract one. How could that not "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.

How exactly would you like to calculate what sprintf will do before it does it? We have no snprintf (with "sophisticated" return values), and I don't think it's worth the effort either.

I will change the 5 to an 8 if it makes you happy, though I would be very surprised to see a disk with partitions in even the (base 10) triple digits.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]