Re: grub-mkimage and module loading

[Marco Gerards]

Hollis Blanchard <address@hidden> writes:

> This is a more complete patch for PPC grub-mkimage that I posted a while
> back. Important points:
> - move grub_load_modules() into i386/pc as grub_arch_load_modules()
> - implement PPC's grub_arch_load_modules() using an in-memory structure
>   telling us how many modules to expect

I would prefer a grub_load_modules and a function to return the
address and size of the modules in memory.  I explained this in my
other email.

Anyway, I'd prefer this to work the same on every machine and this
code to be shared.

> - the structure contains a version field to avoid grub-mkimage/grub
>   binary layout mismatches

I don't think versioning is required.

> I think there is still some PC cleanup needed, e.g. grub_end_addr and
> grub_add_unused_region() should also be moved into i386 code, but my
> patch is already fairly large, and I'd prefer a PC person were involved
> in this additional cleanup anyways.

Ok.  Can you at least make sure it compiles on the PC and try not to
break it?

> This patch has been tested and is needed to load modules on PPC. Please
> comment soon so we can get PPC modules working in the main tree...

I assume you will make a changelog entry when the patch is more or
less ok?  It's easier to review a patch using a changelog entry.

> Index: include/grub/kernel.h
> ===================================================================
> RCS file: /cvsroot/grub/grub2/include/grub/kernel.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 kernel.h
> --- include/grub/kernel.h     4 Apr 2004 13:46:00 -0000       1.4
> +++ include/grub/kernel.h     2 Jan 2005 19:01:53 -0000
> @@ -31,8 +31,14 @@ struct grub_module_header
>    grub_size_t size;
>  };
>  
> -/* The start address of the kernel.  */
> -extern grub_addr_t grub_start_addr;

...

> -/* Return the end address of the core image.  */
> -grub_addr_t grub_get_end_addr (void);

Doesn't this break the PC port?

> +/* TODO: endianness */

Better move this to the TODO list.

> +void load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)

GCS nitpick: put the return type on a separate line:

void
load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)

Same for all other functions, btw.

> +void load_modules (Elf32_Phdr *phdr, const char *dir, char *mods[], FILE 
> *out)
> +{
> +  char *module_img;
> +  struct grub_util_path_list *path_list, *p;

Please use separate lines for every declaration:

struct grub_util_path_list *path_list;
struct grub_util_path_list *p;

Thanks,
Marco