[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