Re: [coreboot] [PATCH] [RESEND] cbfstool "add-lzma" method

[Paul Menzel]

Dear Kevin,

Am Sonntag, den 16.01.2011, 13:23 -0500 schrieb Kevin O'Connor:
> This is a resend of a patch I sent last year.  Current distros ship a
> version of lzma that does not set the "size" property of the
> compressed archive.  This makes files compressed via the system
> version of lzma unusable for coreboot and seabios.  This patch adds a
> helper to cbfstool so users can easily add compressed "raw" files.
>
> -Kevin
>
> --------------------------- patch -----------------------
>
> Enhance cbfstool so that it can support "cbfstool ROM add-lzma FILE
> NAME" calls.  This is useful for callers that wish to add an lzma
> compressed raw file.
>
> Right now, SeaBIOS supports lzma raw files for things like floppy
> images.  Today, adding one of these files requires a two step process
> for example:
>
> $ lzma -zc /path/to/myfloppy.img > myfloppy.img.lzma
> $ cbfstool coreboot.rom add myfloppy.img.lzma floppyimg/MyFloppy.lzma raw
>
> Unfortunately, various versions of "lzma" are quirky and the above can
> be troublesome.

could you give a hint what versions of `lzma` are quirky. Is that an
upstream problem and is this problem reported upstream?
> With this patch, a user need only execute:

s/a user need only execute:/a user only needs to execute:/ (?)
> $ cbfstool coreboot.rom add-lzma myfloppy.img floppyimg/MyFloppy.lzma
> Signed-off-by: Kevin O'Connor
>
> util/cbfstool/cbfs.h     |    1 +
>  util/cbfstool/cbfstool.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++
>  util/cbfstool/common.c   |    1 +
>  3 files changed, 73 insertions(+), 0 deletions(-)
>
> diff --git a/util/cbfstool/cbfs.h b/util/cbfstool/cbfs.h
> index 6fb9edd..44c1fa0 100644
> --- a/util/cbfstool/cbfs.h
> +++ b/util/cbfstool/cbfs.h
> @@ -73,6 +73,7 @@ struct cbfs_payload {
>  #define CBFS_COMPONENT_OPTIONROM  0x30
>  #define CBFS_COMPONENT_BOOTSPLASH 0x40
>  #define CBFS_COMPONENT_RAW        0x50
> +#define CBFS_COMPONENT_LZMA_RAW   0x40000050
>  #define CBFS_COMPONENT_VSA        0x51
>  #define CBFS_COMPONENT_MBI        0x52
>  #define CBFS_COMPONENT_MICROCODE  0x53
> diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
> index 507edc2..d437994 100644
> --- a/util/cbfstool/cbfstool.c
> +++ b/util/cbfstool/cbfstool.c
> @@ -18,6 +18,7 @@
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
>
> */
>
> +#include <stdlib.h>

I do not know the coding style. Are headers sorted alphabetically?
> #include <stdio.h>
>  #include <string.h>
>  #include "common.h"
> @@ -27,6 +28,7 @@ typedef enum {
>    CMD_ADD,
>    CMD_ADD_PAYLOAD,
>    CMD_ADD_STAGE,
> CMD_ADD_LZMA,
> CMD_CREATE,
> CMD_LOCATE,
> CMD_PRINT,
>
> @@ -187,6 +189,73 @@ static int cbfs_add_stage(int argc, char **argv)
>    return 0;
>  }
>
> +static int cbfs_add_lzma(int argc, char **argv)
> +{
> char *romname = argv[1];
> char *cmd = argv[2];
> void *rom = loadrom(romname);
> if (rom == NULL) {
> printf("Could not load ROM image '%s'.\n", romname);
> return 1;
> }
> if (argc < 5) {
> printf("not enough arguments to '%s'.\n", cmd);

Capital letter at the start and s/to/to run/ (?).

Sorry if this is incorrect. I am no native speaker.
> return 1;
> }
> char *filename = argv[3];
> char *cbfsname = argv[4];
> uint32_t filesize = 0;
> void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
> if (filedata == NULL) {
> printf("Could not load file '%s'.\n", filename);
> return 1;
> }
> uint32_t base = 0;
> void *cbfsfile = NULL;
> uint32_t type = CBFS_COMPONENT_LZMA_RAW;
> if (argc > 5) {
> 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);
> }
> uint32_t compresssize = filesize;
> void *compressed_data;
> for (;;) {
> compressed_data = malloc(compresssize);
> if (! compressed_data) {
> 	printf("Could not allocate %d space\n", compresssize);
> 	return 1;
> }
> uint32_t actualsize = compresssize;
> extern void do_lzma_compress(char *in, int in_len,
> 			     char *out, int *out_len);
> do_lzma_compress(filedata, filesize, compressed_data,
> 		 &compresssize);
> if (compresssize <= actualsize)
> 	break;
> // Try again with required size.
> free(compressed_data);
> }
> cbfsfile = create_cbfs_file(cbfsname, compressed_data,
> 		    &compresssize, type, &base);
> if (add_file_to_cbfs(cbfsfile, compresssize, base))
> return 1;
> if (writerom(romname, rom, romsize))
> return 1;
> return 0;
>
> +}
>
> static int cbfs_create(int argc, char **argv)
>  {
>    char *romname = argv[1];
> @@ -269,6 +338,7 @@ struct command commands[] = {
>    {CMD_ADD, "add", cbfs_add},
>    {CMD_ADD_PAYLOAD, "add-payload", cbfs_add_payload},
>    {CMD_ADD_STAGE, "add-stage", cbfs_add_stage},
> {CMD_ADD_LZMA, "add-lzma", cbfs_add_lzma},
> {CMD_CREATE, "create", cbfs_create},
> {CMD_LOCATE, "locate", cbfs_locate},
> {CMD_PRINT, "print", cbfs_print},
>
> @@ -286,6 +356,7 @@ void usage(void)
>         " 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-lzma FILE NAME [TYPE] [base]     Lzma compress and add\n"
>   " create SIZE BOOTBLOCK [ALIGN]        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"
>
> diff --git a/util/cbfstool/common.c b/util/cbfstool/common.c
> index 0fb0200..788d843 100644
> --- a/util/cbfstool/common.c
> +++ b/util/cbfstool/common.c
> @@ -142,6 +142,7 @@ struct filetypes_t {
>    {CBFS_COMPONENT_OPTIONROM, "optionrom"},
>    {CBFS_COMPONENT_BOOTSPLASH, "bootsplash"},
>    {CBFS_COMPONENT_RAW, "raw"},
> {CBFS_COMPONENT_LZMA_RAW, "lzma-raw"},
> {CBFS_COMPONENT_VSA, "vsa"},
> {CBFS_COMPONENT_MBI, "mbi"},
> {CBFS_COMPONENT_MICROCODE, "microcode"},

I have not had time to test, so only

Reviewed-by: Paul Menzel

Thanks,

Paul