[LinuxBIOS] [PATCH][LAR] New LAR access functions

[Peter Stuge]

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