Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-memory ROM files

[Kevin O'Connor <kevin@koconnor.net>]

On Sun, Jul 07, 2013 at 06:42:35PM +0300, Michael S. Tsirkin wrote:
> Add ability for a ROM file to point to
> it's image in memory. When file is in memory,
> add utility that can patch it, storing
> pointers to one file within another file.

Thanks.  See my comments below.

[...]
> --- /dev/null
> +++ b/src/linker.c
[...]
> +void linker_loader_execute(const char *name)
> +{
> +    struct linker_loader_entry_s *entry;
> +    int size, offset = 0;
> +    void *data = romfile_loadfile(name, &size);
> +    if (!data)
> +        return;
> +
> +    for (offset = 0; offset < size; offset += sizeof *entry) {

For consistent style, please treat sizeof like a function (ie,
sizeof(*entry) ).

> +        entry = data + offset;
> +        /* Check that entry fits in buffer. */
> +        if (offset + sizeof *entry > size) {
> +            warn_internalerror();
> +            break;
> +        }
> +	switch (le32_to_cpu(entry->command)) {
> +		case LINKER_LOADER_COMMAND_ALLOCATE:
> +			linker_loader_allocate(entry);

SeaBIOS uses 4 spaces for indentation, and no tabs.

[...]
> --- a/src/util.h
> +++ b/src/util.h
> @@ -436,6 +436,7 @@ struct romfile_s {
>      char name[128];
>      u32 size;
>      int (*copy)(struct romfile_s *file, void *dest, u32 maxlen);
> +    void *data;
>  };

I'd prefer to see this tracked within the "linker" code and not in the
generic romfile struct.

Also, is there another name besides "linker" that could be used?
SeaBIOS has code to self-relocate and fixup code relocations.  I think
having code in the repo called "linker" could cause confusion.

-Kevin