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

[Jordan Crouse]

Okay, here we go.  This patch is the meat and potatoes of the whole
operation.  This takes the code previous in create.c, extract.c, list.c
and bootblock.c, and consolidates them into one file.  We use the concept
of a "stream" of sorts here - we create or open a lar, apply operations to
it, and then close it when we are done.  This allows us to mmap the LAR
for speed and simplicity.  This has the slight side effect of requiring that
the user specify a size for the LAR when they create it, but I don't think
thats really a bad thing.

One thing you might miss is that I've turned the path name of the bootblock
into a constant name 'bootblock'.  I did this for a number of reasons -
mainly because its easier to partition off the "bootblock area", and keep
the other blobs from infringing on it - but also because I can't grok an
good reason to have arbitrary bootblock names.

BTW - There isn't anything in this patch precluding a 'lar_delete_files"
function.  If deleting things from a LAR is interesting, then it should be
pretty easy to get that in here too.

Jordan

[PATCH][LAR] New LAR access functions

In preparation for adding new LAR functionality - this patch consolidates
creating and accessing the LAR into new code utilizing mmap which
facilitates moving about within the archive.

This code also turns the bootblock path name as a constant value.
It also requires that the user specify a size when the LAR is
created.

Signed-off-by: Jordan crouse 
Index: LinuxBIOSv3/util/lar/Makefile
===================================================================
--- LinuxBIOSv3.orig/util/lar/Makefile	2007-07-11 11:40:37.000000000 -0600
+++ LinuxBIOSv3/util/lar/Makefile	2007-07-11 11:41:37.000000000 -0600
@@ -18,7 +18,7 @@
 ## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
 ##

-LAROBJ := lar.o create.o extract.o list.o lib.o bootblock.o
+LAROBJ := lar.o stream.o lib.o

LARDIR := lardir

Index: LinuxBIOSv3/util/lar/lar.c
===================================================================
--- LinuxBIOSv3.orig/util/lar/lar.c	2007-07-11 11:40:29.000000000 -0600
+++ LinuxBIOSv3/util/lar/lar.c	2007-07-11 11:41:37.000000000 -0600
@@ -62,6 +62,64 @@
    return bootblock;
 }

+int create_lar(const char *archivename, struct file *files)
+{
+	struct lar *lar = lar_new_archive(archivename, larsize);
+
+	if (lar == NULL) {
+		fprintf(stderr, "Unable to create %s as a LAR archive.\n",
+			archivename);
+		exit(1);
+	}
+
+	for( ; files; files = files->next) {
+		if (lar_add_file(lar, files->name)) {
+			fprintf(stderr, "Error adding %s to the LAR.\n", files->name);
+			lar_close_archive(lar);
+			exit(1);
+		}
+	}
+
+	if (lar_add_bootblock(lar, bootblock)) {
+		fprintf(stderr, "Error adding the bootblock to the LAR.\n");
+		lar_close_archive(lar);
+		exit(1);
+	}
+
+	lar_close_archive(lar);
+	return 0;
+}
+
+int list_lar(const char *archivename, struct file *files)
+{
+	struct lar *lar = lar_open_archive(archivename);
+
+	if (lar == NULL) {
+		fprintf(stderr, "Unable to open LAR archive %s\n", archivename);
+		exit(1);
+	}
+
+	lar_list_files(lar, files);
+	lar_close_archive(lar);
+	return 0;
+}
+
+int extract_lar(const char *archivename, struct file *files)
+{
+	int ret;
+
+	struct lar *lar = lar_open_archive(archivename);
+
+	if (lar == NULL) {
+		fprintf(stderr, "Unable to open LAR archive %s\n", archivename);
+		exit(1);
+	}
+
+	ret = lar_extract_files(lar, files);
+	lar_close_archive(lar);
+	return ret;
+}
+
 int main(int argc, char *argv[])
 {
    int opt;
@@ -173,16 +231,9 @@

/* adding a bootblock only makes sense when creating a lar */
    	if (!larsize) {
-			printf("Warning: When specifying a bootblock "
-			       "you should also set an archive size.\n");
-		}
-
-		/* load the bootblock */
-		if (larmode == CREATE) {
-			load_bootblock(bootblock);
-			fixup_bootblock();
+			printf("When creating a LAR archive, you must specify an archive size.\n");
+			exit(1);
    	}
-
    }

if (optind < argc) {
Index: LinuxBIOSv3/util/lar/lar.h
===================================================================
--- LinuxBIOSv3.orig/util/lar/lar.h	2007-07-11 11:40:29.000000000 -0600
+++ LinuxBIOSv3/util/lar/lar.h	2007-07-11 11:41:37.000000000 -0600
@@ -55,6 +55,9 @@
 #define MAX_PATHLEN 1024
 #define BOOTBLOCK_SIZE 16384

+#define BOOTBLOCK_NAME "bootblock"
+#define BOOTBLOCK_NAME_LEN 16
+
 typedef uint32_t u32;

struct lar_header {
@@ -72,6 +75,12 @@
    u32 compression;
 };

+struct lar {
+	int fd;
+	unsigned char *map;
+	int size;
+};
+
 enum compalgo { none = 0, lzma = 1, nrv2b = 2 };

typedef void (*compress_func) (char *, u32, char *, u32 *);
Index: LinuxBIOSv3/util/lar/stream.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ LinuxBIOSv3/util/lar/stream.c	2007-07-11 11:48:23.000000000 -0600
@@ -0,0 +1,555 @@
+/*
+ * lar - LinuxBIOS archiver
+ *
+ * This includes code from previous versions of the LAR utility,
+ * including create.c, list.c, extract.c and bootblock.c
+ *
+ * Copyright (C) 2006-2007 coresystems GmbH
+ * (Written by Stefan Reinauer  for coresystems GmbH)
+ * Copyright (C) 2007 Patrick Georgi 
+ * Copyright (C) 2007 Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <netinet/in.h>
+#include <libgen.h>
+
+#include "lib.h"
+#include "lar.h"
+
+#define err(fmt,args...) fprintf(stderr, fmt, ##args)
+
+extern enum compalgo algo;
+
+static inline int get_bootblock_offset(int size)
+{
+	return size - (BOOTBLOCK_SIZE + sizeof(struct lar_header) + BOOTBLOCK_NAME_LEN);
+}
+
+static int lar_read_size(struct lar *lar, int filelen)
+{
+	unsigned int *ptr = (unsigned int *) (lar->map + (filelen - 12));
+	return ptr[0];
+}
+
+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';
+
+	p = (unsigned int *) (ptr + BOOTBLOCK_SIZE - 12);
+	p[0] = size;
+}
+
+int lar_add_bootblock(struct lar *lar, char *bootblock)
+{
+	unsigned char *offset;
+	struct lar_header *header;
+	int ret;
+
+	if (bootblock != NULL) {
+		struct stat s;
+
+	        ret = stat(bootblock, &s);
+
+		if (ret == -1) {
+			err("Unable to stat %s\n", bootblock);
+			return -1;
+		}
+
+		if (s.st_size != BOOTBLOCK_SIZE) {
+			err("Bootblock %s does not appear to be a bootblock.\n",
+			    bootblock);
+
+			return -1;
+		}
+	}
+
+	offset = lar->map + get_bootblock_offset(lar->size);
+	header = (struct lar_header *) offset;
+
+	memcpy(header->magic, MAGIC, 8);
+	header->reallen = htonl(BOOTBLOCK_SIZE);
+	header->len = htonl(BOOTBLOCK_SIZE);
+	header->offset = htonl(sizeof(struct lar_header) + BOOTBLOCK_NAME_LEN);
+
+	offset += sizeof(struct lar_header);
+	strcpy((char *) offset, BOOTBLOCK_NAME);
+
+	offset += BOOTBLOCK_NAME_LEN;
+
+	/* If a file waas specified, then load it, and read it directly into place */
+
+	if (bootblock != NULL) {
+
+		int fd = open(bootblock, O_RDONLY);
+
+		if (fd == -1) {
+			err("Unable to read bootblock file %s\n", bootblock);
+			return -1;
+		}
+
+		ret = read(fd, offset, BOOTBLOCK_SIZE);
+
+		close(fd);
+
+		if (ret != BOOTBLOCK_SIZE) {
+			err("Unable to read all the bytes in the bootblock file.\n");
+			return -1;
+		}
+	}
+
+	annotate_bootblock(offset, lar->size);
+	return 0;
+}
+
+static struct lar * _open_lar(const char *archive, unsigned int size, int flags)
+{
+	struct lar *lar = calloc(sizeof(*lar), 1);
+
+	if (lar == NULL) {
+		err("Unable to allocate memory.\n");
+		return NULL;
+	}
+
+	lar->fd = open(archive, flags, S_IRUSR | S_IWUSR);
+
+	if (lar->fd == -1) {
+		err("Couldn't open the archive %s\n", archive);
+		goto err;
+	}
+
+	/* Expand the file to the correct size */
+
+	if (lseek(lar->fd, size - 1, SEEK_SET) == -1) {
+	  err("Unable to write the archive %s\n", archive);
+	  goto err;
+	}
+
+	if (write(lar->fd, "", 1) != 1) {
+	  err("Unable to write the file %s\n", archive);
+	  goto err;
+	}
+
+	lar->map = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED,
+		lar->fd, 0);
+
+	lar->size = size;
+
+	if (lar->map == MAP_FAILED) {
+		err("Unable to map the archive %s\n", archive);
+		goto err;
+	}
+
+	return lar;
+
+ err:
+	if (lar->fd >= 0)
+		close(lar->fd);
+
+	unlink(archive);
+
+	if (lar)
+		free(lar);
+
+	return NULL;
+}
+
+static void _close_lar(struct lar *lar)
+{
+	munmap(lar->map, lar->size);
+	close(lar->fd);
+
+	free(lar);
+}
+
+void lar_close_archive(struct lar *lar)
+{
+	_close_lar(lar);
+}
+
+struct lar * lar_new_archive(const char *archive, unsigned int size)
+{
+	struct lar *lar;
+	int i;
+
+	if (!access(archive, F_OK)) {
+		err("Archive file %s already exists\n", archive);
+		return NULL;
+	}
+
+	lar = _open_lar(archive, size, O_RDWR | O_CREAT);
+
+	if (lar == NULL)
+		return NULL;
+
+	/* Fill the whole thing with flash friendly 0xFFs */
+	memset(lar->map, 0xFF, lar->size);
+
+	/* Make a dummy bootblock */
+
+	if (lar_add_bootblock(lar, NULL)) {
+		_close_lar(lar);
+		return NULL;
+	}
+
+	return lar;
+}
+
+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);
+
+	/* Sanity check - make sure the bootblock header is the same length as the LAR archive */
+
+	romlen = lar_read_size(lar, s.st_size);
+
+	if (romlen != s.st_size) {
+		err("Size mismatch - the header says %d but the file is %d bytes long.\n",
+		    romlen, (int) s.st_size);
+		_close_lar(lar);
+		return NULL;
+	}
+
+	return lar;
+}
+
+/* return the offset of the empty space in the LAR */
+
+static int lar_empty_offset(struct lar *lar)
+{
+	int offset = 0;
+	struct lar_header *header;
+
+	while (offset < get_bootblock_offset(lar->size)) {
+		header = (struct lar_header *) (lar->map + offset);
+
+		/* We interpet the absence of the magic as empty space */
+
+		if (strncmp(header->magic, MAGIC, 8))
+			break;
+
+		offset += (ntohl(header->len) + ntohl(header->offset) - 1)
+			& 0xfffffff0;
+	}
+
+	if (offset >= get_bootblock_offset(lar->size))
+		return -1;
+
+	return offset;
+}
+
+static int file_in_list(struct file *files, char *filename)
+{
+	struct file *p;
+
+	if (files == NULL)
+		return 1;
+
+	for(p = files ; p; p = p->next) {
+		if (!strcmp(p->name, filename))
+			return 1;
+	}
+
+	return 0;
+}
+
+void lar_list_files(struct lar *lar, struct file *files)
+{
+	unsigned char *ptr = lar->map;
+	char *filename;
+
+	struct lar_header *header;
+	struct file *fp;
+
+	while (ptr < (lar->map + get_bootblock_offset(lar->size))) {
+		header = (struct lar_header *) ptr;
+
+		/* We interpet the absence of the magic as empty space */
+
+		if (strncmp(header->magic, MAGIC, 8))
+			break;
+
+		filename = (char *) (ptr + sizeof(struct lar_header));
+
+		if (file_in_list(files, filename)) {
+			printf("  %s ", filename);
+
+			if (ntohl(header->compression) == none) {
+				printf("(%d bytes @0x%lx)\n",
+				       ntohl(header->len),
+				       (unsigned long)(ptr - lar->map) +
+				       ntohl(header->offset));
+			} else {
+				printf("(%d bytes, %s compressed to %d bytes "
+				       "@0x%lx)\n",
+				       ntohl(header->reallen),
+				       algo_name[ntohl(header->compression)],
+				       ntohl(header->len),
+				       (unsigned long)(ptr - lar->map) +
+				       ntohl(header->offset));
+			}
+		}
+
+		ptr += (ntohl(header->len) + ntohl(header->offset) - 1)
+			& 0xfffffff0;
+	}
+
+	/* Show the bootblock */
+
+	if (file_in_list(files, BOOTBLOCK_NAME)) {
+		header = (struct lar_header *)
+			(lar->map + get_bootblock_offset(lar->size));
+
+		printf("  bootblock (%d bytes @0x%x)\n",
+		       ntohl(header->len),
+		       get_bootblock_offset(lar->size) +
+		       ntohl(header->offset));
+	}
+}
+
+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;
+	}
+
+	ret = write(fd, buffer, len);
+
+	if (ret != len)
+		err("Error writingthe file %s\n", filename);
+
+	close(fd);
+	return (ret == len) ? 0 : -1;
+}
+
+int lar_extract_files(struct lar *lar, struct file *files)
+{
+	unsigned char *ptr = lar->map;
+	char *filename;
+	struct lar_header *header;
+	int ret = 0;
+
+	while (ptr < (lar->map + get_bootblock_offset(lar->size))) {
+
+		header = (struct lar_header *) ptr;
+
+		if (strncmp(header->magic, MAGIC, 8))
+			break;
+
+		filename = (char *) (ptr + sizeof(struct lar_header));
+
+		if (file_in_list(files, filename)) {
+
+			if (ntohl(header->compression) == none) {
+				ret = _write_file(filename,
+						  ptr + ntohl(header->offset),
+						  ntohl(header->len));
+			}
+			else {
+				unsigned char *buf =
+					malloc(ntohl(header->reallen));
+
+				if (buf == NULL) {
+					err("Out of memory.\n");
+					return -1;
+				}
+
+				uncompress_functions[ntohl(header->compression)](
+					(char*) buf,
+					(char *) ptr + ntohl(header->offset),
+					ntohl(header->len));
+
+				ret = _write_file(filename, buf,
+					ntohl(header->reallen));
+
+				free(buf);
+			}
+
+			if (ret == -1)
+				return -1;
+		}
+
+		ptr += (ntohl(header->len) + ntohl(header->offset) - 1)
+			& 0xfffffff0;
+	}
+
+	if (file_in_list(files, BOOTBLOCK_NAME)) {
+		header = (struct lar_header *)
+			(lar->map + get_bootblock_offset(lar->size));
+
+		ret = _write_file((char *) BOOTBLOCK_NAME,
+				  lar->map + (get_bootblock_offset(lar->size) + ntohl(header->offset)),
+				  BOOTBLOCK_SIZE);
+	}
+
+	return ret;
+}
+
+int lar_add_file(struct lar *lar, char *name)
+{
+	char *filename, *ptr, *temp;
+	enum compalgo thisalgo;
+	struct lar_header *header;
+	int offset, ret, fd, hlen;
+	u32 complen;
+	int pathlen;
+	struct stat s;
+	u32 *walk,  csum;
+
+	/* Find the beginning of the available space in the LAR */
+	offset = lar_empty_offset(lar);
+
+	thisalgo = algo;
+
+	filename = name;
+
+	if (strstr(name, "nocompress:") == name) {
+		filename += 11;
+		thisalgo = none;
+	}
+
+	if (filename[0] == '.' && filename[1] == '/')
+		filename += 2;
+
+	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);
+
+	if (fd == -1) {
+		err("Unable to open %s\n", filename);
+		free(temp);
+		return -1;
+	}
+
+	ptr = mmap(0, s.st_size, PROT_READ, MAP_SHARED, fd, 0);
+
+	if (ptr == MAP_FAILED) {
+		err("Unable to map %s\n", filename);
+		close(fd);
+		free(temp);
+		return -1;
+	}
+
+
+	/* 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);
+	}
+
+	munmap(ptr, s.st_size);
+	close(fd);
+
+	pathlen = strlen(filename) + 1 > MAX_PATHLEN ? MAX_PATHLEN : strlen(filename) + 1;
+
+	/* Figure out how big our header will be */
+	hlen = sizeof(struct lar_header) + pathlen;
+	hlen = (hlen + 15) & 0xFFFFFFF0;
+
+	if (offset + hlen + complen >= get_bootblock_offset(lar->size)) {
+		err("Not enough room in the LAR to add the file.\n");
+		free(temp);
+		return -1;
+	}
+
+	/* Lets do this thing */
+
+	/* Zero out the header area */
+	memset(lar->map + offset, 0, hlen);
+
+	header = (struct lar_header *) (lar->map + offset);
+
+	memcpy(header, MAGIC, 8);
+	header->compression = htonl(thisalgo);
+	header->reallen = htonl(s.st_size);
+	header->len = htonl(complen);
+	header->offset = htonl(hlen);
+
+	/* Copy the path name */
+	strncpy((char *) (lar->map + offset + sizeof(struct lar_header)),
+		filename, pathlen - 1);
+
+	/* Copy in the data */
+	memcpy(lar->map + (offset + hlen), temp, complen);
+
+	/* Figure out the checksum */
+
+	csum = 0;
+	for (walk = (u32 *) (lar->map + offset);
+	     walk < (u32 *) (temp + complen + hlen);
+	     walk++) {
+		csum += ntohl(*walk);
+	}
+	header->checksum = htonl(csum);
+
+	free(temp);
+	return 0;
+}

[Peter Stuge]

First some general discussion. More concrete patch comments will
follow.

On Wed, Jul 11, 2007 at 12:15:17PM -0600, Jordan Crouse wrote:
> This has the slight side effect of requiring that the user specify
> a size for the LAR when they create it, but I don't think thats
> really a bad thing.
>
> One thing you might miss is that I've turned the path name of the
> bootblock into a constant name 'bootblock'.

Some ideas have been tossed around;

1. teach lar about flash chip sector sizes
2. bootblock at top or at bottom

1 means combining flashrom and lar somehow.. Thoughts?

Unfortunately this isn't as easy as a straight modulus operation
because not all flash chips have uniform sector sizes across the
entire chip.

The only way I can see it working is to create a lar for a flash chip
rather than for a size. The flash chip id could be at the end of the
lar (we have a few bytes left right?) and lars could also be generic,
ie. not yet adapted to any chip. This generic lar is what we do right
now, with a fixed 8k-or-so sector size and a fixed total size.

We would also want a flag for each file indicating whether it can be
moved or if it must remain at it's current offset. This flag limits
which lar files can be reflashed independently by flashrom.

And we'd need to add some lar knowledge to flashrom, but only enough
to pick files out of lars.

2 depends on the target architecture. If we can autodetect we should
not make it an option, just stick with assuming top for now and fix
when it's needed.
> It also requires that the user specify a size when the LAR is
> created.

So we're approaching the flash chip. Can we do something more clever
than just specify the size already? I'm not sure. I think a fixed
size is fine for now.

//Peter

[Segher Boessenkool]

> Some ideas have been tossed around;
> teach lar about flash chip sector sizes
> bootblock at top or at bottom
>
> 1 means combining flashrom and lar somehow.. Thoughts?

I think the boot block should be outside of the LAR completely.
A normal LinuxBIOS upgrade shouldn't touch the bootblock; that
way, recovery code in there can help the user load a working
image into flash after some screw-up, over a serial port or
whatever.

This also handily solves the current problem :-)

Segher

[Peter Stuge]

On Thu, Jul 12, 2007 at 02:16:21AM +0200, Segher Boessenkool wrote:
> > 1 means combining flashrom and lar somehow.. Thoughts?
>
> I think the boot block should be outside of the LAR completely.

Hmm. Interesting!
> A normal LinuxBIOS upgrade shouldn't touch the bootblock;

I agree. That's why lar should know about flash chip sector sizes.

And/or flashrom about larballs.

The idea is to rearrange the files in the larball apropriately for
the current flash chip before writing.
> that way, recovery code in there can help the user load a working
> image into flash after some screw-up, over a serial port or
> whatever.
>
> This also handily solves the current problem :-)

But it creates another quite real problem for the first upgrade to LB
where we need to distribute the bootblock separately. I think that's
a _really_ undesirable scenario. :\ Especially since it will be the
common case for some time still.

One file is no file, two files are two files too many. ;)

//Peter

[LinuxBIOS] New LAR access functions

[Jordan Crouse]

On 12/07/07 03:05 +0200, Peter Stuge wrote:
> On Thu, Jul 12, 2007 at 02:16:21AM +0200, Segher Boessenkool wrote:
> > > 1 means combining flashrom and lar somehow.. Thoughts?
> >
> > I think the boot block should be outside of the LAR completely.
>
> Hmm. Interesting!
> > A normal LinuxBIOS upgrade shouldn't touch the bootblock;
>
> I agree. That's why lar should know about flash chip sector sizes.

This is probably best - lar is best suited for doing the math and
making sure that the blobs are written out in the right format. Somewhat
safer then making flashrom rearrange the bits later.

I'm thinking that we could probably get away with someting simple like
a -S <sector size> option, and LAR would would arrange the archive
correctly, and then write the sector size along with the total ROM size
in the "bootblock header".
> And/or flashrom about larballs.

flashrom should at the very least know how to read a lar and make sure
it is sane.  If we include the sector size from above, then flashrom 
could do some very basic sanity checking before starting.
> > that way, recovery code in there can help the user load a working
> > image into flash after some screw-up, over a serial port or
> > whatever.
> >
> > This also handily solves the current problem :-)
>
> But it creates another quite real problem for the first upgrade to LB
> where we need to distribute the bootblock separately. I think that's
> a _really_ undesirable scenario. :\ Especially since it will be the
> common case for some time still.
>
> One file is no file, two files are two files too many. ;)

I completely agree.  Its always best to only have a single deliverable.

Jordan

[LinuxBIOS] New LAR access functions

[Peter Stuge]

On Thu, Jul 12, 2007 at 09:27:49AM -0600, Jordan Crouse wrote:
> > I agree. That's why lar should know about flash chip sector sizes.
>
> This is probably best - lar is best suited for doing the math and
> making sure that the blobs are written out in the right format.
> Somewhat safer then making flashrom rearrange the bits later.

I agree. But..
> I'm thinking that we could probably get away with someting simple
> like a -S <sector size> option, and LAR would would arrange the
> archive correctly, and then write the sector size along with the
> total ROM size in the "bootblock header".

..flash doesn't always have uniform sector sizes. E.g. SST49LF080A
http://www.sst.com/downloads/datasheet/S71235.pdf which has 16*4kb at
the bottom and 15*64kb for the rest. It would be nice to be able to
use those 16*4kb sectors for different things and not always need to
treat them as a single 64kb sector. -S is a great start though.
> > And/or flashrom about larballs.
>
> flashrom should at the very least know how to read a lar and make
> sure it is sane.  If we include the sector size from above, then
> flashrom could do some very basic sanity checking before starting.

The real fun begins when larball has one sector size and flash chip
another, and we only want to replace a single larfile.

//Peter

[LinuxBIOS] New LAR access functions

[Stefan Reinauer]

* Peter Stuge  [070712 18:21]:
> > flashrom should at the very least know how to read a lar and make
> > sure it is sane.  If we include the sector size from above, then
> > flashrom could do some very basic sanity checking before starting.
>
> The real fun begins when larball has one sector size and flash chip
> another, and we only want to replace a single larfile.

So the question is: Who should reorganize the lar file so it uses the
chip optimally? If it is flashrom we might lock out other bios flashers
to be used with linuxbios.

Stefan

[LinuxBIOS] New LAR access functions

[Peter Stuge]

On Thu, Jul 12, 2007 at 07:02:51PM +0200, Stefan Reinauer wrote:
> > The real fun begins when larball has one sector size and flash
> > chip another, and we only want to replace a single larfile.
>
> So the question is: Who should reorganize the lar file so it uses
> the chip optimally? If it is flashrom we might lock out other bios
> flashers to be used with linuxbios.

I dislike flashrom for this mostly because it basically needs all of
lar, re other flashers they can always flash the complete image and
be done.

The KISS approach may be the best;

flashrom exits with an error telling root to reorganize using lar.

flashrom of course suggests the correct lar arguments for the
detected flash chip. This is -S magicsize to begin with.

Then user has to run lar to repack, then flashrom again to replace
single larfiles in flash.

//Peter

[LinuxBIOS] New LAR access functions

[Stefan Reinauer]

* Peter Stuge  [070712 19:12]:
> I dislike flashrom for this mostly because it basically needs all of
> lar, re other flashers they can always flash the complete image and
> be done.

I agree. But it seems a bit nasty. Either flash chip knowledge in lar,
or lar knowledge in flashrom. Both is not very nice.
> The KISS approach may be the best;
>
> flashrom exits with an error telling root to reorganize using lar.
>
> flashrom of course suggests the correct lar arguments for the
> detected flash chip. This is -S magicsize to begin with.
>
> Then user has to run lar to repack, then flashrom again to replace
> single larfiles in flash.

In case lar is in the path, flashrom could be told to run lar with the
options for the user, just asking the user whether he really wants to
continue (with a no warranty clause printed in capital letters ;)

[LinuxBIOS] New LAR access functions

[Peter Stuge]

On Thu, Jul 12, 2007 at 07:42:42PM +0200, Stefan Reinauer wrote:
> Peter Stuge  [070712 19:12]:
> > I dislike flashrom for this mostly because it basically needs all of
> > lar, re other flashers they can always flash the complete image and
> > be done.
>
> I agree. But it seems a bit nasty. Either flash chip knowledge in
> lar, or lar knowledge in flashrom. Both is not very nice.

I honestly think it's an awesome feature, never before seen (by me)
in any bootloader flash tools.

But that's no reason to put _all_ of lar in flashrom. I do think they
can and should share some code, but they still need to be separate
tools. Ie. flashrom will use lar code to read larballs but never
anything to write them. Maybe liblar?
> > The KISS approach may be the best;
>
> In case lar is in the path, flashrom could be told to run lar with
> the options for the user, just asking the user whether he really
> wants to continue (with a no warranty clause printed in capital
> letters ;)

Yes, definately. Good idea. As long as flashrom doesn't go flashing
even if lar fails it's all good!

//Peter

[LinuxBIOS] New LAR access functions

[Stefan Reinauer]

* Jordan Crouse  [070712 17:27]:
> > > A normal LinuxBIOS upgrade shouldn't touch the bootblock;
> >
> > I agree. That's why lar should know about flash chip sector sizes.
>
> This is probably best - lar is best suited for doing the math and
> making sure that the blobs are written out in the right format. Somewhat
> safer then making flashrom rearrange the bits later.

Actually both lar and flashrom should be able to read flash chip
description files.
> I'm thinking that we could probably get away with someting simple like
> a -S <sector size> option, and LAR would would arrange the archive
> correctly, and then write the sector size along with the total ROM size
> in the "bootblock header".

Sectors might have different sizes. Not sure whether this "assymetrical"
concept will be kept up in the future. Many modern chips are
"symmetrical", ie have all equal sized blocks.
> flashrom should at the very least know how to read a lar and make sure
> it is sane.  If we include the sector size from above, then flashrom 
> could do some very basic sanity checking before starting.

It would be nice if flashrom knows how to flash a part of a lar. ie
"only normal, not fallback" But 90% of flashrom's flash chip drivers
dont know how to do that, and it would be quite some redesign to change
it.
> > But it creates another quite real problem for the first upgrade to LB
> > where we need to distribute the bootblock separately. I think that's
> > a _really_ undesirable scenario. :\ Especially since it will be the
> > common case for some time still.
> >
> > One file is no file, two files are two files too many. ;)
>
> I completely agree.  Its always best to only have a single deliverable.

It should be viable to flash "everything but the bootblock" in lar. But
for deployment a complete image is what we really want. The fact that
the bootblock is part of the lar is just artificial to make the handling
nicer.

[LinuxBIOS] New LAR access functions

[Peter Stuge]

On Thu, Jul 12, 2007 at 07:00:44PM +0200, Stefan Reinauer wrote:
> > > I agree. That's why lar should know about flash chip sector sizes.
> >
> > This is probably best - lar is best suited for doing the math and
>
> Actually both lar and flashrom should be able to read flash chip
> description files.

Re these files, I desperately want a fallback so that something
intelligent can be done also without any flash description files.
Again single/many deliverables/components. Copying those small files
around can be a huge hassle. I would love to compile the latest list
available into the binary at build time, and allow a newer text file
to override it at run time. (Maybe even replace in the binary with
ELF tricks? :)
> Sectors might have different sizes. Not sure whether this
> "assymetrical" concept will be kept up in the future. Many modern
> chips are "symmetrical", ie have all equal sized blocks.

Hard to say. Sharing of single flash chips between multiple readers
on a single board supports having varying sector sizes, but lower
cost probably supports uniform sector sizes. OTOH flash may be so
cheap anyway that the little extra cost for varying sector sizes win.

I like the varying sector sizes a lot because it means we can use the
flash as NV storage in more ways, and I think we need the space.
> It would be nice if flashrom knows how to flash a part of a lar. ie
> "only normal, not fallback"

Definately a worthwhile goal!
> But 90% of flashrom's flash chip drivers dont know how to do that,
> and it would be quite some redesign to change it.

At least they know about sectors and page sizes already.
> > > One file is no file, two files are two files too many. ;)
> >
> > I completely agree.  Its always best to only have a single
> > deliverable.
>
> It should be viable to flash "everything but the bootblock" in lar.

Good idea! It should even be the default. If flashrom detects that
the boot block sector already has an LB bootblock it doesn't
erase/flash it unless explicitly told to do so.

Note this means the bootblock eats up 64kb of SST49LF080A chips.
Plenty of room for serial recovery yay. :)
> But for deployment a complete image is what we really want. The
> fact that the bootblock is part of the lar is just artificial to
> make the handling nicer.

Agreed.

//Peter

[LinuxBIOS] New LAR access functions

[Stefan Reinauer]

* Peter Stuge  [070712 19:25]:
> Re these files, I desperately want a fallback so that something
> intelligent can be done also without any flash description files.

This could just be assume a block size of 4, 8 or 16k for the archive.
Currently blocks are aligned to 16 bytes. Smallest sector size I have
seen was 256 bytes I think.
> Again single/many deliverables/components. Copying those small files
> around can be a huge hassle. I would love to compile the latest list
> available into the binary at build time, and allow a newer text file
> to override it at run time. (Maybe even replace in the binary with
> ELF tricks? :)

objcopy is your friend here. Hey, we could pack the descriptions into a
lar file and link that into the binary ;)
> Hard to say. Sharing of single flash chips between multiple readers
> on a single board supports having varying sector sizes, but lower
> cost probably supports uniform sector sizes. OTOH flash may be so
> cheap anyway that the little extra cost for varying sector sizes win.

What does SPI do? Just like DIP, PLCC will probably go away in some ys.
(By the time we have large flash chips ;-)
> I like the varying sector sizes a lot because it means we can use the
> flash as NV storage in more ways, and I think we need the space.

Indeed.
> > But 90% of flashrom's flash chip drivers dont know how to do that,
> > and it would be quite some redesign to change it.
>
> At least they know about sectors and page sizes already.

Do they? ;) Not for the non-uniform sizes at least...

Usually flashing works like this:

erase all of the chip with jedec
rewrite it with whatever function is defined to do so
> > It should be viable to flash "everything but the bootblock" in lar.
>
> Good idea! It should even be the default. If flashrom detects that
> the boot block sector already has an LB bootblock it doesn't
> erase/flash it unless explicitly told to do so.

yes.
> Note this means the bootblock eats up 64kb of SST49LF080A chips.
> Plenty of room for serial recovery yay. :)

:-) On the 080A we can almost say we have that space. Our bootblock is
already pretty large (16k).. I wonder whether we can shrink it. (Before
we make it big again.. :)

[LinuxBIOS] New LAR access functions

[Peter Stuge]

On Thu, Jul 12, 2007 at 07:59:42PM +0200, Stefan Reinauer wrote:
> Peter Stuge  [070712 19:25]:
> > Re these files, I desperately want a fallback so that something
> > intelligent can be done also without any flash description files.
>
> This could just be assume a block size of 4, 8 or 16k for the
> archive. Currently blocks are aligned to 16 bytes. Smallest sector
> size I have seen was 256 bytes I think.

Block size could be 1 until the larball needs to be repacked to match
the flash chip contets so that flashrom can reflash a single file
from it. Then block sizes need to match the flash chip.

It could be argued that flashrom shouldn't try to fix the larball
since this is an error in the process preceding the flashrom
invocation and the process should be fixed to produce correct
larballs for the target flash chip?
> > I would love to compile the latest list available into the binary
> > at build time, and allow a newer text file to override it at run
> > time. (Maybe even replace in the binary with ELF tricks? :)
>
> objcopy is your friend here. Hey, we could pack the descriptions
> into a lar file and link that into the binary ;)

I like it.
> > Hard to say. Sharing of single flash chips between multiple readers
> > on a single board supports having varying sector sizes, but lower
> > cost probably supports uniform sector sizes. OTOH flash may be so
> > cheap anyway that the little extra cost for varying sector sizes win.
>
> What does SPI do? Just like DIP, PLCC will probably go away in some
> ys. (By the time we have large flash chips ;-)

Fortunately this doesn't depend on the interface. It's just about how
the memory is organized internally.
> > At least they know about sectors and page sizes already.
>
> Do they? ;) Not for the non-uniform sizes at least...

True.
> Usually flashing works like this:
>
> erase all of the chip with jedec
> rewrite it with whatever function is defined to do so

The few drivers I just checked didn't do chip erase but did sector
erase over the entire chip instead. (Chip erase wasn't available at
all times.)

Yes, it needs a bit of hacking. No doubt.
> > Note this means the bootblock eats up 64kb of SST49LF080A chips.
> > Plenty of room for serial recovery yay. :)
>
> :-) On the 080A we can almost say we have that space.

Agreed. Not much to do anyway. Just make the most of the space.
> Our bootblock is already pretty large (16k).. I wonder whether we
> can shrink it. (Before we make it big again.. :)

That would be nice. What takes up 16k?

//Peter

[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

[Peter Stuge]

On Thu, Jul 12, 2007 at 12:49:20AM +0200, Peter Stuge wrote:
> > mkdirp((const char *) dirname(path), 0755);
> I don't think lar should mkdir -p implicitly. Just open() and
> perror() or print strerror(errno) in case of failure.

Sorry. Since we don't store directories in lar it should indeed mkdir
-p implicitly. But I think a bit of sanity would be nice here since a
lar could otherwise be used to overwrite arbitrary system files.

I'll make a patch for mkdirp() that ensures the directory to be
created is actually below the current directory (and also improve the
return-to-cwd code in mkdir() a bit while at it) if there's interest.
Would that be the right place to put it?

//Peter

[Stefan Reinauer]

* Peter Stuge  [070712 00:58]:
> Sorry. Since we don't store directories in lar it should indeed mkdir
> -p implicitly. But I think a bit of sanity would be nice here since a
> lar could otherwise be used to overwrite arbitrary system files.

You compile LinuxBIOS as root?
> I'll make a patch for mkdirp() that ensures the directory to be
> created is actually below the current directory (and also improve the
> return-to-cwd code in mkdir() a bit while at it) if there's interest.
> Would that be the right place to put it?

Rather check the path before mkdir()ing.

I am pretty sure the mkdir efforts can easily be tricked by a couple of
symlinks in the path, so I wonder how much use there is in trying to
make this "secure" since it never runs as root anyways, and in a very
controlled environment.

Stefan

[Peter Stuge]

On Thu, Jul 12, 2007 at 04:49:35PM +0200, Stefan Reinauer wrote:
> > > mkdirp((const char *) dirname(path), 0755);
> >
> > I don't think lar should mkdir -p implicitly.
>
> Oh it does not do this implicitly. It only does it if the archive
> explicitly contains a directory.

It needs to be there, but I think the sanity checking could be good.

On Thu, Jul 12, 2007 at 04:52:12PM +0200, Stefan Reinauer wrote:
> > But I think a bit of sanity would be nice here since a lar could
> > otherwise be used to overwrite arbitrary system files.
>
> You compile LinuxBIOS as root?

No, but I may run lar as root because I need to tweak something right
before running flashrom as root. Yes, bad practice. No, I'm not the
only one.
> > I'll make a patch for mkdirp() that ensures the directory to be
> > created is actually below the current directory
>
> Rather check the path before mkdir()ing.

Exactly the idea.
> I am pretty sure the mkdir efforts can easily be tricked by a
> couple of symlinks in the path,

realpath() handles that. To do full path resolution is not so simple
though, so I cut some corners. It may be too much effort to be at all
worthwhile and we'll instead let root shoot foot.
> so I wonder how much use there is in trying to make this "secure"
> since it never runs as root anyways, and in a very controlled
> environment.

LB build mostly yes, lar not so sure.

//Peter

[Stefan Reinauer]

* Peter Stuge  [070712 18:43]:
> > Rather check the path before mkdir()ing.
>
> Exactly the idea.

Great. Instead of calling mkdirp with ".", can we assume "." is always
the starting point? (I think it is not called differently at any other
point)

Stefan

[Peter Stuge]

On Thu, Jul 12, 2007 at 07:26:23PM +0200, Stefan Reinauer wrote:
> > > Rather check the path before mkdir()ing.
> >
> > Exactly the idea.

Maybe I should explain mkdirp_below() a bit more.

It parses the new path one subdir at a time (as separated by / or )
and calls realpath() to resolve it into an absolute path.

If realpath() fails for some other reason than the path not existing,
return an error.

If the resolved absolute path is outside the specified parent
directory, also return an error.

Otherwise, the new dir is created (no error if it already exists) and
iterate to the next subdir.

Since realpath() only works with existing paths, only one dir can be
checked at a time. This means subdir/../subdir will be considered to
be outside subdir because subdir/.. in the second iteration tests
false.

The only way to properly handle this is to do full path resolution
per path_resolution(2) but that is too big of a wheel to re-invent.

subdir/.. constructs could be removed, but symlinks must also be
checked then.
> Great. Instead of calling mkdirp with ".", can we assume "." is
> always the starting point? (I think it is not called differently
> at any other point)

It's not, but it might be with a new -C option? Or should that call
chdir() before extracting any files? I generally avoid chdir() since
all of a sudden something needs to be done back in the startup
working directory.

Simpler to chdir() and use ready-made paths (if they're relative)
than to copy stuff around to create "relocated" paths though.

Maybe I'm too strict?

//Peter

[LinuxBIOS] New LAR access functions

[Jordan Crouse]

On 12/07/07 00:49 +0200, Peter Stuge wrote:
> 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?

I guess, though I'll bet this code doesn't survive long enough to
see those 2G ROM chips.. :)
> > +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?

lar->map is a uchar, which is a size of 1, so the math works.   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.
> > +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..

Oops - I copied that without thinking.  Yeah - memset best.
> > 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.  I guess I 
could use u32 instead - but thats just going to end up decoding back down
to unsigned int.    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.
> > 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 - failure to malloc lar: on line 132 will immediately
return.  err: is there specifically to cleanup lar->fd.
> > +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.

Thats a good idea.
> > /* 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?

Hmm - sure.
> > +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.

Okay, I agree.  It will be a little bit more complex, but its probably
better.
> 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?
> > +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".
> > 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.
> > +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.

This came from the original code - I think that Stefan and Patrick were
going for the same behavior as tar - and tar definately does implicitly
use mkdir -p on extract.
> > 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. :)

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.
> > if (strncmp(header->magic, MAGIC, 8))
> > 	break;
>
> Is strncmp() good for MAGIC ? Maybe memcmp() ?

Yes - memcmp is better.
> > 	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.
> > if (strstr(name, "nocompress:") == name) {
> > filename += 11;
> > thisalgo = none;
> > }
>
> strncmp() ?

Yeah - I guess either one would be ok.  Should go with
strncmp - its probably faster.
> > 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.  I tried to squeeze
the filename : pathname stuff in here without making too many changes - 
perhaps we should revisit that.
> > 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.

Got it.
> > /* 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?

I think it does it to avoid complex logic - but we can look at this 
again - there's probably cleanup that can happen here.

Thanks for your comments.
Jordan

[LinuxBIOS] New LAR access functions

[Peter Stuge]

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

[LinuxBIOS] New LAR access functions

[Jordan Crouse]

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

[LinuxBIOS] New LAR access functions

[Stefan Reinauer]

* Jordan Crouse  [070712 17:34]:
> > 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.

Can you resend that patch in the latest version so we can get it in?

Stefan

[LinuxBIOS] New LAR access functions

[Jordan Crouse]

On 22/07/07 18:59 +0200, Stefan Reinauer wrote:
> Jordan Crouse  [070712 17:34]:
> > > 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.
>
> Can you resend that patch in the latest version so we can get it in?

Yes - sorry, I was away over the weekend at a funeral.  New patches
will be coming soon.

Jordan

[Stefan Reinauer]

* Peter Stuge  [070712 00:49]:
> > +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?

This code is not new, it is in another file already.
> Doesn't adding 1 to a pointer actually add sizeof pointed-to-type to
> the address? Ie. this code isn't portable?

What makes you think so? It fetches an int from 12 bytes before the end
of the file (ie. after the reset vector).

It is of course not portable because there is no reset vector at
fffffff0 on non x86 machines. But that's another issue.
> 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?

int is 32bit on all GNU architectures I know that could theoretically
run LinuxBIOS. But since this is an x86 only thing anyways I would not
care too much.
> > 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?

What kind of situation that realisticly happens would produce such an
issue? i.e. While a payload builds a lar, or while LinuxBIOS is
compiled?
> > 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.

Oh it does not do this implicitly. It only does it if the archive
explicitly contains a directory.

Stefan

[Peter Stuge]

On Thu, Jul 12, 2007 at 04:49:35PM +0200, Stefan Reinauer wrote:
> > > unsigned int *ptr = (unsigned int *) (lar->map + (filelen - 12));
> >
> > What about all this pointer arithmetic? Is it really correct?
>
> This code is not new, it is in another file already.

Yes. Sorry. It's fine.
> > Doesn't adding 1 to a pointer actually add sizeof pointed-to-type
> > to the address? Ie. this code isn't portable?
>
> What makes you think so? It fetches an int from 12 bytes before the
> end of the file (ie. after the reset vector).

I did not pay attention to the type of lar->map.
> It is of course not portable because there is no reset vector
> at fffffff0 on non x86 machines. But that's another issue.

..
> > but then writing size to p[0] will write a different number of
> > bytes on different archs, right?
>
> int is 32bit on all GNU architectures I know that could
> theoretically run LinuxBIOS.

My point is that this is lar and not LB. While LB is completely tied
to the architecture we want lar to be the opposite. I expect lar and
larballs to be platform independent, just like other archivers and
good file systems. :)
> But since this is an x86 only thing anyways I would not care too
> much.

lar shouldn't be an x86 only thing, should it?
> > > lar =  _open_lar(archive, s.st_size, O_RDWR);
> >
> > Race conditions. First open() then fstat() to get the size.
>
> What kind of situation that realisticly happens would produce such
> an issue? i.e. While a payload builds a lar, or while LinuxBIOS is
> compiled?

Interactive buildrom and automated v3 abuild running at the same
time e.g. (In the same place.)

Yes, it's rather theoretical and there's no point in jumping through
hoops trying to recover from it - I'm requesting a way to measure
that the larball was broken by someone else and that lar doesn't
create a crazy big file.

//Peter

[Stefan Reinauer]

* Peter Stuge  [070712 18:37]:
> > int is 32bit on all GNU architectures I know that could
> > theoretically run LinuxBIOS.
>
> My point is that this is lar and not LB. While LB is completely tied
> to the architecture we want lar to be the opposite. I expect lar and
> larballs to be platform independent, just like other archivers and
> good file systems. :)

Sure. But this is the code that fixes up the x86 bootblock in a lar
file. How many other uses on non x86 systems do you expect to see over
time for this very purpose?
You are right though, this stuff should work on non-x86 platforms, in
case someone is cross compiling for x86.
> lar shouldn't be an x86 only thing, should it?

Since LinuxBIOSv3 is x86 only at the moment, I am not sure we should try
to imply something that is not there. The rest of the code is rather
portable, but creating x86 reset vectors is something very specific to
the x86 platform, no matter how much we abstract it. 
When we add support for a second platform, this code should of course be
executed conditionally, in case the image is going to be an x86 bios
image. on some platform the bootblock will not be part of the lar as the reset
vector is 0 and so there is no room for a lar header before that.

lar images will be linuxbios images in 99.99% of the time. I agree we
want this to be flexible, but nobody is ever gonna use it to archive her
mp3s (well who knows... ;)
> > What kind of situation that realisticly happens would produce such
> > an issue? i.e. While a payload builds a lar, or while LinuxBIOS is
> > compiled?
>
> Interactive buildrom and automated v3 abuild running at the same
> time e.g. (In the same place.)
>
> Yes, it's rather theoretical and there's no point in jumping through
> hoops trying to recover from it - I'm requesting a way to measure
> that the larball was broken by someone else and that lar doesn't
> create a crazy big file.

Oh not at all, this is a very good example. You are right. We should
think of this.

[Stefan Reinauer]

* Jordan Crouse  [070711 20:15]:
> This takes the code previous in create.c, extract.c, list.c
> and bootblock.c, and consolidates them into one file.

Why?