On Mon, Apr 01, 2013 at 06:18:17PM +0200, Francesco Lavra wrote: > On 03/24/2013 06:01 PM, Leif Lindholm wrote: > > === added directory 'grub-core/loader/arm' > > === added file 'grub-core/loader/arm/linux.c' > > --- grub-core/loader/arm/linux.c 1970-01-01 00:00:00 +0000 > > +++ grub-core/loader/arm/linux.c 2013-03-24 13:49:04 +0000 [...] > > +static grub_addr_t initrd_start; > > +static grub_size_t initrd_end; > > initrd_end should be the same type as initrd_start. Clearly.
[...] > > +static grub_err_t > > +linux_prepare_fdt (void) > > +{ > > + int node; > > + int retval; > > + int tmp_size; > > + void *tmp_fdt; > > + > > + tmp_size = fdt_totalsize ((void *) boot_data) + > > FDT_ADDITIONAL_ENTRIES_SIZE; > > Having a fixed size limit (FDT_ADDITIONAL_ENTRIES_SIZE) to insert > variable-sized data (such as the linux command line) seems suboptimal, > as this may fail when a very long command line is passed. How about > removing the FDT_ADDITIONAL_ENTRIES_SIZE define from the header file > include/grub/arm/linux.h (after all, this is an implementation detail > and IMHO shouldn't go in a public header file) and defining it in this > function, say: > > #define FDT_ADDITIONAL_ENTRIES_SIZE (0x100 + \ > grub_strlen (linux_args)) Mmm. It was only used in one location though, so I'll use your suggestion, but just inline it. > [...] > > +#ifdef GRUB_MACHINE_EFI > > + linux_addr = (grub_addr_t) grub_efi_allocate_loader_memory > > (LINUX_PHYS_OFFSET, size); > > There should be a check against allocation failure. Yes, this is a global issue with this loader under EFI - dealing with memory allocation/deallocation. Will be resolved in next version. > > +static grub_err_t > > +linux_unload (void) > > +{ > > + grub_dl_unref (my_mod); > > + > > + grub_free (linux_args); > > + linux_args = NULL; > > linux_args might already be NULL, if memory allocation for it failed in > grub_cmd_linux(). Yes, but on the other hand grub_free (like libc free) checks for NULL. > In the EFI case, the memory allocated for the kernel image should be > freed as well. Yes. > > + > > + initrd_start = initrd_end = 0; > > Why should the initrd data be discarded here? It probably shouldn't. > > + > > + return GRUB_ERR_NONE; > > +} > > +static grub_err_t > > +grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), > > + int argc, char *argv[]) > > +{ > > + int size, retval; > > The return type of linux_load() is grub_err_t, so retval should be the > same type. > > > + grub_file_t file; > > + grub_dl_ref (my_mod); > > + > > + if (argc == 0) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); > > + > > + file = grub_file_open (argv[0]); > > + if (!file) > > + goto fail; > > + > > + retval = linux_load (argv[0]); > > Here the file is already open: why not just pass the file handle to > linux_load()? Unfortunate leftover from earlier version, thanks. > > + grub_file_close (file); > > + if (retval != GRUB_ERR_NONE) > > + goto fail; > > In order to correctly report all error types, grub_errno should be set > to retval before entering the error path. OK, adding grub_error() to all failure paths in linux_load. > > + > > + grub_loader_set (linux_boot, linux_unload, 1); > > Since linux_boot() may return control to its caller, the third argument > of grub_loader_set() should be 0, otherwise linux_boot() returning > control to its caller results in a dysfunctional state. OK. > > + > > + size = grub_loader_cmdline_size (argc, argv); > > + linux_args = grub_malloc (size + sizeof (LINUX_IMAGE)); > > + if (!linux_args) > > + goto fail; > > grub_loader_unset() should be called in this error path, otherwise if > one tries to boot in this state, linux_prepare_fdt() will access a NULL > pointer. Yes. > > + if (grub_file_read (file, (void *) initrd_start, size) != size) > > + goto fail; > > In the EFI case, the memory allocated for the initrd should be freed > here. And in both EFI and U-Boot cases, initrd_start should be zeroed. Yes. > > +#else > > + boot_data = LINUX_FDT_ADDRESS; > > +#endif > > + grub_dprintf ("loader", "Loading device tree to 0x%08x\n", > > + (grub_addr_t) boot_data); > > + fdt_move (blob, (void *) boot_data, fdt_totalsize (blob)); > > + grub_free (blob); > > I don't get the point of allocating memory for the FDT in load_dtb() > just to move the FDT data to boot_data and then freeing the temporary > memory. Isn't it easier if load_dtb() operates directly on boot_data? Maybe not the sanest of reasons, but the U-Boot port permits passing through ATAGs if no FDT is available, so I prefer not corrupting boot_data until a successful load has occurred. > > + > > + /* > > + * We've successfully loaded an FDT, so any machine type passed > > + * from firmware is now obsolete. > > + */ > > + machine_type = ARM_FDT_MACHINE_TYPE; > > + > > + return GRUB_ERR_NONE; > > The file should be closed before returning. Yes. / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel