[LinuxBIOS] [PATCH][LAR] New LAR access functions
Peter Stuge
peter at stuge.se
Thu Jul 12 00:49:20 CEST 2007
Really good ideas! But some comments on implementation..
On Wed, Jul 11, 2007 at 12:15:17PM -0600, Jordan Crouse wrote:
> +struct lar {
> + int fd;
> + unsigned char *map;
> + int size;
> +};
u32 size maybe?
> +static inline int get_bootblock_offset(int size)
> +{
> + return size - (BOOTBLOCK_SIZE + sizeof(struct lar_header) + BOOTBLOCK_NAME_LEN);
> +}
Same here, u32 for sizes.
> +static int lar_read_size(struct lar *lar, int filelen)
> +{
> + unsigned int *ptr = (unsigned int *) (lar->map + (filelen - 12));
> + return ptr[0];
> +}
What about all this pointer arithmetic? Is it really correct?
Doesn't adding 1 to a pointer actually add sizeof pointed-to-type to
the address? Ie. this code isn't portable?
> +static void annotate_bootblock(unsigned char *ptr, unsigned int size)
> +{
> + int i;
> + unsigned int *p;
> +
> + for(i = 13; i > 0; i--)
> + ptr[BOOTBLOCK_SIZE - i] = '\0';
I think a memset() call here would be nicer..
> + 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?
> + 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.
> +static void _close_lar(struct lar *lar)
> +{
> + munmap(lar->map, lar->size);
> + close(lar->fd);
> +
> + free(lar);
> +}
Maybe add if(!lar) return; ? Maybe even complain with a warning.
> + /* Make a dummy bootblock */
> +
> + if (lar_add_bootblock(lar, NULL)) {
> + _close_lar(lar);
> + return NULL;
> + }
This will leave a half-baked lar file hanging if adding the bootblock
fails. Perhaps unlink() too in case of error?
> +struct lar * lar_open_archive(const char *archive)
> +{
> + struct lar *lar;
> + int ret, romlen;
> + struct stat s;
> +
> + ret = stat(archive, &s);
> +
> + if (ret == -1) {
> + err("Unable to stat %s\n", archive);
> + return NULL;
> + }
> +
> + lar = _open_lar(archive, s.st_size, O_RDWR);
Race conditions. First open() then fstat() to get the size.
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?
> +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?
> + for(p = files ; p; p = p->next) {
> + if (!strcmp(p->name, filename))
> + return 1;
> + }
> +
> + return 0;
..since return 1 is used to indicate the file is actually in the
list?
> + 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?
> +static int _write_file(char *filename, unsigned char *buffer, int len)
> +{
> + char *path = strdup(filename);
> + int fd, ret;
> +
> + if (path == NULL) {
> + err("Out of memory.\n");
> + return -1;
> + }
> +
> + mkdirp((const char *) dirname(path), 0755);
> + free(path);
> +
> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +
> + if (fd == -1) {
> + err("Error creating file %s\n", filename);
> + return -1;
> + }
I don't think lar should mkdir -p implicitly. Just open() and
perror() or print strerror(errno) in case of failure.
> + ret = write(fd, buffer, len);
> +
> + if (ret != len)
> + err("Error writingthe file %s\n", filename);
Again perror() or strerror(errno) would be nice. Goes for the entire
patch of course. :)
> + if (strncmp(header->magic, MAGIC, 8))
> + break;
Is strncmp() good for MAGIC ? Maybe memcmp() ?
> + 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?
> + if (strstr(name, "nocompress:") == name) {
> + filename += 11;
> + thisalgo = none;
> + }
strncmp() ?
> + if (filename[0] == '.' && filename[1] == '/')
> + filename += 2;
realpath() ?
> + ret = stat (filename, &s);
> +
> + if (ret) {
> + err("Unable to stat %s\n", filename);
> + return -1;
> + }
> +
> + /* Allocate a temporary buffer to compress into - this is unavoidable,
> + because we need to make sure that the compressed data will fit in
> + the LAR, and we won't know the size of the compressed data until
> + we actually compress it */
> +
> + temp = calloc(s.st_size, 1);
> +
> + if (temp == NULL) {
> + err("Out of memory.\n");
> + return -1;
> + }
> +
> + /* Open the file */
> + fd = open(filename, O_RDONLY);
Race again.
> + /* Do the compression step */
> + compress_functions[thisalgo](ptr, s.st_size, temp, &complen);
> +
> + if (complen >= s.st_size && (thisalgo != none)) {
> + thisalgo = none;
> + compress_functions[thisalgo](ptr, s.st_size, temp, &complen);
> + }
?! Is the compressor function required even with algo none?
//Peter
More information about the coreboot
mailing list