[LinuxBIOS] New LAR access functions
Peter Stuge
peter at stuge.se
Thu Jul 12 02:46:06 CEST 2007
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.
> > > + unsigned int *ptr = (unsigned int *) (lar->map + (filelen - 12));
> >
> > What about all this pointer arithmetic? Is it really correct?
>
> lar->map is a uchar, which is a size of 1, so the math works.
Yes, that's right. Sorry about the noise.
> I think by convention, sizeof(uchar) is 1 everywhere, so there
> shouldn't be a portability issue here. If there is, then we'll
> have to do some very ugly casting in about 30 places in the code,
> and I'm hoping it doesn't come to that.
The alternative is u8 *map. Copy u32 size reasoning but much less
important. No. Never mind.
> > > + 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. ;)
> I guess I could use u32 instead -
Please do. Or do I need to be hit over the head hard with a C
cluestick?
> but thats just going to end up decoding back down to unsigned int.
Fine.
> If we used an unsigned long, then we would be in trouble, but I've
> made very sure we didn't do something like that.
Yeah, that's good. But is it good enough?
> > > + 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.
I'd like this though:
if (lar) {
if (lar->fd >= 0)
close(lar->fd);
free(lar);
}
unlink(archive);
Is lar->fd initialized to -1 btw?
> > Race conditions. First open() then fstat() to get the size.
>
> Okay, I agree. It will be a little bit more complex, but its
> probably better.
Make the size be a & argument to _open_lar() instead and hide the
fstat() in there.
> > Also, the file size can change even though we've opened the file.
> >
> > Since the size is a rather important variable throughout lar we
> > want to be able to handle it changing under our feet, but I don't
> > know what the best way would be?
>
> Do we care? We're not designing LAR to be able to handle
> concurent processes changing the file at the same time - if it
> happens, it will be just pure, unmigitaged bad luck - same as if
> you happen to change a file while you 'cat' or 'dd' it. Is this
> something thats realistic enough to worry about?
It depends on the rest of the code. As long as lar does not go crazy
if a larball changes in size while running we don't care, but some
calculations do use that total size..
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().
> > > +static int file_in_list(struct file *files, char *filename)
> > > +{
> > > + struct file *p;
> > > +
> > > + if (files == NULL)
> > > + return 1;
> >
> > Shouldn't this if just be removed so that a NULL list falls
> > through the for and returns 0?
>
> No - it should return a 1, because by design, files==NULL means
> "show all files".
That seems sort of unintuitive for a function named file_in_list?
I'd prefer having that negation in the caller. Or at the very least
doxygen comment it.
> > > + ptr += (ntohl(header->len) + ntohl(header->offset) - 1)
> > > + & 0xfffffff0;
> >
> > We want this piece of code in a single place before it's in 1000
> > places. Could you make a function of it?
>
> Sure.
Also note that ntohl() already uses uint32_t - another reason for us
to use all u32.
> > Again perror() or strerror(errno) would be nice. Goes for the
> > entire patch of course. :)
>
> Most of these were pretty quick and dirty. I was thinking while I
> was writing this that we should go through and standardize all the
> error and warning messages. This would be one of those things to
> do.
Sure, separate patch for this some other time would be great. I
figured I'd mention it since this was all new code though.
> > > + if (ntohl(header->compression) == none) {
> >
> > This ntohl() everywhere is tedious and a bit error prone. Perhaps
> > it would be better to convert all data to machine byte order only
> > when it's read rather than every time it's used?
>
> The file is mmaped, so thats really impossible. It is tedious, but
> its the only way to ensure compatability with big endian machines.
We can abstract the header. I am generally strongly against
unneccessary abstraction but in this case I think it's warranted. The
code will be MUCH more readable and writable without all the ntohl().
Either have get functions for the header fields or make a native copy
of the header in the new function that finds the next valid file
header. I like the latter.
> > > + if (filename[0] == '.' && filename[1] == '/')
> > > + filename += 2;
> >
> > realpath() ?
>
> I don't know if realpath is the right one here - because it returns
> the absolute path, which I don't think is the goal here.
No, but cwd would be stripped away to get a canonicalized relative
path. I can break the above code just by doing ././filename while a
construct based on realpath() would clean anything up.
There's no reason for a person to write ././foo of course, but it
could easily happen with a layer or two of scripting.
> I tried to squeeze the filename : pathname stuff in here without
> making too many changes - perhaps we should revisit that.
I think so.
> > ?! Is the compressor function required even with algo none?
>
> I think it does it to avoid complex logic - but we can look at this
> again - there's probably cleanup that can happen here.
Not that complex.. if (algo != none) { do_compression_stuff; } :)
> Thanks for your comments.
Welcome. Thanks for the patch! =)
//Peter
More information about the coreboot
mailing list