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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 15, 2013 at 11:01:02AM +0300, Michael S. Tsirkin wrote:
> 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?


Could you respond on above please?

> -- 
> MST