Re: [patch] set prefix on PPC

[Marco Gerards <metgerards@student.han.nl>]

Hollis Blanchard <hollis@penguinppc.org> 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  <hollis@penguinppc.org>

[...]

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

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