Re: [patch] set prefix on PPC

[Marco Gerards]

Hollis Blanchard <address@hidden> writes:

Hi Hollis,

Sorry for the late reply.  For some reason I did not see this email.

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

Using malloc is way better.

> 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. :)

Sure, I don't have a big problem with this in this case.  But please
don't do that all the time because that will just make a crap out of
GRUB, which is something I don't want.

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

Ok.

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

Great. :)

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

When dealing with some open firmware implementations, it is better to
be paranoid. ;)

> 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"

So we don't get an alias... hmm.

Doesn't this cause any problems?

> ... where "briqboot" is the contents of the the boot-file environment
> variable. This is likely the behavior Sven said was broken.

Ok..

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

Because not in all cases GRUB and the firmware will count partitions
the same way.  A good example is the PC partition map.  In linux
primary partitions are numbers from 1 to 4, extended partitions are
numbered from 5 (IIRC).  One other way to count these partitions is
just by starting counting from 1.

This is just an example.  There are a lot of partition table layouts
and many ways to interpret partition numbers.  I can imagine GRUB
does not always work the same as a specific firmware implementation
all the time.

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

Please do.

--
Marco