[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-me
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-memory ROM files |
Date: |
Mon, 15 Jul 2013 11:01:02 +0300 |
On Sun, Jul 14, 2013 at 02:24:52PM -0400, Kevin O'Connor wrote:
> 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.
A way to associate a romfile instance with a value seems generally
useful, no? Still, that's not too hard - it would only mean an extra
linked list of
struct linker {
char name[56]
void *data;
struct hlist_node node;
}
is this preferable?
> 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
romfile_loader?
--
MST
[Qemu-devel] [PATCH v2 2/5] pmm: add a way to test whether memory is in FSEG, Michael S. Tsirkin, 2013/07/07
[Qemu-devel] [PATCH v2 3/5] acpi: pack rsdp, Michael S. Tsirkin, 2013/07/07
[Qemu-devel] [PATCH v2 4/5] acpi: load and link tables from /etc/acpi/, Michael S. Tsirkin, 2013/07/07
[Qemu-devel] [PATCH v2 5/5] acpi: add an option to disable builtin tables, Michael S. Tsirkin, 2013/07/07