From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43951)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1UydhE-0007Ig-Q9
	for qemu-devel@nongnu.org; Mon, 15 Jul 2013 03:59:51 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1UydhC-0007r1-G1
	for qemu-devel@nongnu.org; Mon, 15 Jul 2013 03:59:48 -0400
Received: from mx1.redhat.com ([209.132.183.28]:25588)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1UydhC-0007qq-7v
	for qemu-devel@nongnu.org; Mon, 15 Jul 2013 03:59:46 -0400
Date: Mon, 15 Jul 2013 11:01:02 +0300
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20130715080102.GA31638@redhat.com>
References: <1373211589-8097-1-git-send-email-mst@redhat.com>
	<1373211589-8097-2-git-send-email-mst@redhat.com>
	<20130714182452.GB7607@morn.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20130714182452.GB7607@morn.localdomain>
Subject: Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch
 in-memory ROM files
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Kevin O'Connor <kevin@koconnor.net>
Cc: seabios@seabios.org, qemu-devel@nongnu.org

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