Re: [PATCH] fix multiboot/aout, cleanup relocators, etc

[Robert Millan]

On Thu, Aug 07, 2008 at 01:27:53AM +0200, Robert Millan wrote:
> 
> Hi,
> 
> It seems when adding the relocators for ELF, I failed to notice both ELF and
> a.out loaders share the same grub_multiboot_real_boot function, and hence my
> code was trashing %eax.
> 
> Since this functionality is useful for the a.out loader anyway, I decided to
> implement it as well.  In the process I did some cleanup, mostly to reduce
> code duplication among both users of the relocators.
> 
> So here's the patch.  Please comment!

I think I should ellaborate a bit on this:

>    if (header->flags & MULTIBOOT_AOUT_KLUDGE)
>      {
> -      int ofs;
> +      int offset = ((char *) header - buffer -
> +                 (header->header_addr - header->load_addr));
> +      int load_size = ((header->load_end_addr == 0) ? file->size - offset :
> +                    header->load_end_addr - header->load_addr);
>  
> -      ofs = (char *) header - buffer -
> -            (header->header_addr - header->load_addr);
> -      if ((grub_aout_load (file, ofs, header->load_addr,
> -                           ((header->load_end_addr == 0) ? 0 :
> -                            header->load_end_addr - header->load_addr),
> -                           header->bss_end_addr))
> -          !=GRUB_ERR_NONE)
> -        goto fail;
> +      if (header->bss_end_addr)
> +     grub_multiboot_payload_size = (header->bss_end_addr - 
> header->load_addr);
> +      else
> +     grub_multiboot_payload_size = load_size;
> +      grub_multiboot_payload_dest = header->load_addr;
>  
> -      entry = header->entry_addr;
> +      playground = grub_malloc (RELOCATOR_SIZEOF(forward) + 
> grub_multiboot_payload_size + RELOCATOR_SIZEOF(backward));
> +      if (! playground)
> +     goto fail;
> +
> +      grub_multiboot_payload_orig = (long) playground + 
> RELOCATOR_SIZEOF(forward);
> +
> +      if ((grub_file_seek (file, offset)) == (grub_off_t) - 1)
> +     goto fail;
> +
> +      grub_file_read (file, grub_multiboot_payload_orig, load_size);
> +      if (grub_errno)
> +     goto fail;
> +      
> +      if (header->bss_end_addr)
> +     grub_memset (grub_multiboot_payload_orig + load_size, 0,
> +                  header->bss_end_addr - header->load_addr - load_size);
> +      
> +      grub_multiboot_payload_entry_offset = header->entry_addr - 
> header->load_addr;
> +
>      }

The problem I had here is that grub_aout_load() was constrained to the api
used by our BSD loaders, and the relocator approach requires to intermangle
things, so I had to move it to multiboot.c and transform it.

Splitting it to a separate function (like we do for multiboot/ELF) wouldn't
be hard if this is seen as a good idea, but making it global wouldn't be so
sound IMHO: it relies on the relocator variables, so one would have to
implement relocators in each a.out loader, which doesn't sound like such a
bad idea, but the relocator code itself is biased towards multiboot (written
to preserve %eax and %ebx), and so it's not as generic as it looks, as other
load protocols might have conflicting constraints.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."