Re: [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