Re: [PATCH V3 2/3] Add support for avoiding firmware in relocations

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 08.02.2012 17:55, Matthew Garrett wrote:
> EFI and OF both support firmware regions which may be in use during loading.
>
> +unsigned
> +grub_relocator_firmware_overlaps (grub_phys_addr_t target, grub_size_t size)
> +{
> +  grub_efi_uintn_t mmapsize = 0, desc_size = 0;
> +  grub_efi_uint32_t descriptor_version = 0;
> +  grub_efi_memory_descriptor_t *descs = NULL;
> +  grub_efi_uintn_t key;
> +  int overlap = 0;
> +  grub_efi_memory_descriptor_t *desc;
> +
> +  grub_efi_get_memory_map (&mmapsize, NULL,&key,&desc_size,
> +                       &descriptor_version);
You need to check return value.
You don't use key. You can just make it NULL (grub_efi_get_memory_map 
handles it).
> +  descs = grub_malloc (mmapsize);
> +  if (!descs)
> +    return 0;
> +
You seem to lack error handling as whole. I'd suggest make this function 
return grub_err_t and generate a grub_error itself if overlap is detected.
You current code would happily allocate anywhere if grub_malloc fails.
> +  grub_efi_get_memory_map (&mmapsize, descs,&key,&desc_size,
> +                       &descriptor_version);
> +
> +
>     adjust_limits (rel,&min_addr,&max_addr, target, target);
>
> +  if (avoid_firmware)
> +    {
> +#if GRUB_RELOCATOR_HAVE_FIRMWARE_REQUESTS
> +      if (grub_relocator_firmware_overlaps(target, size))
> +
> +      return grub_error(GRUB_ERR_BAD_ARGUMENT, "target overlaps with 
> firmware");
> +#endif
> +    }
> +
Ditto. Also here you have indentation problem

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