Re: [PATCH] ieee1275: split up grub_machine_get_bootlocation

[Daniel Kiper]

On Wed, Mar 07, 2018 at 01:48:10PM -0800, Eric Snowberg wrote:
> Split up some of the functionality in grub_machine_get_bootlocation into
> grub_ieee1275_get_boot_dev.  This will allow for code reuse in a follow on
> patch.

I would like to see follow up patches in such cases like that one.

> Signed-off-by: Eric Snowberg <address@hidden>
> ---
>  grub-core/kern/ieee1275/init.c   |   24 ++++--------------------
>  grub-core/kern/ieee1275/openfw.c |   27 +++++++++++++++++++++++++++
>  include/grub/ieee1275/ieee1275.h |    1 +
>  3 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
> index 1259022..dff2184 100644
> --- a/grub-core/kern/ieee1275/init.c
> +++ b/grub-core/kern/ieee1275/init.c
> @@ -93,29 +93,13 @@ void (*grub_ieee1275_net_config) (const char *dev, char 
> **device, char **path,
>  void
>  grub_machine_get_bootlocation (char **device, char **path)
>  {
> -  char *bootpath;
> -  grub_ssize_t bootpath_size;
> +  char *bootpath = NULL;

I do not think that you need initialize this if...

>    char *filename;
>    char *type;
>
> -  if (grub_ieee1275_get_property_length (grub_ieee1275_chosen, "bootpath",
> -                                      &bootpath_size)
> -      || bootpath_size <= 0)
> -    {
> -      /* Should never happen.  */
> -      grub_printf ("/chosen/bootpath property missing!\n");
> -      return;
> -    }
> -
> -  bootpath = (char *) grub_malloc ((grub_size_t) bootpath_size + 64);
> -  if (! bootpath)
> -    {
> -      grub_print_error ();
> -      return;
> -    }
> -  grub_ieee1275_get_property (grub_ieee1275_chosen, "bootpath", bootpath,
> -                              (grub_size_t) bootpath_size + 1, 0);
> -  bootpath[bootpath_size] = '\0';
> +  grub_ieee1275_get_boot_dev (&bootpath);
> +  if (bootpath == NULL)

     if (! bootpath)

> +    return;
>
>    /* Transform an OF device path to a GRUB path.  */
>
> diff --git a/grub-core/kern/ieee1275/openfw.c 
> b/grub-core/kern/ieee1275/openfw.c
> index ddb7783..81c03cf 100644
> --- a/grub-core/kern/ieee1275/openfw.c
> +++ b/grub-core/kern/ieee1275/openfw.c
> @@ -561,3 +561,30 @@ grub_ieee1275_canonicalise_devname (const char *path)
>    return NULL;
>  }
>
> +void
> +grub_ieee1275_get_boot_dev (char **boot_dev)

...you return char * here...

Daniel