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 <m...@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 <benja...@sipsolutions.net> > + * C implementation based on generation-script by Christian Lamparter > <chunk...@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@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel