[PATCH] v3: print name of compression algorithm

[Carl-Daniel Hailfinger]

Print name of compression algorithm in addition to the corresponding 
number during boot.
Convert process_file() to use enum compalgo instead of hardcoded 
"1","2","3" and change the control structure from a series of if() 
statements to a switch() statement.

Compile and boot tested on Qemu.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3-algonaming/include/lar.h
===================================================================
--- LinuxBIOSv3-algonaming/include/lar.h	(Revision 605)
+++ LinuxBIOSv3-algonaming/include/lar.h	(Arbeitskopie)
@@ -74,6 +74,24 @@
    u64 loadaddress;
 };

+enum compalgo {
+	none = 0,
+	lzma = 1,
+	nrv2b = 2,
+	zeroes = 3,
+	/* invalid should always be the last entry. */
+	invalid
+};
+
+static const char *algo_name[] = {
+	"",
+	"lzma",
+	"nrv2b",
+	"zeroes",
+	/* invalid should always be the last entry. */
+	"invalid"
+};
+
 struct mem_file {
    void *start;
    int len;
Index: LinuxBIOSv3-algonaming/lib/lar.c
===================================================================
--- LinuxBIOSv3-algonaming/lib/lar.c	(Revision 605)
+++ LinuxBIOSv3-algonaming/lib/lar.c	(Arbeitskopie)
@@ -147,36 +147,41 @@

int process_file(const struct mem_file *archive, void *where)
 {
-	printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", archive->compression);
+	const char *algoname = algo_name[(archive->compression >= invalid)
+					? invalid : archive->compression];
+	printk(BIOS_SPEW, "LAR: Compression algorithm #%i (%s) used\n",
+		archive->compression, algoname);
+	switch (archive->compression) {
    /* no compression */
-	if (archive->compression == 0) {
+	case none:
    	memcpy(where, archive->start, archive->len);
    	return 0;
-	}
 #ifdef CONFIG_COMPRESSION_LZMA
    /* lzma */
-	unsigned long ulzma(unsigned char *src, unsigned char *dst);
-	if (archive->compression == 1) {
+	case lzma: {
+		unsigned long ulzma(unsigned char *src, unsigned char *dst);
    	ulzma(archive->start, where);
    	return 0;
    }
 #endif
 #ifdef CONFIG_COMPRESSION_NRV2B
    /* nrv2b */
-	unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
-	if (archive->compression == 2) {
+	case nrv2b: {
+		unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
    	unsigned long tmp;
    	unrv2b(archive->start, where, &tmp);
    	return 0;
    }
 #endif
    /* zeroes */
-	if (archive->compression == 3) {
+	case zeroes:
    	memset(where, 0, archive->reallen);
    	return 0;
+	default:
+		printk(BIOS_INFO, "LAR: Compression algorithm #%i (%s) not"
+			" supported!\n", archive->compression, algoname);
+		return -1;
    }
-	printk(BIOS_INFO, "LAR: Compression algorithm #%i not supported!\n", archive->compression);
-	return -1;
 }

/**
Index: LinuxBIOSv3-algonaming/util/lar/lar.h
===================================================================
--- LinuxBIOSv3-algonaming/util/lar/lar.h	(Revision 605)
+++ LinuxBIOSv3-algonaming/util/lar/lar.h	(Arbeitskopie)
@@ -92,7 +92,14 @@
    u32 size; /**< Size of the mmaped file */
 };

-enum compalgo { none = 0, lzma = 1, nrv2b = 2, zeroes = 3 };
+enum compalgo {
+	none = 0,
+	lzma = 1,
+	nrv2b = 2,
+	zeroes = 3,
+	/* invalid should always be the last entry. */
+	invalid
+};

typedef void (*compress_func) (char *, int, char *, int *);
 typedef void (*uncompress_func) (char *, int, char *, int);
@@ -128,4 +135,6 @@
    "lzma",
    "nrv2b",
    "zeroes",
+	/* invalid should always be the last entry. */
+	"invalid"
 };

[Peter Stuge]

On Sun, Feb 17, 2008 at 12:34:24AM +0100, Carl-Daniel Hailfinger wrote:
> +enum compalgo {
> none = 0,
> lzma = 1,
> nrv2b = 2,
> zeroes = 3,
> /* invalid should always be the last entry. */
> invalid
>
> +};

It's fairly common to uppercase these. Do we want that too?
> const char *algoname = algo_name[(archive->compression >= invalid)
> 			? invalid : archive->compression];

Not sure about invalid. I would be less worried if invalid was set to
some explicit value, e.g. -1. The current proposal means we can never
remove algorithms.

//Peter

[Carl-Daniel Hailfinger]

On 17.02.2008 05:02, Peter Stuge wrote:
> On Sun, Feb 17, 2008 at 12:34:24AM +0100, Carl-Daniel Hailfinger wrote:
> > +enum compalgo {
> > none = 0,
> > lzma = 1,
> > nrv2b = 2,
> > zeroes = 3,
> > /* invalid should always be the last entry. */
> > invalid
> >
> > +};
>
> It's fairly common to uppercase these. Do we want that too?

This was just a straight copy from util/lar, but I agree. We use these 
enums like #defines, so uppercasing seems indeed the best way to bring 
this hint to the developer.
> > const char *algoname = algo_name[(archive->compression >= invalid)
> > 			? invalid : archive->compression];
>
> Not sure about invalid. I would be less worried if invalid was set to
> some explicit value, e.g. -1.

We use archive->compression as array index. The line above is there to 
make sure we do not dereference a random pointer outside the array. That 
also rules out a negative value for invalid because accessing arrays 
with negative indices is real scary EVIL.
> The current proposal means we can never remove algorithms.

How so? We have to keep the number reserved if we ever abandon a 
compression algorithm, but that's it.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 17.02.2008 13:02, Carl-Daniel Hailfinger wrote:
> On 17.02.2008 05:02, Peter Stuge wrote:
> > On Sun, Feb 17, 2008 at 12:34:24AM +0100, Carl-Daniel Hailfinger wrote:
> > > +enum compalgo {
> > > none = 0,
> > > lzma = 1,
> > > nrv2b = 2,
> > > zeroes = 3,
> > > /* invalid should always be the last entry. */
> > > invalid
> > >
> > > +};
> >
> > It's fairly common to uppercase these. Do we want that too?
>
> This was just a straight copy from util/lar, but I agree. We use these 
> enums like #defines, so uppercasing seems indeed the best way to bring 
> this hint to the developer.

Uppercasing also found a name clash between NONE as compression algo and 
NONE as operation mode of util/lar.

Print name of compression algorithm in addition to the corresponding 
number during boot.
Convert process_file() to use enum compalgo instead of hardcoded 
"1","2","3" and change the control structure from a series of if() 
statements to a switch() statement.

Compile and boot tested on Qemu.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv3-algonaming/include/lar.h
===================================================================
--- LinuxBIOSv3-algonaming/include/lar.h	(Revision 605)
+++ LinuxBIOSv3-algonaming/include/lar.h	(Arbeitskopie)
@@ -74,6 +74,15 @@
    u64 loadaddress;
 };

+enum compalgo {
+	ALGO_NONE = 0,
+	ALGO_LZMA = 1,
+	ALGO_NRV2B = 2,
+	ALGO_ZEROES = 3,
+	/* invalid should always be the last entry. */
+	ALGO_INVALID
+};
+
 struct mem_file {
    void *start;
    int len;
Index: LinuxBIOSv3-algonaming/lib/lar.c
===================================================================
--- LinuxBIOSv3-algonaming/lib/lar.c	(Revision 605)
+++ LinuxBIOSv3-algonaming/lib/lar.c	(Arbeitskopie)
@@ -31,6 +31,15 @@
 #define ntohl(x) (x)
 #endif

+static const char *algo_name[] = {
+	"none",
+	"lzma",
+	"nrv2b",
+	"zeroes",
+	/* invalid should always be the last entry. */
+	"invalid"
+};
+
 /**
  * run_address is passed the address of a function taking no parameters and
  * jumps to it, returning the result. 
@@ -147,36 +156,41 @@

int process_file(const struct mem_file *archive, void *where)
 {
-	printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", archive->compression);
+	const char *algoname = algo_name[(archive->compression >= ALGO_INVALID)
+					? ALGO_INVALID : archive->compression];
+	printk(BIOS_SPEW, "LAR: Compression algorithm #%i (%s) used\n",
+		archive->compression, algoname);
+	switch (archive->compression) {
    /* no compression */
-	if (archive->compression == 0) {
+	case ALGO_NONE:
    	memcpy(where, archive->start, archive->len);
    	return 0;
-	}
 #ifdef CONFIG_COMPRESSION_LZMA
    /* lzma */
-	unsigned long ulzma(unsigned char *src, unsigned char *dst);
-	if (archive->compression == 1) {
+	case ALGO_LZMA: {
+		unsigned long ulzma(unsigned char *src, unsigned char *dst);
    	ulzma(archive->start, where);
    	return 0;
    }
 #endif
 #ifdef CONFIG_COMPRESSION_NRV2B
    /* nrv2b */
-	unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
-	if (archive->compression == 2) {
+	case ALGO_NRV2B: {
+		unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
    	unsigned long tmp;
    	unrv2b(archive->start, where, &tmp);
    	return 0;
    }
 #endif
    /* zeroes */
-	if (archive->compression == 3) {
+	case ALGO_ZEROES:
    	memset(where, 0, archive->reallen);
    	return 0;
+	default:
+		printk(BIOS_INFO, "LAR: Compression algorithm #%i (%s) not"
+			" supported!\n", archive->compression, algoname);
+		return -1;
    }
-	printk(BIOS_INFO, "LAR: Compression algorithm #%i not supported!\n", archive->compression);
-	return -1;
 }

/**
Index: LinuxBIOSv3-algonaming/util/lar/lar.c
===================================================================
--- LinuxBIOSv3-algonaming/util/lar/lar.c	(Revision 605)
+++ LinuxBIOSv3-algonaming/util/lar/lar.c	(Arbeitskopie)
@@ -40,7 +40,7 @@
 static int iselfparse = 0;
 static long larsize = 0;
 static char *bootblock = NULL;
-enum compalgo algo = none;
+enum compalgo algo = ALGO_NONE;

static void usage(char *name)
 {
@@ -272,10 +272,10 @@
    		break;
    	case 'C':
    		if (strcasecmp("lzma", optarg) == 0) {
-				algo = lzma;
+				algo = ALGO_LZMA;
    		}
    		if (strcasecmp("nrv2b", optarg) == 0) {
-				algo = nrv2b;
+				algo = ALGO_NRV2B;
    		}
    		break;
    	case 'l':
Index: LinuxBIOSv3-algonaming/util/lar/lar.h
===================================================================
--- LinuxBIOSv3-algonaming/util/lar/lar.h	(Revision 605)
+++ LinuxBIOSv3-algonaming/util/lar/lar.h	(Arbeitskopie)
@@ -92,7 +92,14 @@
    u32 size; /**< Size of the mmaped file */
 };

-enum compalgo { none = 0, lzma = 1, nrv2b = 2, zeroes = 3 };
+enum compalgo {
+	ALGO_NONE = 0,
+	ALGO_LZMA = 1,
+	ALGO_NRV2B = 2,
+	ALGO_ZEROES = 3,
+	/* invalid should always be the last entry. */
+	ALGO_INVALID
+};

typedef void (*compress_func) (char *, int, char *, int *);
 typedef void (*uncompress_func) (char *, int, char *, int);
@@ -124,8 +131,10 @@
 };

static const char *algo_name[] = {
-	"",
+	"none",
    "lzma",
    "nrv2b",
    "zeroes",
+	/* invalid should always be the last entry. */
+	"invalid"
 };
Index: LinuxBIOSv3-algonaming/util/lar/stream.c
===================================================================
--- LinuxBIOSv3-algonaming/util/lar/stream.c	(Revision 605)
+++ LinuxBIOSv3-algonaming/util/lar/stream.c	(Arbeitskopie)
@@ -147,7 +147,7 @@
    			fprintf(stderr, "Dropping empty section\n");
    		continue;
    	}
-		thisalgo = zeroes;
+		thisalgo = ALGO_ZEROES;
    	if (verbose())
    		fprintf(stderr,  "New section addr %#x size %#x\n",
    		(u32)shdr[i].sh_addr, (u32)shdr[i].sh_size);
@@ -565,7 +565,7 @@
    	if (file_in_list(files, filename)) {
    		printf("  %s ", filename);

-			if (ntohl(header->compression) == none) {
+			if (ntohl(header->compression) == ALGO_NONE) {
    			printf("(%d bytes @0x%lx);",
    			       ntohl(header->len),
    			       (unsigned long)(ptr - lar->map) +
@@ -669,7 +669,7 @@

if (file_in_list(files, filename)) {

-			if (ntohl(header->compression) == none) {
+			if (ntohl(header->compression) == ALGO_NONE) {
    			ret = _write_file(filename,
    					  (u8 *) (ptr + ntohl(header->offset)),
    					  (u32) ntohl(header->len));
@@ -730,7 +730,7 @@

if (!strncmp(name, "nocompress:",11)) {
    	filename += 11;
-		*thisalgo = none;
+		*thisalgo = ALGO_NONE;
    }

/* this is dangerous */
@@ -846,8 +846,8 @@
    int complen;
    compress_functions[*thisalgo](ptr, size, temp, &complen);

-	if (complen >= size && (*thisalgo != none)) {
-		*thisalgo = none;
+	if (complen >= size && (*thisalgo != ALGO_NONE)) {
+		*thisalgo = ALGO_NONE;
    	compress_functions[*thisalgo](ptr, size, temp, &complen);
    }
    return complen;

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 17.02.2008 13:02, Carl-Daniel Hailfinger wrote:
> > On 17.02.2008 05:02, Peter Stuge wrote:
> > > On Sun, Feb 17, 2008 at 12:34:24AM +0100, Carl-Daniel Hailfinger wrote:
> > > > +enum compalgo {
> > > > none = 0,
> > > > lzma = 1,
> > > > nrv2b = 2,
> > > > zeroes = 3,
> > > > /* invalid should always be the last entry. */
> > > > invalid
> > > >
> > > > +};
> > >
> > > It's fairly common to uppercase these. Do we want that too?
> >
> > This was just a straight copy from util/lar, but I agree. We use these 
> > enums like #defines, so uppercasing seems indeed the best way to bring 
> > this hint to the developer.
>
> Uppercasing also found a name clash between NONE as compression algo and 
> NONE as operation mode of util/lar.
>
> Print name of compression algorithm in addition to the corresponding 
> number during boot.
> Convert process_file() to use enum compalgo instead of hardcoded 
> "1","2","3" and change the control structure from a series of if() 
> statements to a switch() statement.
>
> Compile and boot tested on Qemu.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Stefan Reinauer

Stefan

[Carl-Daniel Hailfinger]

On 17.02.2008 14:26, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > Uppercasing also found a name clash between NONE as compression algo 
> > and NONE as operation mode of util/lar.
> >
> > Print name of compression algorithm in addition to the corresponding 
> > number during boot.
> > Convert process_file() to use enum compalgo instead of hardcoded 
> > "1","2","3" and change the control structure from a series of if() 
> > statements to a switch() statement.
> >
> > Compile and boot tested on Qemu.
> >
> > Signed-off-by: Carl-Daniel Hailfinger 
>
> Acked-by: Stefan Reinauer

Thanks, r606.

Regards,
Carl-Daniel