[LinuxBIOS] New LAR access functions
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 = size;
> > > > +}
> > >
> > > Then there's the matter of the pointer arithmetic again. Since ptr
> > > is uchar * this will work, but then writing size to p 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
> 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.. :)
Systems Software Development Engineer
Advanced Micro Devices, Inc.
More information about the coreboot