Re: grub2 efi patches

[Marco Gerards]

Alexandre Boeglin <address@hidden> writes:

> On Sun, 4 Nov 2007 01:21:24 +0100, Alexandre Boeglin <address@hidden>
> wrote:
>> Le Sun, Nov 04, 2007 at 01:04:24AM +0100, Alexandre Boeglin a écrit :
>>> Sorry, there was a mistake in the previous one ...
>> Oops, and line 91 of this one should be
>
> Hi, here is a "more correct" version of this patch: it also frees the
> memory allocated to the options string when appropriate.

I should've started with this patch ;-)

I can better do a full review again for clarity :-)

> Index: include/grub/efi/chainloader.h
> ===================================================================
> RCS file: /sources/grub/grub2/include/grub/efi/chainloader.h,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader.h
> --- include/grub/efi/chainloader.h    21 Jul 2007 23:32:22 -0000      1.2
> +++ include/grub/efi/chainloader.h    5 Nov 2007 22:58:51 -0000
> @@ -19,6 +19,6 @@
>  #ifndef GRUB_EFI_CHAINLOADER_HEADER
>  #define GRUB_EFI_CHAINLOADER_HEADER  1
>  
> -void grub_chainloader_cmd (const char *filename);
> +void grub_chainloader_cmd (int argc, char *argv[]);
>  
>  #endif /* ! GRUB_EFI_CHAINLOADER_HEADER */
> Index: loader/efi/chainloader.c
> ===================================================================
> RCS file: /sources/grub/grub2/loader/efi/chainloader.c,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader.c
> --- loader/efi/chainloader.c  21 Jul 2007 23:32:28 -0000      1.2
> +++ loader/efi/chainloader.c  5 Nov 2007 22:58:58 -0000
> @@ -17,8 +17,6 @@
>   *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -/* TODO: support load options.  */
> -
>  #include <grub/loader.h>
>  #include <grub/file.h>
>  #include <grub/err.h>
> @@ -38,6 +36,8 @@
>  
>  static grub_efi_physical_address_t address;
>  static grub_efi_uintn_t pages;
> +static grub_efi_char16_t *options;
> +static grub_efi_uintn_t options_len;
>  static grub_efi_device_path_t *file_path;
>  static grub_efi_handle_t image_handle;
>  
> @@ -49,6 +49,8 @@
>    b = grub_efi_system_table->boot_services;
>    b->unload_image (image_handle);
>    b->free_pages (address, pages);
> +  if (options)
> +    b->free_pool (options);
>    grub_free (file_path);
>    
>    grub_dl_unref (my_mod);
> @@ -175,7 +177,7 @@
>  }
>  
>  void
> -grub_chainloader_cmd (const char *filename)
> +grub_chainloader_cmd (int argc, char *argv[])
>  {
>    grub_file_t file = 0;
>    grub_ssize_t size;
> @@ -185,16 +187,47 @@
>    grub_device_t dev = 0;
>    grub_efi_device_path_t *dp = 0;
>    grub_efi_loaded_image_t *loaded_image;
> +  char *filename = argv[0];
>    
>    grub_dl_ref (my_mod);
>  
>    /* Initialize some global variables.  */
>    address = 0;
> +  options = 0;
> +  options_len = 0;
>    image_handle = 0;
>    file_path = 0;
>    
>    b = grub_efi_system_table->boot_services;
>  
> +  if (argc == 0)
> +  {
> +    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> +    goto fail;
> +  }

The indentation is not right.

> +  /* copy options */

/* Copy options.  */

> +  if (argc > 1)
> +  {

The indentation is not right.

> +    grub_efi_char16_t *p;
> +    grub_efi_int16_t i;
> +    grub_efi_uint16_t j;
> +    for (i = 1; i < argc; i++)
> +      options_len += (grub_strlen (argv[i]) + 1) * sizeof (*options);
> +    options_len += sizeof (*options); /* \0 */
> +
> +    status = b->allocate_pool (GRUB_EFI_LOADER_DATA, options_len, (void *) 
> &options);
> +    if (status != GRUB_EFI_SUCCESS)
> +      goto fail;
> +    p = options;

Please use grub_malloc here, I think it should work for what you are
doing, right?

> +    for (i = 1; i < argc; i++){
> +      *p++ = ' ';
> +      for (j = 0; j < grub_strlen (argv[i]) + 1; j++)
> +        p[j] = (grub_efi_char16_t) argv[i][j];
> +    }
> +  }
> +
>    file = grub_file_open (filename);
>    if (! file)
>      goto fail;
> @@ -268,6 +301,10 @@
>      }
>    loaded_image->device_handle = dev_handle;
>    
> +  if (options_len)
> +    loaded_image->load_options = options;
> +    loaded_image->load_options_size = options_len;

The second loaded_image... line seems to be incorrectly indented.

>    grub_file_close (file);
>    grub_loader_set (grub_chainloader_boot, grub_chainloader_unload, 0);
>    return;
> @@ -286,16 +323,16 @@
>    if (address)
>      b->free_pages (address, pages);
>    
> +  if (options)
> +    b->free_pool (options);
> +
>    grub_dl_unref (my_mod);
>  }
>  
>  static void
>  grub_rescue_cmd_chainloader (int argc, char *argv[])
>  {
> -  if (argc == 0)
> -    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> -  else
> -    grub_chainloader_cmd (argv[0]);
> +  grub_chainloader_cmd (argc, argv);
>  }
>  
>  static const char loader_name[] = "chainloader";
> Index: loader/efi/chainloader_normal.c
> ===================================================================
> RCS file: /sources/grub/grub2/loader/efi/chainloader_normal.c,v
> retrieving revision 1.2
> diff -u -r1.2 chainloader_normal.c
> --- loader/efi/chainloader_normal.c   21 Jul 2007 23:32:28 -0000      1.2
> +++ loader/efi/chainloader_normal.c   5 Nov 2007 22:58:59 -0000
> @@ -26,10 +26,7 @@
>  chainloader_command (struct grub_arg_list *state __attribute__ ((unused)),
>                    int argc, char **args)
>  {
> -  if (argc == 0)
> -    grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> -  else
> -    grub_chainloader_cmd (args[0]);
> +  grub_chainloader_cmd (argc, args);
>    return grub_errno;
>  }