Re: [PATCH 2/4] Add grub_efi_get_variable

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 08.02.2012 17:51, Matthew Garrett wrote:
> Code may wish to obtain system information from EFI variables. Add support
> for making the platform call.
> ---
>   ChangeLog                |    6 ++++++
>   grub-core/kern/efi/efi.c |   32 ++++++++++++++++++++++++++++++++
>   include/grub/efi/api.h   |    4 ++++
>   include/grub/efi/efi.h   |    3 ++-
>   4 files changed, 44 insertions(+), 1 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index ca07786..e662f3d 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,11 @@
>   2012-02-08  Matthew Garrett<address@hidden>
>
> +       * grub-core/kern/efi/efi.c (grub_efi_get_variable): Add new function.
> +       * include/grub/efi/efi.h: Likewise.
> +       * include/grub/efi/api.h: Add guid for EFI-specified variables.
> +
> +2012-02-08  Matthew Garrett<address@hidden>
> +
>         * include/grub/efi/pci.h: New file to define EFI PCI protocols.
>
>   2012-02-07  Vladimir Serbinenko<address@hidden>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 9a2c5e6..d366f8a 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -182,6 +182,38 @@ grub_efi_set_virtual_address_map (grub_efi_uintn_t 
> memory_map_size,
>     return grub_error (GRUB_ERR_IO, "set_virtual_address_map failed");
>   }
>
> +void *
> +grub_efi_get_variable(grub_uint8_t *var, grub_efi_guid_t *guid)
> +{
> +  grub_efi_status_t status;
> +  grub_efi_uintn_t datasize = 0;
> +  grub_efi_runtime_services_t *r;
> +  grub_efi_char16_t *var16;
> +  int i;
> +  void *data;
> +
> +  var16 = grub_malloc((grub_strlen((char *)var) +1) * 2);
> +
Indentation.
You also don't check that malloc succeeded. Also I recommend using 
intermediary variable for length
> +  for (i=0; i<(int)grub_strlen((char *)var); i++)
> +    var16[i] = var[i];
> +  var16[i] = '\0';
> +
We use grub_utf8_to_utf16. Also don't forget to multiply the malloc 
length by GRUB_MAX_UTF16_PER_UTF8 ((GRUB_MAX_UTF16_PER_UTF8 * length + 
1) * sizeof (var16[0]))
> +  r = grub_efi_system_table->runtime_services;
> +
> +  status = efi_call_5 (r->get_variable, var16, guid, NULL,&datasize, NULL);
> +
> +  data = grub_malloc(datasize);
> +
Again: no check for success.
> +  status = efi_call_5 (r->get_variable, var16, guid, NULL,&datasize, data);
> +
> +  grub_free(var16);
> +
> +  if (status == GRUB_EFI_SUCCESS)
> +      return data;


--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko