> -----Original Message-----
> From: Grub-devel [mailto:grub-devel-bounces+elliott=hpe....@gnu.org]
> On Behalf Of Alexander Graf
> Sent: Thursday, May 19, 2016 8:38 AM
> Subject: [PATCH] efi: Free malloc regions on exit
...
> +struct efi_allocation {
If no other file is using this, mark it as static.
> + grub_uint64_t start_addr;
That should be grub_efi_physical_address_t.
> + grub_uint64_t pages;
That should be grub_efi_uint64_t (the parameter type
for add_memory_regions) or grub_efi_uintn_t (the parameter
type for grub_efi_allocate_pages and grub_efi_free_pages).
> +} efi_allocated_memory[16];
Why 16 and not some other number? Consider a #define and making
the comment about 16 in add_memory_regions more generic.
> +unsigned int efi_allocated_memory_idx = 0;
The C language definition guarantees that global variables
are initialized to 0.
> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
> (void *) ((grub_addr_t) start),
> (unsigned) pages);
>
> + /* Track up to 16 regions that we allocate from */
> + if (efi_allocated_memory_idx <
> ARRAY_SIZE(efi_allocated_memory)) {
> + efi_allocated_memory[efi_allocated_memory_idx].start_addr =
> start;
> + efi_allocated_memory[efi_allocated_memory_idx].pages =
> pages;
> + efi_allocated_memory_idx++;
> + }
> +
Consider printing if the magic number is exceeded, rather than
silently changing the behavior.
> grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
>
> required_pages -= pages;
> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
> grub_fatal ("too little memory");
> }
>
> +void
> +grub_efi_memory_fini (void)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < efi_allocated_memory_idx; i++) {
> + grub_efi_free_pages (efi_allocated_memory[i].start_addr,
> + efi_allocated_memory[i].pages);
> + }
> +}
Consider clearing efi_allocated_memory_idx after that, since
this function cannot be sure it's the last one to use a
global variable.
Clearing start_addr might help prevent stale pointer
reference bugs. Pointers that are no longer valid are
dangerous to leave around.
---
Robert Elliott, HPE Persistent Memory
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
