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. -- linuxbios mailing list [email protected] http://www.linuxbios.org/mailman/listinfo/linuxbios
