cbfstool: Rework to use getopt style parameters

- Adding more and more optional and non-optional parameters
  bloated cbfstool and made the code hard to read with a lot
  of parsing in the actual cbfs handling functions. This change
  switches over to use getopt style options for everything but
  command and cbfs file name.
- This allows us to simplify the coreboot Makefiles a bit
- Also, add guards to include files
- Fix some 80+ character lines
- Add more detailed error reporting
- Free memory we're allocating

Signed-off-by: Stefan Reinauer <reinauer@google.com>
Change-Id: Ia9137942deb8d26bbb30068e6de72466afe9b0a7
Reviewed-on: http://review.coreboot.org/1800
Tested-by: build bot (Jenkins)
Reviewed-by: Ronald G. Minnich <rminnich@gmail.com>

7 files changed, 505 insertions, 280 deletions

diff --git a/Makefile.inc b/Makefile.inc
index f85bbdac0a..2a5e821bbf 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -136,16 +136,14 @@ cbfs-files-handler= \
 
 #######################################################################
 # a variety of flags for our build
-CBFS_COMPRESS_FLAG:=
+CBFS_COMPRESS_FLAG:=none
 ifeq ($(CONFIG_COMPRESS_RAMSTAGE),y)
-CBFS_COMPRESS_FLAG:=l
+CBFS_COMPRESS_FLAG:=LZMA
 endif
 
-CBFS_PAYLOAD_COMPRESS_FLAG:=
-CBFS_PAYLOAD_COMPRESS_NAME:=none
+CBFS_PAYLOAD_COMPRESS_FLAG:=none
 ifeq ($(CONFIG_COMPRESSED_PAYLOAD_LZMA),y)
-CBFS_PAYLOAD_COMPRESS_FLAG:=l
-CBFS_PAYLOAD_COMPRESS_NAME:=LZMA
+CBFS_PAYLOAD_COMPRESS_FLAG:=LZMA
 endif
 
 ifneq ($(CONFIG_LOCALVERSION),"")
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index e41bc26b96..6e35dee753 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -59,13 +59,15 @@ prebuild-files = \
 	$(foreach file,$(cbfs-files), \
 	$(CBFSTOOL) $@.tmp \
 	add$(if $(filter stage,$(call extract_nth,3,$(file))),-stage)$(if $(filter payload,$(call extract_nth,3,$(file))),-payload) \
-	$(call extract_nth,1,$(file)) \
-	$(call extract_nth,2,$(file)) $(if $(filter-out stage payload,$(call extract_nth,3,$(file))),$(call extract_nth,3,$(file))) \
-	$(call extract_nth,4,$(file)) &&)
+	-f $(call extract_nth,1,$(file)) \
+	-n $(call extract_nth,2,$(file)) $(if $(filter-out stage,$(call extract_nth,3,$(file))),-t $(call extract_nth,3,$(file))) \
+	$(if $(call extract_nth,4,$(file)),-b $(call extract_nth,4,$(file))) &&)
 prebuilt-files = $(foreach file,$(cbfs-files), $(call extract_nth,1,$(file)))
 
 $(obj)/coreboot.pre1: $(objcbfs)/bootblock.bin $$(prebuilt-files) $(CBFSTOOL)
-	$(CBFSTOOL) $@.tmp create $(CONFIG_COREBOOT_ROMSIZE_KB)K $(objcbfs)/bootblock.bin 64 $$(( $(CONFIG_ROM_SIZE) - $(CONFIG_CBFS_SIZE) ))
+	$(CBFSTOOL) $@.tmp create -s $(CONFIG_COREBOOT_ROMSIZE_KB)K \
+		-B $(objcbfs)/bootblock.bin -a 64 \
+		-o $$(( $(CONFIG_ROM_SIZE) - $(CONFIG_CBFS_SIZE) ))
 	$(prebuild-files) true
 	mv $@.tmp $@
 else
@@ -79,30 +81,30 @@ $(obj)/coreboot.rom: $(obj)/coreboot.pre $(objcbfs)/coreboot_ram.elf $(CBFSTOOL)
 	cp $(obj)/coreboot.pre $@.tmp
 	if [ -f $(objcbfs)/coreboot_ap.elf ]; \
 	then \
-		$(CBFSTOOL) $@.tmp add-stage $(objcbfs)/coreboot_ap.elf $(CONFIG_CBFS_PREFIX)/coreboot_ap $(CBFS_COMPRESS_FLAG); \
+		$(CBFSTOOL) $@.tmp add-stage -f $(objcbfs)/coreboot_ap.elf -n $(CONFIG_CBFS_PREFIX)/coreboot_ap -c $(CBFS_COMPRESS_FLAG); \
 	fi
-	$(CBFSTOOL) $@.tmp add-stage $(objcbfs)/coreboot_ram.elf $(CONFIG_CBFS_PREFIX)/coreboot_ram $(CBFS_COMPRESS_FLAG)
+	$(CBFSTOOL) $@.tmp add-stage -f $(objcbfs)/coreboot_ram.elf -n $(CONFIG_CBFS_PREFIX)/coreboot_ram -c $(CBFS_COMPRESS_FLAG)
 ifeq ($(CONFIG_PAYLOAD_NONE),y)
 	@printf "    PAYLOAD    \e[1;31mnone (as specified by user)\e[0m\n"
 endif
 ifeq ($(CONFIG_PAYLOAD_ELF),y)
-	@printf "    PAYLOAD    $(CONFIG_PAYLOAD_FILE) (compression: $(CBFS_PAYLOAD_COMPRESS_NAME))\n"
-	$(CBFSTOOL) $@.tmp add-payload $(CONFIG_PAYLOAD_FILE) $(CONFIG_CBFS_PREFIX)/payload $(CBFS_PAYLOAD_COMPRESS_FLAG)
+	@printf "    PAYLOAD    $(CONFIG_PAYLOAD_FILE) (compression: $(CBFS_PAYLOAD_COMPRESS_FLAG))\n"
+	$(CBFSTOOL) $@.tmp add-payload -f $(CONFIG_PAYLOAD_FILE) -n $(CONFIG_CBFS_PREFIX)/payload -c $(CBFS_PAYLOAD_COMPRESS_FLAG)
 endif
 ifeq ($(CONFIG_PAYLOAD_SEABIOS),y)
-	@printf "    PAYLOAD    SeaBIOS (internal, compression: $(CBFS_PAYLOAD_COMPRESS_NAME))\n"
-	$(CBFSTOOL) $@.tmp add-payload $(CONFIG_PAYLOAD_FILE) $(CONFIG_CBFS_PREFIX)/payload $(CBFS_PAYLOAD_COMPRESS_FLAG)
+	@printf "    PAYLOAD    SeaBIOS (internal, compression: $(CBFS_PAYLOAD_COMPRESS_FLAG))\n"
+	$(CBFSTOOL) $@.tmp add-payload -f $(CONFIG_PAYLOAD_FILE) -n $(CONFIG_CBFS_PREFIX)/payload -c $(CBFS_PAYLOAD_COMPRESS_FLAG)
 endif
 ifeq ($(CONFIG_PAYLOAD_FILO),y)
-	@printf "    PAYLOAD    FILO (internal, compression: $(CBFS_PAYLOAD_COMPRESS_NAME))\n"
-	$(CBFSTOOL) $@.tmp add-payload $(CONFIG_PAYLOAD_FILE) $(CONFIG_CBFS_PREFIX)/payload $(CBFS_PAYLOAD_COMPRESS_FLAG)
+	@printf "    PAYLOAD    FILO (internal, compression: $(CBFS_PAYLOAD_COMPRESS_FLAG))\n"
+	$(CBFSTOOL) $@.tmp add-payload -f $(CONFIG_PAYLOAD_FILE) -n $(CONFIG_CBFS_PREFIX)/payload -c $(CBFS_PAYLOAD_COMPRESS_FLAG)
 endif
 ifeq ($(CONFIG_INCLUDE_CONFIG_FILE),y)
 	@printf "    CONFIG     $(DOTCONFIG)\n"
 	if [ -f $(DOTCONFIG) ]; then \
 	echo "# This image was built using git revision" `git rev-parse HEAD` > $(obj)/config.tmp ; \
 	sed -e '/^#/d' -e '/^ *$$/d' $(DOTCONFIG) >> $(obj)/config.tmp ; \
-	$(CBFSTOOL) $@.tmp add $(obj)/config.tmp config raw; rm -f $(obj)/config.tmp ; fi
+	$(CBFSTOOL) $@.tmp add -f $(obj)/config.tmp -n config -t raw; rm -f $(obj)/config.tmp ; fi
 endif
 	mv $@.tmp $@
 	@printf "    CBFSPRINT  $(subst $(obj)/,,$(@))\n\n"
@@ -270,8 +272,9 @@ endif
 $(obj)/coreboot.pre: $(objcbfs)/romstage_xip.elf $(obj)/coreboot.pre1 $(CBFSTOOL)
 	@printf "    CBFS       $(subst $(obj)/,,$(@))\n"
 	cp $(obj)/coreboot.pre1 $@.tmp
-	$(CBFSTOOL) $@.tmp add-stage $(objcbfs)/romstage_xip.elf \
-		$(CONFIG_CBFS_PREFIX)/romstage x $(shell cat $(objcbfs)/base_xip.txt)
+	$(CBFSTOOL) $@.tmp add-stage -f $(objcbfs)/romstage_xip.elf \
+		-n $(CONFIG_CBFS_PREFIX)/romstage -c none \
+		-b $(shell cat $(objcbfs)/base_xip.txt)
 	mv $@.tmp $@
 
 ################################################################################
@@ -370,7 +373,7 @@ $(objgenerated)/romstage_xip.ld: $(objgenerated)/romstage_null.ld $(objcbfs)/bas
 
 $(objcbfs)/base_xip.txt: $(obj)/coreboot.pre1 $(objcbfs)/romstage_null.bin
 	rm -f $@
-	$(CBFSTOOL) $(obj)/coreboot.pre1 locate $(objcbfs)/romstage_null.bin $(CONFIG_CBFS_PREFIX)/romstage $(CONFIG_XIP_ROM_SIZE) > $@.tmp \
+	$(CBFSTOOL) $(obj)/coreboot.pre1 locate -f $(objcbfs)/romstage_null.bin -n $(CONFIG_CBFS_PREFIX)/romstage -a $(CONFIG_XIP_ROM_SIZE) > $@.tmp \
 	 || { echo "The romstage is larger than XIP size. Please expand the CONFIG_XIP_ROM_SIZE" ; exit 1; }
 	sed -e 's/^/0x/g' $@.tmp > $@.tmp2
 	rm $@.tmp
diff --git a/util/cbfstool/cbfs-mkstage.c b/util/cbfstool/cbfs-mkstage.c
index 6b80a37dc5..55c81c6ebe 100644
--- a/util/cbfstool/cbfs-mkstage.c
+++ b/util/cbfstool/cbfs-mkstage.c
@@ -31,7 +31,7 @@
 #include "common.h"
 #include "cbfs.h"
 
-unsigned int idemp(unsigned int x)
+static unsigned int idemp(unsigned int x)
 {
 	return x;
 }
@@ -123,7 +123,8 @@ int parse_elf_to_stage(unsigned char *input, unsigned char **output,
 	}
 
 	if (data_end <= data_start) {
-		fprintf(stderr, "E: data ends before it starts. Make sure the ELF file is correct and resides in ROM space.\n");
+		fprintf(stderr, "E: data ends before it starts. Make sure the "
+			"ELF file is correct and resides in ROM space.\n");
 		exit(1);
 	}
 
@@ -175,6 +176,8 @@ int parse_elf_to_stage(unsigned char *input, unsigned char **output,
 	compress(buffer, data_end - data_start,
 		 (char *)(out + sizeof(struct cbfs_stage)), (int *)&stage->len);
 
+	free(buffer);
+
 	*output = out;
 
 	if (*location)
diff --git a/util/cbfstool/cbfs.h b/util/cbfstool/cbfs.h
index cdd003062f..f161ed445a 100644
--- a/util/cbfstool/cbfs.h
+++ b/util/cbfstool/cbfs.h
@@ -16,6 +16,9 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
  */
 
+#ifndef __CBFS_H
+#define __CBFS_H
+
 #include <stdint.h>
 
 struct cbfs_header {
@@ -93,3 +96,5 @@ struct cbfs_payload {
 
 int cbfs_file_header(uint32_t physaddr);
 struct cbfs_file *cbfs_create_empty_file(uint32_t physaddr, uint32_t size);
+
+#endif
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 12c972ef10..66db379d02 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -22,401 +22,491 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
+#include <unistd.h>
+#include <getopt.h>
 #include "common.h"
 #include "cbfs.h"
 
 struct command {
 	const char *name;
-	int (*function) (int argc, char **argv);
+	const char *optstring;
+	int (*function) (void);
 };
 
-static int cbfs_add(int argc, char **argv)
+int verbose = 0;
+static char *cbfs_name = NULL;
+static char *rom_name = NULL;
+static char *rom_filename = NULL;
+static char *rom_bootblock = NULL;
+static uint32_t rom_type = 0;
+static uint32_t rom_baseaddress = 0;
+static uint32_t rom_loadaddress = 0;
+static uint32_t rom_entrypoint = 0;
+static uint32_t rom_size = 0;
+static uint32_t rom_alignment = 0;
+static uint32_t rom_offset = 0;
+static comp_algo rom_algo = CBFS_COMPRESS_NONE;
+
+static int cbfs_add(void)
 {
-	char *romname = argv[1];
-	char *cmd = argv[2];
-	void *rom = loadrom(romname);
+	uint32_t filesize = 0;
+	void *rom, *filedata, *cbfsfile;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
 		return 1;
 	}
 
-	if (argc < 5) {
-		printf("not enough arguments to '%s'.\n", cmd);
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	char *filename = argv[3];
-	char *cbfsname = argv[4];
+	if (rom_type == 0) {
+		fprintf(stderr, "E: You need to specify a valid -t/--type.\n");
+		return 1;
+	}
 
-	uint32_t filesize = 0;
-	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
+		return 1;
+	}
+
+	filedata = loadfile(rom_filename, &filesize, 0, SEEK_SET);
 	if (filedata == NULL) {
-		printf("Could not load file '%s'.\n", filename);
+		fprintf(stderr, "E: Could not load file '%s'.\n",
+			rom_filename);
+		free(rom);
 		return 1;
 	}
 
-	uint32_t base = 0;
-	void *cbfsfile = NULL;
+	cbfsfile = create_cbfs_file(rom_name, filedata, &filesize,
+					rom_type, &rom_baseaddress);
+	free(filedata);
 
-	if (argc < 6) {
-		printf("not enough arguments to 'add'.\n");
+	if (add_file_to_cbfs(cbfsfile, filesize, rom_baseaddress)) {
+		fprintf(stderr, "E: Adding file '%s' failed.\n", rom_filename);
+		free(cbfsfile);
+		free(rom);
 		return 1;
 	}
-	uint32_t type;
-	if (intfiletype(argv[5]) != ((uint64_t) - 1))
-		type = intfiletype(argv[5]);
-	else
-		type = strtoul(argv[5], NULL, 0);
-	if (argc > 6) {
-		base = strtoul(argv[6], NULL, 0);
-	}
-	cbfsfile = create_cbfs_file(cbfsname, filedata, &filesize, type, &base);
-	if (add_file_to_cbfs(cbfsfile, filesize, base)) {
-		printf("Adding file '%s' failed.\n", filename);
+	if (writerom(cbfs_name, rom, romsize)) {
+		free(cbfsfile);
+		free(rom);
 		return 1;
 	}
-	if (writerom(romname, rom, romsize))
-		return 1;
+
+	free(cbfsfile);
+	free(rom);
 	return 0;
 }
 
-static int cbfs_add_payload(int argc, char **argv)
+static int cbfs_add_payload(void)
 {
-	char *romname = argv[1];
-	char *cmd = argv[2];
-	void *rom = loadrom(romname);
+	uint32_t filesize = 0;
+	void *rom, *filedata, *cbfsfile;
+	unsigned char *payload;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
 		return 1;
 	}
 
-	if (argc < 5) {
-		printf("not enough arguments to '%s'.\n", cmd);
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	char *filename = argv[3];
-	char *cbfsname = argv[4];
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
+		return 1;
+	}
 
-	uint32_t filesize = 0;
-	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	filedata = loadfile(rom_filename, &filesize, 0, SEEK_SET);
 	if (filedata == NULL) {
-		printf("Could not load file '%s'.\n", filename);
+		fprintf(stderr, "E: Could not load file '%s'.\n",
+			rom_filename);
+		free(rom);
 		return 1;
 	}
 
-	uint32_t base = 0;
-	void *cbfsfile = NULL;
+	filesize = parse_elf_to_payload(filedata, &payload, rom_algo);
 
-	comp_algo algo = CBFS_COMPRESS_NONE;
-	if (argc > 5) {
-		if (argv[5][0] == 'l')
-			algo = CBFS_COMPRESS_LZMA;
-	}
-	if (argc > 6) {
-		base = strtoul(argv[6], NULL, 0);
-	}
-	unsigned char *payload;
-	filesize = parse_elf_to_payload(filedata, &payload, algo);
-	cbfsfile =
-	    create_cbfs_file(cbfsname, payload, &filesize,
-			     CBFS_COMPONENT_PAYLOAD, &base);
-	if (add_file_to_cbfs(cbfsfile, filesize, base)) {
-		printf("Adding payload '%s' failed.\n", filename);
+	cbfsfile = create_cbfs_file(rom_name, payload, &filesize,
+				CBFS_COMPONENT_PAYLOAD, &rom_baseaddress);
+
+	free(filedata);
+	free(payload);
+
+	if (add_file_to_cbfs(cbfsfile, filesize, rom_baseaddress)) {
+		fprintf(stderr, "E: Adding payload '%s' failed.\n",
+			rom_filename);
+		free(cbfsfile);
+		free(rom);
 		return 1;
 	}
-	if (writerom(romname, rom, romsize))
+
+	if (writerom(cbfs_name, rom, romsize)) {
+		free(cbfsfile);
+		free(rom);
 		return 1;
+	}
+
+	free(cbfsfile);
+	free(rom);
 	return 0;
 }
 
-static int cbfs_add_stage(int argc, char **argv)
+static int cbfs_add_stage(void)
 {
-	char *romname = argv[1];
-	char *cmd = argv[2];
-	void *rom = loadrom(romname);
+	uint32_t filesize = 0;
+	void *rom, *filedata, *cbfsfile;
+	unsigned char *stage;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
 		return 1;
 	}
 
-	if (argc < 5) {
-		printf("not enough arguments to '%s'.\n", cmd);
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	char *filename = argv[3];
-	char *cbfsname = argv[4];
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
+		return 1;
+	}
 
-	uint32_t filesize = 0;
-	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	filedata = loadfile(rom_filename, &filesize, 0, SEEK_SET);
 	if (filedata == NULL) {
-		printf("Could not load file '%s'.\n", filename);
+		fprintf(stderr, "E: Could not load file '%s'.\n",
+			rom_filename);
+		free(rom);
 		return 1;
 	}
 
-	uint32_t base = 0;
-	void *cbfsfile = NULL;
+	filesize = parse_elf_to_stage(filedata, &stage, rom_algo, &rom_baseaddress);
 
-	comp_algo algo = CBFS_COMPRESS_NONE;
-	if (argc > 5) {
-		if (argv[5][0] == 'l')
-			algo = CBFS_COMPRESS_LZMA;
-	}
-	if (argc > 6) {
-		base = strtoul(argv[6], NULL, 0);
-	}
-	unsigned char *stage;
-	filesize = parse_elf_to_stage(filedata, &stage, algo, &base);
-	cbfsfile =
-	    create_cbfs_file(cbfsname, stage, &filesize,
-			     CBFS_COMPONENT_STAGE, &base);
+	cbfsfile = create_cbfs_file(rom_name, stage, &filesize,
+				CBFS_COMPONENT_STAGE, &rom_baseaddress);
 
-	if (add_file_to_cbfs(cbfsfile, filesize, base)) {
-		printf("Adding stage '%s' failed.\n", filename);
+	free(filedata);
+	free(stage);
+
+	if (add_file_to_cbfs(cbfsfile, filesize, rom_baseaddress)) {
+		fprintf(stderr, "E: Adding stage '%s' failed.\n",
+			rom_filename);
+		free(cbfsfile);
+		free(rom);
 		return 1;
 	}
-	if (writerom(romname, rom, romsize))
+
+	if (writerom(cbfs_name, rom, romsize)) {
+		free(cbfsfile);
+		free(rom);
 		return 1;
+	}
+
+	free(cbfsfile);
+	free(rom);
 	return 0;
 }
 
-static int cbfs_add_flat_binary(int argc, char **argv)
+static int cbfs_add_flat_binary(void)
 {
-	char *romname = argv[1];
-	char *cmd = argv[2];
-	void *rom = loadrom(romname);
+	uint32_t filesize = 0;
+	uint32_t final_size;
+	void *rom, *filedata, *cbfsfile;
+	unsigned char *payload;
+	comp_func_ptr compress;
+	struct cbfs_payload_segment *segs;
+	int doffset, len = 0;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
 		return 1;
 	}
 
-	if (argc < 7) {
-		printf("not enough arguments to '%s'.\n", cmd);
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	char *filename = argv[3];
-	char *cbfsname = argv[4];
-	unsigned long load_address = strtoul(argv[5], NULL, 0);
-	unsigned long entry_point = strtoul(argv[6], NULL, 0);
-
-	uint32_t base = 0;
-	void *cbfsfile = NULL;
-
-	comp_algo algo = CBFS_COMPRESS_NONE;
-	if (argc > 7) {
-		if (argv[7][0] == 'l')
-			algo = CBFS_COMPRESS_LZMA;
+	if (rom_loadaddress == 0) {
+		fprintf(stderr, "E: You need to specify a valid "
+			"-l/--load-address.\n");
+		return 1;
 	}
-	if (argc > 8) {
-		base = strtoul(argv[8], NULL, 0);
+
+	if (rom_entrypoint == 0) {
+		fprintf(stderr, "You need to specify a valid "
+			"-e/--entry-point.\n");
+		return 1;
 	}
-	comp_func_ptr compress = compression_function(algo);
+
+	compress = compression_function(rom_algo);
 	if (!compress)
 		return 1;
 
-	uint32_t filesize = 0;
-	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "Could not load ROM image '%s'.\n",
+			cbfs_name);
+		return 1;
+	}
+
+	filedata = loadfile(rom_filename, &filesize, 0, SEEK_SET);
 	if (filedata == NULL) {
-		printf("Could not load file '%s'.\n", filename);
+		fprintf(stderr, "E: Could not load file '%s'.\n",
+			rom_filename);
+		free(rom);
 		return 1;
 	}
 
-	unsigned char *payload;
+	/* FIXME compressed file size might be bigger than original file */
 	payload = calloc((2 * sizeof(struct cbfs_payload_segment)) + filesize, 1);
 	if (payload == NULL) {
-		printf("Could not allocate memory.\n");
+		fprintf(stderr, "E: Could not allocate memory.\n");
+		free(filedata);
+		free(rom);
 		return 1;
 	}
 
-	struct cbfs_payload_segment *segs;
 	segs = (struct cbfs_payload_segment *)payload;
-	int doffset = (2 * sizeof(struct cbfs_payload_segment));
+	doffset = (2 * sizeof(struct cbfs_payload_segment));
 
+	/* Prepare code segment */
 	segs[0].type = PAYLOAD_SEGMENT_CODE;
-	segs[0].load_addr = (uint64_t)htonll(load_address);
+	segs[0].load_addr = (uint64_t)htonll(rom_loadaddress);
 	segs[0].mem_len = (uint32_t)htonl(filesize);
-	segs[0].offset = htonl(doffset);
-
-	int len = 0;
+	segs[0].offset = (uint32_t)htonl(doffset);
 
 	compress(filedata, filesize, (char *)(payload + doffset), &len);
-	segs[0].compression = htonl(algo);
+	segs[0].compression = htonl(rom_algo);
 	segs[0].len = htonl(len);
 
-	if ((unsigned int)len > filesize) {
+	if ((unsigned int)len >= filesize) {
 		segs[0].compression = 0;
 		segs[0].len = htonl(filesize);
 		memcpy((char *)(payload + doffset), filedata, filesize);
 	}
-	uint32_t final_size = doffset + ntohl(segs[0].len);
 
+	/* prepare entry point segment */
 	segs[1].type = PAYLOAD_SEGMENT_ENTRY;
-	segs[1].load_addr = (uint64_t)htonll(entry_point);
+	segs[1].load_addr = (uint64_t)htonll(rom_entrypoint);
 
+	final_size = doffset + ntohl(segs[0].len);
 	cbfsfile =
-	    create_cbfs_file(cbfsname, payload, &final_size,
-			     CBFS_COMPONENT_PAYLOAD, &base);
-	if (add_file_to_cbfs(cbfsfile, final_size, base)) {
-		printf("Adding payload '%s' failed.\n", filename);
+	    create_cbfs_file(rom_name, payload, &final_size,
+			     CBFS_COMPONENT_PAYLOAD, &rom_baseaddress);
+
+	free(filedata);
+	free(payload);
+
+	if (add_file_to_cbfs(cbfsfile, final_size, rom_baseaddress)) {
+		fprintf(stderr, "E: Adding payload '%s' failed.\n",
+			rom_filename);
+		free(cbfsfile);
+		free(rom);
 		return 1;
 	}
-	if (writerom(romname, rom, romsize))
+	if (writerom(cbfs_name, rom, romsize)) {
+		free(cbfsfile);
+		free(rom);
 		return 1;
+	}
+
+	free(cbfsfile);
+	free(rom);
 	return 0;
 }
 
-static int cbfs_remove(int argc, char **argv)
+static int cbfs_remove(void)
 {
-	char *romname = argv[1];
-	char *cmd = argv[2];
-	void *rom = loadrom(romname);
+	void *rom;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	if (argc < 4) {
-		printf("not enough arguments to '%s'.\n", cmd);
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
 		return 1;
 	}
 
-	char *cbfsname = argv[3];
-
-	if (remove_file_from_cbfs(cbfsname)) {
-		printf("Removing file '%s' failed.\n", cbfsname);
+	if (remove_file_from_cbfs(rom_name)) {
+		fprintf(stderr, "E: Removing file '%s' failed.\n",
+			rom_name);
+		free(rom);
 		return 1;
 	}
-	if (writerom(romname, rom, romsize))
+
+	if (writerom(cbfs_name, rom, romsize)) {
+		free(rom);
 		return 1;
+	}
+
+	free(rom);
 	return 0;
 }
 
-static int cbfs_create(int argc, char **argv)
+static int cbfs_create(void)
 {
-	char *romname = argv[1];
-	if (argc < 5) {
-		printf("not enough arguments to 'create'.\n");
+	if (rom_size == 0) {
+		printf("You need to specify a valid -s/--size.\n");
 		return 1;
 	}
 
-	char* suffix;
-	uint32_t size = strtoul(argv[3], &suffix, 0);
-	if (tolower(suffix[0])=='k') {
-		size *= 1024;
-	}
-	if (tolower(suffix[0])=='m') {
-		size *= 1024 * 1024;
+	if (!rom_bootblock) {
+		printf("You need to specify -b/--bootblock.\n");
+		return 1;
 	}
-	char *bootblock = argv[4];
-	uint32_t align = 0;
 
-	if (argc > 5)
-		align = strtoul(argv[5], NULL, 0);
-
-	uint32_t offs = 0;
-	if (argc > 6)
-		offs = strtoul(argv[6], NULL, 0);
-
-	return create_cbfs_image(romname, size, bootblock, align, offs);
+	return create_cbfs_image(cbfs_name, rom_size, rom_bootblock,
+						rom_alignment, rom_offset);
 }
 
-static int cbfs_locate(int argc, char **argv)
+static int cbfs_locate(void)
 {
-	char *romname = argv[1];
-	if (argc < 6) {
-		printf("not enough arguments to 'locate'.\n");
+	uint32_t filesize, location;
+
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
+		return 1;
+	}
+
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	const char *file = argv[3];
-	uint32_t filesize = getfilesize(file);
-	const char *filename = argv[4];
-	int align = strtoul(argv[5], NULL, 0);
-	uint32_t location = cbfs_find_location(romname, filesize, filename, align);
+	filesize = getfilesize(rom_filename);
+
+	location = cbfs_find_location(cbfs_name, filesize,
+					rom_name, rom_alignment);
 
 	printf("%x\n", location);
 	return location == 0 ? 1 : 0;
 }
 
-static int cbfs_print(int argc, char **argv)
+static int cbfs_print(void)
 {
-	char *romname = argv[1];
-	void *rom = loadrom(romname);
+	void *rom;
 
+	rom = loadrom(cbfs_name);
 	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
 		return 1;
 	}
 
-	print_cbfs_directory(romname);
+	print_cbfs_directory(cbfs_name);
+
+	free(rom);
 	return 0;
 }
 
-static int cbfs_extract(int argc, char **argv)
+static int cbfs_extract(void)
 {
-	char *romname = argv[1];
-	void *rom = loadrom(romname);
+	void *rom;
+	int ret;
 
-	if (rom == NULL) {
-		printf("Could not load ROM image '%s'.\n", romname);
+	if (!rom_filename) {
+		fprintf(stderr, "E: You need to specify -f/--filename.\n");
+		return 1;
+	}
+
+	if (!rom_name) {
+		fprintf(stderr, "E: You need to specify -n/--name.\n");
 		return 1;
 	}
 
-	if (argc != 5)
-	{
-		printf("Error: you must specify a CBFS name and a file to dump it in.\n");
+	rom = loadrom(cbfs_name);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			cbfs_name);
 		return 1;
 	}
 
-	return extract_file_from_cbfs(romname, argv[3], argv[4]);
+	ret = extract_file_from_cbfs(cbfs_name, rom_name, rom_filename);
+
+	free(rom);
+	return ret;
 }
 
 static const struct command commands[] = {
-	{"add", cbfs_add},
-	{"add-payload", cbfs_add_payload},
-	{"add-stage", cbfs_add_stage},
-	{"add-flat-binary", cbfs_add_flat_binary},
-	{"remove", cbfs_remove},
-	{"create", cbfs_create},
-	{"locate", cbfs_locate},
-	{"print", cbfs_print},
-	{"extract", cbfs_extract},
+	{"add", "f:n:t:b:h?", cbfs_add},
+	{"add-payload", "f:n:t:c:b:h?", cbfs_add_payload},
+	{"add-stage", "f:n:t:c:b:h?", cbfs_add_stage},
+	{"add-flat-binary", "f:n:l:e:c:b:h?", cbfs_add_flat_binary},
+	{"remove", "n:h?", cbfs_remove},
+	{"create", "s:B:a:o:h?", cbfs_create},
+	{"locate", "f:n:a:h?", cbfs_locate},
+	{"print", "h?", cbfs_print},
+	{"extract", "n:f:h?", cbfs_extract},
+};
+
+static struct option long_options[] = {
+	{"name",         required_argument, 0, 'n' },
+	{"type",         required_argument, 0, 't' },
+	{"compression",  required_argument, 0, 'c' },
+	{"base-address", required_argument, 0, 'b' },
+	{"load-address", required_argument, 0, 'l' },
+	{"entry-point",  required_argument, 0, 'e' },
+	{"size",         required_argument, 0, 's' },
+	{"bootblock",    required_argument, 0, 'B' },
+	{"alignment",    required_argument, 0, 'a' },
+	{"offset",       required_argument, 0, 'o' },
+	{"file",         required_argument, 0, 'f' },
+	{"verbose",      no_argument,       0, 'v' },
+	{"help",         no_argument,       0, 'h' },
+	{NULL,           0,                 0,  0  }
 };
 
-static void usage(void)
+static void usage(char *name)
 {
 	printf
 	    ("cbfstool: Management utility for CBFS formatted ROM images\n\n"
-	     "USAGE:\n" " cbfstool [-h]\n"
-	     " cbfstool FILE COMMAND [PARAMETERS]...\n\n" "OPTIONs:\n"
+	     "USAGE:\n" " %s [-h]\n"
+	     " %s FILE COMMAND [PARAMETERS]...\n\n" "OPTIONs:\n"
 	     "  -h		Display this help message\n\n"
 	     "COMMANDs:\n"
-	     " add FILE NAME TYPE [base address]      Add a component\n"
-	     " add-payload FILE NAME [COMP] [base]    Add a payload to the ROM\n"
-	     " add-stage FILE NAME [COMP] [base]      Add a stage to the ROM\n"
-	     " add-flat-binary FILE NAME LOAD ENTRY \\\n"
-	     "                       [COMP] [base]    Add a 32bit flat mode binary\n"
-	     " remove FILE NAME                       Remove a component\n"
-	     " create SIZE BOOTBLOCK [ALIGN] [offset] Create a ROM file\n"
-	     " locate FILE NAME ALIGN                 Find a place for a file of that size\n"
-	     " print                                  Show the contents of the ROM\n"
-	     " extract NAME FILE                      Extracts a raw payload from ROM\n"
+	     " add -f FILE -n NAME -t TYPE [-b base-address]             "
+			"Add a component\n"
+	     " add-payload -f FILE -n NAME [-c compression] [-b base]    "
+			"Add a payload to the ROM\n"
+	     " add-stage -f FILE -n NAME [-c compression] [-b base]      "
+			"Add a stage to the ROM\n"
+	     " add-flat-binary -f FILE -n NAME -l load-address \\\n"
+	     "        -e entry-point [-c compression] [-b base]          "
+			"Add a 32bit flat mode binary\n"
+	     " remove -n NAME                                            "
+			"Remove a component\n"
+	     " create -s size -B bootblock [-a align] [-o offset]        "
+			"Create a ROM file\n"
+	     " locate -f FILE -n NAME -a align                           "
+			"Find a place for a file of that size\n"
+	     " print                                                     "
+			"Show the contents of the ROM\n"
+	     " extract -n NAME -f FILE                                   "
+			"Extracts a raw payload from ROM\n"
 	     "\n"
-	     "TYPEs:\n"
-	     );
+	     "TYPEs:\n", name, name
+	    );
 	print_supported_filetypes();
 }
 
-/* Small, OS/libc independent runtime check
- * for endianess
- */
+/* Small, OS/libc independent runtime check for endianess */
 int host_bigendian = 0;
 
 static void which_endian(void)
@@ -431,23 +521,108 @@ static void which_endian(void)
 int main(int argc, char **argv)
 {
 	size_t i;
+	int c;
 
 	if (argc < 3) {
-		usage();
+		usage(argv[0]);
 		return 1;
 	}
 
 	which_endian();
 
+	cbfs_name = argv[1];
 	char *cmd = argv[2];
+	optind += 2;
 
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
 		if (strcmp(cmd, commands[i].name) != 0)
 			continue;
-		return commands[i].function(argc, argv);
+
+		while (1) {
+			char *suffix = NULL;
+			int option_index = 0;
+
+			c = getopt_long(argc, argv, commands[i].optstring,
+						long_options, &option_index);
+			if (c == -1)
+				break;
+
+			/* filter out illegal long options */
+			if (index(commands[i].optstring, c) == NULL) {
+				/* TODO maybe print actual long option instead */
+				printf("%s: invalid option -- '%c'\n",
+					argv[0], c);
+				c = '?';
+			}
+
+			switch(c) {
+			case 'n':
+				rom_name = optarg;
+				break;
+			case 't':
+				if (intfiletype(optarg) != ((uint64_t) - 1))
+					rom_type = intfiletype(optarg);
+				else
+					rom_type = strtoul(optarg, NULL, 0);
+				if (rom_type == 0)
+					printf("W: Unknown type '%s' ignored\n",
+							optarg);
+				break;
+			case 'c':
+				if (!strncasecmp(optarg, "lzma", 5))
+					rom_algo = CBFS_COMPRESS_LZMA;
+				else if (!strncasecmp(optarg, "none", 5))
+					rom_algo = CBFS_COMPRESS_NONE;
+				else
+					printf("W: Unknown compression '%s'"
+							" ignored.\n", optarg);
+				break;
+			case 'b':
+				rom_baseaddress = strtoul(optarg, NULL, 0);
+				break;
+			case 'l':
+				rom_loadaddress = strtoul(optarg, NULL, 0);
+
+				break;
+			case 'e':
+				rom_entrypoint = strtoul(optarg, NULL, 0);
+				break;
+			case 's':
+				rom_size = strtoul(optarg, &suffix, 0);
+				if (tolower(suffix[0])=='k') {
+					rom_size *= 1024;
+				}
+				if (tolower(suffix[0])=='m') {
+					rom_size *= 1024 * 1024;
+				}
+			case 'B':
+				rom_bootblock = optarg;
+				break;
+			case 'a':
+				rom_alignment = strtoul(optarg, NULL, 0);
+				break;
+			case 'o':
+				rom_offset = strtoul(optarg, NULL, 0);
+				break;
+			case 'f':
+				rom_filename = optarg;
+				break;
+			case 'v':
+				verbose++;
+				break;
+			case 'h':
+			case '?':
+				usage(argv[0]);
+				return 1;
+			default:
+				break;
+			}
+		}
+
+		return commands[i].function();
 	}
 
 	printf("Unknown command '%s'.\n", cmd);
-	usage();
+	usage(argv[0]);
 	return 1;
 }
diff --git a/util/cbfstool/common.c b/util/cbfstool/common.c
index 23f3ecfa87..6c67c396f4 100644
--- a/util/cbfstool/common.c
+++ b/util/cbfstool/common.c
@@ -28,10 +28,13 @@
 
 #define dprintf(x...)
 
-uint32_t getfilesize(const char *filename)
+size_t getfilesize(const char *filename)
 {
-	uint32_t size;
+	size_t size;
 	FILE *file = fopen(filename, "rb");
+	if (file == NULL)
+		return -1;
+
 	fseek(file, 0, SEEK_END);
 	size = ftell(file);
 	fclose(file);
@@ -44,6 +47,7 @@ void *loadfile(const char *filename, uint32_t * romsize_p, void *content,
 	FILE *file = fopen(filename, "rb");
 	if (file == NULL)
 		return NULL;
+
 	fseek(file, 0, SEEK_END);
 	*romsize_p = ftell(file);
 	fseek(file, 0, SEEK_SET);
@@ -58,15 +62,16 @@ void *loadfile(const char *filename, uint32_t * romsize_p, void *content,
 		content -= *romsize_p;
 
 	if (!fread(content, *romsize_p, 1, file)) {
-		printf("failed to read %s\n", filename);
+		printf("Failed to read %s\n", filename);
 		return NULL;
 	}
 	fclose(file);
 	return content;
 }
 
-struct cbfs_header *master_header;
-uint32_t phys_start, phys_end, align, romsize;
+static struct cbfs_header *master_header;
+static uint32_t phys_start, phys_end, align;
+uint32_t romsize;
 void *offset;
 
 void recalculate_rom_geometry(void *romarea)
@@ -448,11 +453,15 @@ void *create_cbfs_file(const char *filename, void *data, uint32_t * datasize,
 int create_cbfs_image(const char *romfile, uint32_t _romsize,
 		      const char *bootblock, uint32_t align, uint32_t offs)
 {
+	uint32_t bootblocksize = 0;
+	struct cbfs_header *master_header;
+	unsigned char *romarea, *bootblk;
+
 	romsize = _romsize;
-	unsigned char *romarea = malloc(romsize);
+	romarea = malloc(romsize);
 	if (!romarea) {
-		printf("Could not get %d bytes of memory for CBFS image.\n",
-		       romsize);
+		fprintf(stderr, "E: Could not get %d bytes of memory"
+			" for CBFS image.\n", romsize);
 		exit(1);
 	}
 	memset(romarea, 0xff, romsize);
@@ -463,9 +472,16 @@ int create_cbfs_image(const char *romfile, uint32_t _romsize,
 	if (align == 0)
 		align = 64;
 
-	uint32_t bootblocksize = 0;
-	loadfile(bootblock, &bootblocksize, romarea + romsize, SEEK_END);
-	struct cbfs_header *master_header =
+	bootblk = loadfile(bootblock, &bootblocksize,
+				romarea + romsize, SEEK_END);
+	if (!bootblk) {
+		fprintf(stderr, "E: Could not load bootblock %s.\n",
+			bootblock);
+		free(romarea);
+		return 1;
+	}
+
+	master_header =
 	    (struct cbfs_header *)(romarea + romsize - bootblocksize -
 				   sizeof(struct cbfs_header));
 	master_header->magic = ntohl(0x4f524243);
@@ -485,6 +501,7 @@ int create_cbfs_image(const char *romfile, uint32_t _romsize,
 				   sizeof(struct cbfs_file) - 16);
 
 	writerom(romfile, romarea, romsize);
+	free(romarea);
 	return 0;
 }
 
@@ -496,49 +513,69 @@ static int in_segment(int addr, int size, int gran)
 uint32_t cbfs_find_location(const char *romfile, uint32_t filesize,
 			    const char *filename, uint32_t alignment)
 {
-	loadrom(romfile);
-	size_t filename_size = strlen(filename);
+	void *rom;
+	size_t filename_size, headersize, totalsize;
+	int ret = 0;
+	uint32_t current;
+
+	rom = loadrom(romfile);
+	if (rom == NULL) {
+		fprintf(stderr, "E: Could not load ROM image '%s'.\n",
+			romfile);
+		return 0;
+	}
 
-	size_t headersize =
-	    sizeof(struct cbfs_file) + ALIGN(filename_size + 1,
-					     16) + sizeof(struct cbfs_stage);
-	size_t totalsize = headersize + filesize;
+	filename_size = strlen(filename);
+	headersize = sizeof(struct cbfs_file) + ALIGN(filename_size + 1, 16) +
+			sizeof(struct cbfs_stage);
+	totalsize = headersize + filesize;
 
-	uint32_t current = phys_start;
+	current = phys_start;
 	while (current < phys_end) {
+		uint32_t top;
+		struct cbfs_file *thisfile;
+
 		if (!cbfs_file_header(current)) {
 			current += align;
 			continue;
 		}
-		struct cbfs_file *thisfile =
-		    (struct cbfs_file *)phys_to_virt(current);
 
-		uint32_t top =
-		    current + ntohl(thisfile->len) + ntohl(thisfile->offset);
+		thisfile = (struct cbfs_file *)phys_to_virt(current);
+
+		top = current + ntohl(thisfile->len) + ntohl(thisfile->offset);
+
 		if (((ntohl(thisfile->type) == 0x0)
 		     || (ntohl(thisfile->type) == 0xffffffff))
 		    && (ntohl(thisfile->len) + ntohl(thisfile->offset) >=
 			totalsize)) {
 			if (in_segment
-			    (current + headersize, filesize, alignment))
-				return current + headersize;
+			    (current + headersize, filesize, alignment)) {
+				ret = current + headersize;
+				break;
+			}
 			if ((ALIGN(current, alignment) + filesize < top)
 			    && (ALIGN(current, alignment) - headersize >
 				current)
 			    && in_segment(ALIGN(current, alignment), filesize,
-					  alignment))
-				return ALIGN(current, alignment);
+					  alignment)) {
+				ret = ALIGN(current, alignment);
+				break;
+			}
 			if ((ALIGN(current, alignment) + alignment + filesize <
 			     top)
 			    && (ALIGN(current, alignment) + alignment -
 				headersize > current)
 			    && in_segment(ALIGN(current, alignment) + alignment,
-					  filesize, alignment))
-				return ALIGN(current, alignment) + alignment;
+					  filesize, alignment)) {
+				ret = ALIGN(current, alignment) + alignment;
+				break;
+			}
 		}
 		current =
 		    ALIGN(current + ntohl(thisfile->len) +
 			  ntohl(thisfile->offset), align);
 	}
-	return 0;
+
+	free(rom);
+	return ret;
 }
diff --git a/util/cbfstool/common.h b/util/cbfstool/common.h
index 7ef50c41e1..4fcc1eee68 100644
--- a/util/cbfstool/common.h
+++ b/util/cbfstool/common.h
@@ -16,6 +16,9 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
  */
 
+#ifndef __CBFSTOOL_COMMON_H
+#define __CBFSTOOL_COMMON_H
+
 #include <stdint.h>
 #include "swab.h"
 #define ntohl(x)  (host_bigendian?(x):swab32(x))
@@ -24,8 +27,7 @@
 #define htonll(x) (host_bigendian?(x):swab64(x))
 
 extern void *offset;
-extern struct cbfs_header *master_header;
-extern uint32_t phys_start, phys_end, align, romsize;
+extern uint32_t romsize;
 extern int host_bigendian;
 
 static inline void *phys_to_virt(uint32_t addr)
@@ -40,7 +42,7 @@ static inline uint32_t virt_to_phys(void *addr)
 
 #define ALIGN(val, by) (((val) + (by)-1)&~((by)-1))
 
-uint32_t getfilesize(const char *filename);
+size_t getfilesize(const char *filename);
 void *loadfile(const char *filename, uint32_t * romsize_p, void *content,
 	       int place);
 void *loadrom(const char *filename);
@@ -77,4 +79,6 @@ uint32_t cbfs_find_location(const char *romfile, uint32_t filesize,
 
 void print_supported_filetypes(void);
 
-#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
+#define ARRAY_SIZE(a) (int)(sizeof(a) / sizeof((a)[0]))
+
+#endif