Re: [RFC] post-processing tool

[Marco Gerards <metgerards@student.han.nl>]

Hollis Blanchard <hollis@penguinppc.org> writes:

> Linux uses a post-processing tool called addnote to add the NOTE segment 
> to a normal ELF file. I have written my own version of that, which I'm 
> calling "appendnote" to avoid confusion. If it goes into GRUB, it probably 
> belongs in a util/powerpc/ieee1275 directory.

This sounds sane to me.

> I don't believe there exists a grub-setup for PPC, but this could warrant 
> it. One could run grub-setup on the target machine, which could be a shell 
> script that does the following:
>
> - if the system is Old World Mac, install stage1 with a block list for 
>   grubof
> - if CHRP, run appendnote on grubof
> - if CHRP or New World:
>   - find the OF device path to grubof (like ybin or SUSE's "lilo" script)
>   - use nvsetenv to point firmware to grubof

Doesn't ybin mount the boot partition and copy yaboot there?  I think
we have to do the same.

> In that model, appendnote would be a tool compiled and installed on the 
> target. The alternative is to create grubof and grubof.chrp at compile 
> time, install both on the target, and let grub-setup point OF at one or 
> the other.

Both are fine for me.  But the advantage of having grubof and
grubof.chrp is that distributions just need one package for multiple
platforms.

> Any comments? How do I add this tool to the Makefiles?

Add it to sbin_UTILITIES.  BTW, I think grubof does not belong there,
but there is not a better place yet.  When module loading is
implemented on the PPC, I will work on powerpc-ieee1275.rmk to make it
a bit more like the PC version.

Perhaps it would be nice if this utility checks if the ELF is a
powerpc ELF.

> struct chrp_note {
> 	Elf32_Nhdr header;
> 	unsigned char name[8];
> 	uint32_t real_mode;
> 	uint32_t real_base;
> 	uint32_t real_size;
> 	uint32_t virt_base;
> 	uint32_t virt_size;
> 	uint32_t load_base;
> };

Can you please revise this so the GCS is followed.  In the GCS tabs
are not used, but instead of that two spaces.  Also braces are on
their own lines.  So this code will look like:

struct chrp_note
{
  Elf32_Nhdr header;
  unsigned char name[8];
  uint32_t real_mode;
  uint32_t real_base;
  uint32_t real_size;
  uint32_t virt_base;
  uint32_t virt_size;
  uint32_t load_base;
};


> off_t segtable_size(void *file)

Please put the return type on a separate line.  Add a space after the
function name.

> 	return elf_header->e_phentsize * elf_header->e_phnum;

In case of a pointer, don't add a space after the star.

> 	if (argc < 3) {
> 		fprintf (stderr, "Usage: %s <ELF file> <new ELF file>\n", argv[0]);
> 		return 1;
> 	}

Is it possible to use a parser?  I prefer argp, but we need to agree
what to use for utilities?  This is also in the GCS to keep the GNU
utilities consistent.  For example the help output should be
consistent with other utilities and --help should be accepted.  When
using such parser we get all that for free. :)

I could do that if you want, I am used to argp and can implement such
thing in a few minutes.

Can you please modify the program so it is GCS compliant?  For me it
is important to have a consistent coding style in GRUB.  It is not a
problem for me to change all this, but it is a lot of work for me so I
prefer not to do this for every patch. :)

Thanks,
Marco