[LinuxBIOS] New LAR access functions

Jordan Crouse jordan.crouse at amd.com
Thu Jul 12 17:34:58 CEST 2007


On 12/07/07 02:46 +0200, Peter Stuge wrote:
> On Wed, Jul 11, 2007 at 05:38:41PM -0600, Jordan Crouse wrote:
> > > > +	int size;
> > > 
> > > u32 size maybe?
> > 
> > I guess, though I'll bet this code doesn't survive long enough to
> > see those 2G ROM chips.. :)
> 
> I won't take that bet.
> 
> But it is nice to show that we have thought about it by being
> explicit. Eliminating potential portability problems is another plus.

Agreed - I'll make the change everywhere.

> > > > +	p = (unsigned int *) (ptr + BOOTBLOCK_SIZE - 12);
> > > > +	p[0] = size;
> > > > +}
> > > 
> > > Then there's the matter of the pointer arithmetic again. Since ptr
> > > is uchar * this will work, but then writing size to p[0] will write
> > > a different number of bytes on different archs, right?
> > 
> > sizeof(unsigned int) is 4 on all the platforms we care about.
> 
> 640k will be enough. ;)

Heh.  Fair enough - depending on compilers to obey convention is probably
not a smart idea.    I'll switch over to u8 and u32 universally - then
we'll be absolutely positive that we're !x86 friendly.

> 
> > > > + err:
> > > > +	if (lar->fd >= 0)
> > > > +		close(lar->fd);
> > > > +
> > > > +	unlink(archive);
> > > > +
> > > > +	if (lar)
> > > > +		free(lar);
> > > 
> > > If lar can be 0 at err: then the fd deref is a segfault waiting
> > > to happen.
> > 
> > Lar can't be 0 at err
> 
> Then the if (lar) free(lar); should change IMHO. It's confusing.

Oh wow - I didn't even notice that.  Yeah, it is confusing.  Sorry
about that.

> 
> Is lar->fd initialized to -1 btw?

lar->fd is the result of the open() - which will either be a -1 for
errors or >= for a legit file descriptor.

> The file can be corrupt, I just want to avoid making it 4G-256k.
> 
> I would be happy if lar exits with an error if the file size has
> changed between _open_lar() and _close_lar().

I'll do that - thats an easy check and it will probably save our bacon.
Then you can tell me "I told you so!".

> I'd prefer having that negation in the caller. Or at the very least
> doxygen comment it.

Yes.  Much doxygen is in my future.

Thanks.  I'm updating the code in real time, but suspect that we still
have a bit more discussion before the next revision goes out.. :)

Jordan

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.






More information about the coreboot mailing list