Re: [PATCH v3 2/3] i386: Add support for loading from android bootimg

[Vladimir 'phcoder' Serbinenko]

Separate command is better as it keeps interface tidy and unpoluted, decreasing maintenance cost. Correct me if I'm wrong but it should be clear from context of file is Android image or usual linux one?

Le ven. 12 févr. 2016 20:19, Shea Levy <address@hidden> a écrit :

On 2016-02-12 12:50, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 08.02.2016 21:47, Shea Levy wrote:
>> ---
>>  grub-core/loader/i386/linux.c | 27 +++++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/grub-core/loader/i386/linux.c
>> b/grub-core/loader/i386/linux.c
>> index fddcc46..6ab8d3c 100644
>> --- a/grub-core/loader/i386/linux.c
>> +++ b/grub-core/loader/i386/linux.c
>> @@ -35,6 +35,7 @@
>>  #include <grub/i18n.h>
>>  #include <grub/lib/cmdline.h>
>>  #include <grub/linux.h>
>> +#include <grub/android_bootimg.h>
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -695,7 +696,13 @@ grub_cmd_linux (grub_command_t cmd
>> __attribute__ ((unused)),
>>        goto fail;
>>      }
>>
>> -  file = grub_file_open (argv[0]);
>> +  char android_cmdline[BOOT_ARGS_SIZE];
>> +  android_cmdline[0] = '\0';
>> +  if (grub_memcmp (argv[0], "android_bootimg:", sizeof
>> "android_bootimg:" - 1) == 0)
>> +    grub_android_bootimg_load_kernel (argv[0] + sizeof
>> "android_bootimg:" - 1,
>> +                                      &file, android_cmdline);
>> +  else
>> +      file = grub_file_open (argv[0]);
> I hoped more for autodetection. This gets a bit hairy and proper
> separation is better. Sorry for confusion. I think it's simpler with
> commands like
> android_bootimg [--no-cmdline] [--no-initrd] IMAGE [EXTRA_ARGUMENTS]
> by default it will load both IMAGE, with cmdline and initrd. With
> --no-initrd you can use initrd for custom initrd.

Autodetection would be possible actually, I didn't think of that. If
grub_file_open fails, we can try grub_android_bootimg_load_kernel on the
same file. Would that be preferable or do we still want a separate
command?

>
>>    if (! file)
>>      goto fail;
>>
>> @@ -1008,12 +1015,20 @@ grub_cmd_linux (grub_command_t cmd
>> __attribute__ ((unused)),
>>    linux_cmdline = grub_zalloc (maximal_cmdline_size + 1);
>>    if (!linux_cmdline)
>>      goto fail;
>> -  grub_memcpy (linux_cmdline, LINUX_IMAGE, sizeof (LINUX_IMAGE));
>> +  grub_size_t cmdline_offset = 0;
>> +  if (android_cmdline[0])
>> +    {
>> +      cmdline_offset = grub_strlen (android_cmdline) + 1;
>> +      grub_memcpy (linux_cmdline, android_cmdline, cmdline_offset -
>> 1);
>> +      linux_cmdline[cmdline_offset - 1] = ' ';
>> +    }
>> +  grub_memcpy (linux_cmdline + cmdline_offset, LINUX_IMAGE, sizeof
>> (LINUX_IMAGE));
>> +  cmdline_offset += sizeof LINUX_IMAGE - 1;
> LINUX_IMAGE must be at the beginning. don't forget brackets around
> sizeof.
>>    grub_create_loader_cmdline (argc, argv,
>> -                          linux_cmdline
>> -                          + sizeof (LINUX_IMAGE) - 1,
>> -                          maximal_cmdline_size
>> -                          - (sizeof (LINUX_IMAGE) - 1));
>> +                              linux_cmdline
>> +                              + cmdline_offset,
>> +                              maximal_cmdline_size
>> +                              - cmdline_offset);
>>
>>    len = prot_file_size;
>>    if (grub_file_read (file, prot_mode_mem, len) != len &&
>> !grub_errno)
>>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel


_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel