From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40402) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2L43-0001lP-Rx for qemu-devel@nongnu.org; Thu, 25 Jul 2013 08:54:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2L41-00011H-5U for qemu-devel@nongnu.org; Thu, 25 Jul 2013 08:54:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42576) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2L40-00011C-Su for qemu-devel@nongnu.org; Thu, 25 Jul 2013 08:54:37 -0400 Date: Thu, 25 Jul 2013 15:55:56 +0300 From: "Michael S. Tsirkin" Message-ID: <20130725125555.GA761@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> <20130715080102.GA31638@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130715080102.GA31638@redhat.com> Subject: Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-memory ROM files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: seabios@seabios.org, qemu-devel@nongnu.org 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