Re: [PATCH] ieee1275: split up grub_machine_get_bootlocation

[Eric Snowberg]

> On Mar 8, 2018, at 4:19 AM, Daniel Kiper <address@hidden> wrote:
> 
> 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.

Here is the follow up patch I’m preparing to send that will use it (look at the 
add_bootpath function):

https://github.com/esnowberg/grub2-sparc/commit/94e4e64c98bce8f43ced440928b3d1e83b36ed11.patch

This patch also uses the other additions I’ve sent recently.

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

I thought the compiler complained about that, but I’ll try to remove it.

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

I’ll make this change.

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

The returned value is in the boot_dev.  Just to clarify, you want me to change 
the function prototype to return char * too?