Re: [patch] set prefix on PPC

[Hollis Blanchard]

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.

-Hollis