[OpenWrt-Devel] [PATCH] build: add mkrasimage

Michael Heimpold mhei at heimpold.de
Wed Aug 15 15:47:56 EDT 2018


Hi David,

a few code-styling nitpicks, see comments below:

Am Mittwoch, 15. August 2018, 16:44:03 CEST schrieb David Bauer:
> The current make-ras.sh image generation script for the ZyXEL NBG6617
> has portability issues with bash. Because of this, factory images are
> currently not built correctly by the OpenWRT buildbots.
> 
> This commit replaces the make-ras.sh by C-written mkrasimage. The old
> script is still kept but can be deleted in the future.
> 
> Signed-off-by: David Bauer <mail at david-bauer.net>
> ---
>  include/image-commands.mk             |  13 +
>  target/linux/ipq40xx/image/Makefile   |   2 +-
>  tools/firmware-utils/Makefile         |   1 +
>  tools/firmware-utils/src/mkrasimage.c | 374 ++++++++++++++++++++++++++
>  4 files changed, 389 insertions(+), 1 deletion(-)
>  create mode 100644 tools/firmware-utils/src/mkrasimage.c
> 
> diff --git a/include/image-commands.mk b/include/image-commands.mk
> index 3cc5dc21e1..bb5fe46e1a 100644
> --- a/include/image-commands.mk
> +++ b/include/image-commands.mk
> @@ -62,6 +62,19 @@ define Build/make-ras
>  	@mv $@.new $@
>  endef
> 
> +define Build/zyxel-ras-image
> +	let \
> +		newsize="$(subst k,* 1024,$(RAS_ROOTFS_SIZE))"; \
> +		$(STAGING_DIR_HOST)/bin/mkrasimage \
> +			-b $(RAS_BOARD) \
> +			-v $(RAS_VERSION) \
> +			-k $(call param_get_default,kernel,$(1),$(IMAGE_KERNEL)) \
> +			-r $@ \
> +			-s $$newsize \
> +			-o $@.new
> +	@mv $@.new $@
> +endef
> +
>  define Build/mkbuffaloimg
>  	$(STAGING_DIR_HOST)/bin/mkbuffaloimg -B $(BOARDNAME) \
>  		-R $$(($(subst k, * 1024,$(ROOTFS_SIZE)))) \
> diff --git a/target/linux/ipq40xx/image/Makefile
> b/target/linux/ipq40xx/image/Makefile index d1ee1004fd..6e4125db0b 100644
> --- a/target/linux/ipq40xx/image/Makefile
> +++ b/target/linux/ipq40xx/image/Makefile
> @@ -221,7 +221,7 @@ define Device/zyxel_nbg6617
>  #	at least as large as the one of the initial firmware image (not the
> current #	one on the device). This only applies to the Web-UI, the
> bootlaoder ignores #	this minimum-size. However, the larger image can be
> flashed both ways. -	IMAGE/factory.bin := append-rootfs | pad-rootfs |
> check-size $$$$(ROOTFS_SIZE) | make-ras +	IMAGE/factory.bin :=
> append-rootfs | pad-rootfs | check-size $$$$(ROOTFS_SIZE) | zyxel-ras-image
> IMAGE/sysupgrade.bin/squashfs := append-rootfs | pad-rootfs | check-size
> $$$$(ROOTFS_SIZE) | sysupgrade-tar rootfs=$$$$@ | append-metadata
> DEVICE_PACKAGES := ipq-wifi-zyxel_nbg6617 uboot-envtools
>  endef
> diff --git a/tools/firmware-utils/Makefile b/tools/firmware-utils/Makefile
> index 436a43794c..00917c3417 100644
> --- a/tools/firmware-utils/Makefile
> +++ b/tools/firmware-utils/Makefile
> @@ -70,6 +70,7 @@ define Host/Compile
>  	$(call cc,fix-u-media-header cyg_crc32,-Wall)
>  	$(call cc,hcsmakeimage bcmalgo)
>  	$(call cc,mkporayfw, -Wall)
> +	$(call cc,mkrasimage, --std=gnu99)
>  	$(call cc,mkhilinkfw, -lcrypto)
>  	$(call cc,mkdcs932, -Wall)
>  	$(call cc,mkheader_gemtek,-lz)
> diff --git a/tools/firmware-utils/src/mkrasimage.c
> b/tools/firmware-utils/src/mkrasimage.c new file mode 100644
> index 0000000000..1cac7b5da9
> --- /dev/null
> +++ b/tools/firmware-utils/src/mkrasimage.c
> @@ -0,0 +1,374 @@
> +/*
> + * --- ZyXEL header format ---
> + * Original Version by Benjamin Berg <benjamin at sipsolutions.net>
> + * C implementation based on generation-script by Christian Lamparter
> <chunkeey at gmail.com> + *
> + * The firmware image prefixed with a header (which is written into the MTD
> device). + * The header is one erase block (~64KiB) in size, but the
> checksum only convers the + * first 2KiB. Padding is 0xff. All integers are
> in big-endian.
> + *
> + * The checksum is always a 16-Bit System V checksum (sum -s) stored in a
> 32-Bit integer. + *
> + *   4 bytes:  checksum of the rootfs image
> + *   4 bytes:  length of the contained rootfs image file (big endian)
> + *  32 bytes:  Firmware Version string (NUL terminated, 0xff padded)
> + *   4 bytes:  checksum over the header partition (big endian - see below)
> + *  64 bytes:  Model (e.g. "NBG6617", NUL termiated, 0xff padded)
> + *   4 bytes:  checksum of the kernel partition
> + *   4 bytes:  length of the contained kernel image file (big endian)
> + *      rest:  0xff padding (To erase block size)
> + *
> + * The checksums are calculated by adding up all bytes and if a 16bit
> + * overflow occurs, one is added and the sum is masked to 16 bit:
> + *   csum = csum + databyte; if (csum > 0xffff) { csum += 1; csum &= 0xffff
> }; + * Should the file have an odd number of bytes then the byte len-0x800
> is + * used additionally.
> + *
> + * The checksum for the header is calculated over the first 2048 bytes with
> + * the rootfs image checksum as the placeholder during calculation. + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + */
> +#include <byteswap.h>
> +#include <getopt.h>
> +#include <libgen.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +#define VERSION_STRING_LEN 31
> +#define ROOTFS_HEADER_LEN 40
> +
> +#define KERNEL_HEADER_LEN 8
> +
> +#define BOARD_NAME_LEN 64
> +#define BOARD_HEADER_LEN 68
> +
> +#define HEADER_PARTITION_CALC_LENGTH 2048
> +#define HEADER_PARTITION_LENGTH 0x10000
> +
> +struct file_info {
> +    char *name;    /* name of the file */
> +    char *data;    /* file content */
> +    size_t size;   /* length of the file */
> +};
> +
> +static char *progname;
> +
> +static char *board_name = 0;
> +static char *version_name = 0;
> +static unsigned int rootfs_size = 0;
> +
> +static struct file_info kernel = {0, 0, 0};

This is technically correct, but I would prefer { NULL, NULL, 0 } (note also 
the whitespace), because using 0 for pointers looks unusual.

> +static struct file_info rootfs = {0, 0, 0};
> +static struct file_info rootfs_out = {0, 0, 0};
> +static struct file_info out = {0, 0, 0};
> +
> +#define ERR(fmt, ...) do { \
> +    fflush(0); \

stderr is unbuffered by default, so no need to use fflush

> +    fprintf(stderr, "[%s] *** error: " fmt "\n", \
> +            progname, ## __VA_ARGS__ ); \
> +} while (0)
> +
> +void bufferFile(struct file_info *finfo) {

camelCase while the rest of your code does not use this style?
Please don't use camelCase in C applications, it's not so common.

> +    unsigned int fs = 0;
> +    FILE *f = NULL;
> +
> +    f = fopen(finfo->name, "rb");
> +    if (f == NULL) {
> +        printf("Error while opening file %s.", finfo->name);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    fseek(f, 0L, SEEK_END);
> +    fs = (unsigned int) ftell(f);
> +    rewind(f);
> +
> +    finfo->size = fs;
> +
> +    char *data = malloc(fs);

I would prefer declaration at function beginning.

> +    finfo->data = data;
> +    size_t read = fread(data, fs, 1, f);

Same here. And since read(2) is a well-known function, I would
rename the variable.

> +
> +    if (read != 1) {
> +        printf("Error reading file %s.", finfo->name);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    fclose(f);
> +}

Maybe it's easier to mmap(2) the whole file instead of so much
code to get the file into memory?

> +
> +void writeFile(struct file_info *finfo) {

I would prefer the opening bracket { on the next line when function begin.

> +    FILE *fout = fopen(finfo->name, "w");
> +
> +    if (!fwrite(finfo->data, finfo->size, 1, fout)) {
> +        printf("Wanted to write, but something went wrong.\n");

Usually you really want to know _what_ exactly went wrong (disk full, 
permissions...), so use ferror(3).

> +        exit(1);
> +    }
> +}
> +
> +void usage(int status) {
> +    FILE *stream = (status != EXIT_SUCCESS) ? stderr : stdout;
> +
> +    fprintf(stream, "Usage: %s [OPTIONS...]\n", progname);
> +    fprintf(stream,
> +            "\n"
> +            "Options:\n"
> +            "  -k <kernel>     path for kernel image\n"
> +            "  -r <rootfs>     path for rootfs image\n"
> +            "  -s <rfssize>    size of output rootfs\n"
> +            "  -v <version>    version string\n"
> +            "  -b <boardname>  name of board to generate image for\n"
> +            "  -h              show this screen\n"
> +    );
> +
> +    exit(status);
> +}
> +
> +static int sysv_chksm(const unsigned char *data, int size) {
> +    int r;
> +    int checksum;
> +    unsigned int s = 0; /* The sum of all the input bytes, modulo (UINT_MAX
> + 1).  */ +
> +
> +    for (int i = 0; i < size; i++) {
> +        s += data[i];
> +    }
> +
> +    r = (s & 0xffff) + ((s & 0xffffffff) >> 16);
> +    checksum = (r & 0xffff) + (r >> 16);
> +
> +    return checksum;
> +}
> +
> +char *generate_rootfs_header(struct file_info rootfs, char *version) {
> +    size_t version_string_length;
> +    unsigned int chksm, size;
> +    char *rootfs_header;
> +    size_t ptr = 0;
> +
> +    rootfs_header = malloc(ROOTFS_HEADER_LEN);

What if malloc failed?

> +    memset(rootfs_header, 0xff, ROOTFS_HEADER_LEN);
> +
> +    memcpy(rootfs_out.data, rootfs.data, rootfs.size);
> +
> +    chksm = __bswap_32(sysv_chksm(rootfs_out.data, rootfs_out.size));
> +    size = __bswap_32(rootfs_out.size);
> +
> +    memcpy(rootfs_header + ptr, &chksm, 4);
> +    ptr += 4;
> +
> +    memcpy(rootfs_header + ptr, &size, 4);
> +    ptr += 4;
> +
> +    version_string_length = strlen(version) <= VERSION_STRING_LEN ?
> strlen(version) : VERSION_STRING_LEN; +
> +    memcpy(rootfs_header + ptr, version, version_string_length);
> +    ptr += version_string_length;
> +
> +    rootfs_header[ptr] = 0x0;
> +
> +    return rootfs_header;
> +}
> +
> +char *generate_kernel_header(struct file_info kernel) {
> +    unsigned int chksm, size;
> +    char *kernel_header;
> +    size_t ptr = 0;
> +
> +    kernel_header = malloc(KERNEL_HEADER_LEN);
> +    memset(kernel_header, 0xff, KERNEL_HEADER_LEN);
> +
> +    chksm = __bswap_32(sysv_chksm(kernel.data, kernel.size));

Calling a function starting with underscores looks really wrong. What 
endianess the result should have?
Are you sure that this runs correctly on platforms? I would expect that
swapping is only necessary on platform which have different endianess
that the result should have...

> +    size = __bswap_32(kernel.size);
> +
> +    memcpy(kernel_header + ptr, &chksm, 4);

Some lines below, you are using the array style xyz[ptr], so I would prefer
one style...

> +    ptr += 4;
> +
> +    memcpy(kernel_header + ptr, &size, 4);
> +
> +    return kernel_header;
> +}
> +
> +unsigned int generate_board_header_checksum(char *kernel_hdr, char
> *rootfs_hdr, char *boardname) { +    char *board_hdr_tmp;
> +    size_t ptr = 0;
> +
> +    board_hdr_tmp = malloc(HEADER_PARTITION_CALC_LENGTH);
> +    memset(board_hdr_tmp, 0xff, HEADER_PARTITION_CALC_LENGTH);
> +
> +    memcpy(board_hdr_tmp, rootfs_hdr, ROOTFS_HEADER_LEN);
> +    ptr += ROOTFS_HEADER_LEN;
> +
> +    memcpy(board_hdr_tmp + ptr, rootfs_hdr, 4); /* Use RootFS checksum as
> placeholder */ +    ptr += 4;
> +
> +    memcpy(board_hdr_tmp + ptr, boardname, strlen(boardname)); /* Append
> boardname */ +    ptr += strlen(boardname);
> +
> +    board_hdr_tmp[ptr] = 0x0; /* Add null-terminator */
> +    ptr = ROOTFS_HEADER_LEN + 4 + BOARD_NAME_LEN;
> +
> +    memcpy(board_hdr_tmp + ptr, kernel_hdr, 8);
> +
> +    return __bswap_32(sysv_chksm(board_hdr_tmp, 2048));
> +}
> +
> +char *generate_board_header(char *kernel_hdr, char *rootfs_hdr, char
> *boardname) { +    unsigned int board_checksum;
> +    char *board_hdr;
> +
> +    board_hdr = malloc(BOARD_HEADER_LEN);
> +    memset(board_hdr, 0xff, BOARD_HEADER_LEN);
> +
> +    board_checksum = generate_board_header_checksum(kernel_hdr, rootfs_hdr,
> boardname); +
> +    memcpy(board_hdr, &board_checksum, 4);
> +    memcpy(board_hdr + 4, boardname, strlen(boardname));
> +    board_hdr[4 + strlen(boardname)] = 0x0;
> +
> +    return board_hdr;
> +}
> +
> +int build_image() {
> +    char *rootfs_header, *kernel_header, *board_header;
> +
> +    size_t ptr;
> +
> +    /* Load files */
> +    bufferFile(&kernel);
> +    bufferFile(&rootfs);
> +
> +    /* Allocate memory for temporary ouput rootfs */
typo

> +    rootfs_out.data = malloc(rootfs_out.size);
> +    memset(rootfs_out.data, 0x00, rootfs_out.size);

Use calloc: https://vorpus.org/blog/why-does-calloc-exist/

> +
> +    /* Prepare headers */
> +    rootfs_header = generate_rootfs_header(rootfs, version_name);
> +    kernel_header = generate_kernel_header(kernel);
> +    board_header = generate_board_header(kernel_header, rootfs_header,
> board_name); +
> +    /* Prepare output file */
> +    out.size = HEADER_PARTITION_LENGTH + rootfs_out.size + kernel.size;
> +    out.data = malloc(out.size);
> +    memset(out.data, 0xFF, out.size);
> +
> +    /* Build output image */
> +    memcpy(out.data, rootfs_header, ROOTFS_HEADER_LEN);
> +    memcpy(out.data + ROOTFS_HEADER_LEN, board_header, BOARD_HEADER_LEN);
> +    memcpy(out.data + ROOTFS_HEADER_LEN + BOARD_HEADER_LEN, kernel_header,
> KERNEL_HEADER_LEN); +    ptr = HEADER_PARTITION_LENGTH;
> +    memcpy(out.data + ptr, rootfs_out.data, rootfs_out.size);
> +    ptr += rootfs_out.size;
> +    memcpy(out.data + ptr, kernel.data, kernel.size);
> +
> +    /* Write back output image */
> +    writeFile(&out);
> +
> +    /* Free allocated memory */
> +    free(kernel.data);
> +    free(rootfs.data);
> +    free(out.data);
> +    free(rootfs_out.data);
> +
> +    free(rootfs_header);
> +    free(kernel_header);
> +    free(board_header);
> +
> +    return 0;
> +}
> +
> +int check_options() {
> +    if (kernel.name == 0) {
Please use NULL or "if (!kernel.name) {"

> +        ERR("No kernel filename supplied");
> +        return -1;
> +    }
> +
> +    if (rootfs.name == 0) {
> +        ERR("No rootfs filename supplied");
> +        return -2;
> +    }
> +
> +    if (out.name == 0) {
> +        ERR("No output filename supplied");
> +        return -3;
> +    }
> +
> +    if (board_name == 0) {
> +        ERR("No board-name supplied");
> +        return -4;
> +    }
> +
> +    if (version_name == 0) {
> +        ERR("No version supplied");
> +        return -5;
> +    }
> +
> +    if (rootfs_size <= 0) {
> +        ERR("Invalid rootfs size supplied");
> +        return -6;
> +    }
> +
> +    if (strlen(board_name) > 31) {
> +        ERR("Board name is to long");
> +        return -7;
> +    }
> +    return 0;
> +}
> +
> +int main(int argc, char *argv[]) {
> +    int ret;
> +    progname = basename(argv[0]);
> +    while (1) {
> +        int c;
> +
> +        c = getopt(argc, argv, "b:k:o:r:s:v:h");
> +        if (c == -1)
> +            break;
> +
> +        switch (c) {
> +            case 'b':
> +                board_name = optarg;
> +                break;
> +            case 'h':
> +		usage(EXIT_SUCCESS);
> +		break;
> +            case 'k':
> +                kernel.name = optarg;
> +                break;
> +            case 'o':
> +                out.name = optarg;
> +                break;
> +            case 'r':
> +                rootfs.name = optarg;
> +                break;
> +            case 's':
> +                sscanf(optarg, "%u", &rootfs_size);
> +                break;
> +            case 'v':
> +                version_name = optarg;
> +                break;
> +            default:
> +                usage(EXIT_FAILURE);
> +                break;
> +        }
> +    }
> +
> +    ret = check_options();
> +    if (ret)
> +        usage(EXIT_FAILURE);
> +
> +    /* As ZyXEL Web-GUI only accept images with a rootfs equal or larger
> than the first firmware shipped +     * for the device, we need to pad
> rootfs partition to this size. To perform further calculations, we +     *
> decide the size of this part here. In case the rootfs we want to integrate
> in our image is larger, +     * take it's size, otherwise the supplied
> size.
> +     *
> +     * Be careful! We rely on assertion of correct size to be performed
> beforehand. It is unknown if images +     * with a to large rootfs are
> accepted or not.
> +     */
> +    rootfs_out.size = rootfs_size < rootfs.size ? rootfs.size :
> rootfs_size; +    return build_image();
> +}

There are also some warnings when compiling with -Wall, maybe you could have a 
look.

Thanks,
Michael




_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel



More information about the openwrt-devel mailing list